| Submitter | Davide Bolcioni |
|---|---|
| Date | Dec. 4, 2012, 12:45 a.m. |
| Message ID | <85615e499caecbaf7583.1354581957@dev1103.prn1.facebook.com> |
| Download | mbox | patch |
| Permalink | /patch/11/ |
| State | Rejected |
| Headers | show |
Comments
On Dec 3, 2012, at 6:45 PM, Davide Bolcioni wrote: > # HG changeset patch > # User Davide Bolcioni <dbolcioni at fb.com> > # Date 1354053904 28800 > # Node ID 85615e499caecbaf758363fd90ad52b42e118a1c > # Parent fb14a5dcdc62987512820531fe60719d650491b6 > mq: include involved files in qrefresh -e constructed message (issue3647) I'm +1 on the idea, but... > diff --git a/hgext/mq.py b/hgext/mq.py > --- a/hgext/mq.py > +++ b/hgext/mq.py > @@ -2482,9 +2482,33 @@ > raise util.Abort(_('option "-e" incompatible with "-m" or "-l"')) > patch = q.applied[-1].name > ph = patchheader(q.join(patch), q.plainmode) > - message = ui.edit('\n'.join(ph.message), ph.user or ui.username()) > + > + (modified, added, removed, deleted) = repo.status()[:4] > + msg = [] > + > + if ph.message: > + msg.extend(ph.message) > + > + if not msg: > + msg.append(_('(replace this line with the message)')) We don't fill in a placeholder message for commit, why should qrefresh -e be any different? > + > + if msg[-1]: > + msg.append("") > + > + msg.append(_("HG: Lines beginning with 'HG:' are removed.")) > + > + msg.extend([_('HG: added %s') % f for f in added]) > + msg.extend([_('HG: changed %s') % f for f in modified]) > + msg.extend([_('HG: removed %s') % f for f in removed]) > + msg.extend([_('HG: deleted %s') % f for f in deleted]) > + > + message = ui.edit('\n'.join(msg), ph.user or ui.username()) > # We don't want to lose the patch message if qrefresh fails (issue2062) > + msg = [t for t in message.split('\n') if not t.startswith('HG: ')] Ugh, extending the message just to split and filter it back out again? pacem in terris / ??? / ?????? / ????????? / ?? Kevin R. Bullock
On 4 d?c. 2012, at 01:45, Davide Bolcioni wrote: > # HG changeset patch > # User Davide Bolcioni <dbolcioni at fb.com> > # Date 1354053904 28800 > # Node ID 85615e499caecbaf758363fd90ad52b42e118a1c > # Parent fb14a5dcdc62987512820531fe60719d650491b6 > mq: include involved files in qrefresh -e constructed message (issue3647) The standard way to do that would be to force editor on commit. You can see example usage in hg commit --amend or histedit see http://www.selenic.com/hg/file/5cafcac2414c/mercurial/cmdutil.py#l1734 and http://www.selenic.com/hg/file/5cafcac2414c/hgext/histedit.py#l261 passing commitforceeditor to newcommit should be enough We should not need to re-implement commit message generation here
Pierre-Yves David [mailto:pierre-yves.david at ens-lyon.org] wrote: > On 4 d?c. 2012, at 01:45, Davide Bolcioni wrote: > > > # HG changeset patch > > # User Davide Bolcioni <dbolcioni at fb.com> # Date 1354053904 28800 # > > Node ID 85615e499caecbaf758363fd90ad52b42e118a1c > > # Parent fb14a5dcdc62987512820531fe60719d650491b6 > > mq: include involved files in qrefresh -e constructed message > > (issue3647) > > The standard way to do that would be to force editor on commit. You > can see example usage in hg commit --amend or histedit > > see > http://www.selenic.com/hg/file/5cafcac2414c/mercurial/cmdutil.py#l1734 > and http://www.selenic.com/hg/file/5cafcac2414c/hgext/histedit.py#l261 > > passing commitforceeditor to newcommit should be enough > > We should not need to re-implement commit message generation here This is not a commit, it is a patch refresh. It might be implemented as a commit, after a number of gyrations, but the patch queue abstraction is another piece of context the user must keep in his mind while working and I want to keep them visually different to help with that. Besides, the files involved (added, removed, etc) are not guaranteed to be the same and the above suggestion might actually expose the implementation (didn't try). Davide Bolcioni -- There is no place like /home.
Kevin Bullock [kbullock+mercurial at ringworld.org] wrote > On Dec 3, 2012, at 6:45 PM, Davide Bolcioni wrote: > > > # HG changeset patch > > # User Davide Bolcioni <dbolcioni at fb.com> # Date 1354053904 28800 # > > Node ID 85615e499caecbaf758363fd90ad52b42e118a1c > > # Parent fb14a5dcdc62987512820531fe60719d650491b6 > > mq: include involved files in qrefresh -e constructed message > > (issue3647) [cut] > > + if not msg: > > + msg.append(_('(replace this line with the message)')) > > We don't fill in a placeholder message for commit, why should qrefresh > -e be any different? The MQ extension adds more stuff to juggle in your mind as part of the context during the workflow, so making this look different from a commit seems advisable to me in order to remind the user that the two visually similar operations actually are somewhat different. I am open to suggestions about the wording, e.g. "... with the patch refresh reason". > > + msg.append(_("HG: Lines beginning with 'HG:' are > > + removed.")) > > + > > + msg.extend([_('HG: added %s') % f for f in added]) > > + msg.extend([_('HG: changed %s') % f for f in modified]) > > + msg.extend([_('HG: removed %s') % f for f in removed]) > > + msg.extend([_('HG: deleted %s') % f for f in deleted]) > > + > > + message = ui.edit('\n'.join(msg), ph.user or ui.username()) > > # We don't want to lose the patch message if qrefresh fails > > (issue2062) > > + msg = [t for t in message.split('\n') if not > > + t.startswith('HG: ')] > > Ugh, extending the message just to split and filter it back out again? The HG: lines are only there to help the user mentally lock on context. If I pass them to ui.edit(), they can come back and I do not want them in the patch message since they duplicate information which is in the patch already. Besides, promises are promises (cf. "Lines beginning with HG ..."). Davide Bolcioni -- There is no place like /home.
On Dec 7, 2012, at 3:13 PM, Davide Bolcioni wrote: > Kevin Bullock [kbullock+mercurial at ringworld.org] wrote > >> On Dec 3, 2012, at 6:45 PM, Davide Bolcioni wrote: >> >>> # HG changeset patch >>> # User Davide Bolcioni <dbolcioni at fb.com> # Date 1354053904 28800 # >>> Node ID 85615e499caecbaf758363fd90ad52b42e118a1c >>> # Parent fb14a5dcdc62987512820531fe60719d650491b6 >>> mq: include involved files in qrefresh -e constructed message >>> (issue3647) > > [cut] > >>> + if not msg: >>> + msg.append(_('(replace this line with the message)')) >> >> We don't fill in a placeholder message for commit, why should qrefresh >> -e be any different? > > The MQ extension adds more stuff to juggle in your mind as part of the context during the workflow, so making this look different from a commit seems advisable to me in order to remind the user that the two visually similar operations actually are somewhat different. I am open to suggestions about the wording, e.g. "... with the patch refresh reason". Mmmm, nope, not good enough. 1. Your placeholder text does nothing to remind the user that this is a qrefresh. 2. Adding a commit-message placeholder text isn't in line with your patch description. 3. The user might change their mind about editing the message; with your patch, they would (probably?) get a patch message set to the placeholder text. 4. In any case, we really don't do placeholder text AFAICT. >>> + msg.append(_("HG: Lines beginning with 'HG:' are >>> + removed.")) >>> + >>> + msg.extend([_('HG: added %s') % f for f in added]) >>> + msg.extend([_('HG: changed %s') % f for f in modified]) >>> + msg.extend([_('HG: removed %s') % f for f in removed]) >>> + msg.extend([_('HG: deleted %s') % f for f in deleted]) >>> + >>> + message = ui.edit('\n'.join(msg), ph.user or ui.username()) >>> # We don't want to lose the patch message if qrefresh fails >>> (issue2062) >>> + msg = [t for t in message.split('\n') if not >>> + t.startswith('HG: ')] >> >> Ugh, extending the message just to split and filter it back out again? > > The HG: lines are only there to help the user mentally lock on context. If I pass them to ui.edit(), they can come back and I do not want them in the patch message since they duplicate information which is in the patch already. Besides, promises are promises (cf. "Lines beginning with HG ..."). I just meant that you could save the undecorated message so that you don't have to remove the same 'HG:' lines you yourself added. pacem in terris / ??? / ?????? / ????????? / ?? Kevin R. Bullock
On 7 d?c. 2012, at 22:13, Davide Bolcioni wrote: > Kevin Bullock [kbullock+mercurial at ringworld.org] wrote > >> On Dec 3, 2012, at 6:45 PM, Davide Bolcioni wrote: >> >>> # HG changeset patch >>> # User Davide Bolcioni <dbolcioni at fb.com> # Date 1354053904 28800 # >>> Node ID 85615e499caecbaf758363fd90ad52b42e118a1c >>> # Parent fb14a5dcdc62987512820531fe60719d650491b6 >>> mq: include involved files in qrefresh -e constructed message >>> (issue3647) > > [cut] > >>> + if not msg: >>> + msg.append(_('(replace this line with the message)')) >> >> We don't fill in a placeholder message for commit, why should qrefresh >> -e be any different? > > The MQ extension adds more stuff to juggle in your mind as part of the context during the workflow, so making this look different from a commit seems advisable to me in order to remind the user that the two visually similar operations actually are somewhat different. I am open to suggestions about the wording, e.g. "... with the patch refresh reason". My point is: Is there any strong semantical different between $ hg qrefresh $ hg commit --amend I do not think so and the commit --amend seems right to me currently. You have yet to convinces me of the contrary. Adding more details that an amend/refresh/histedit is in progress may be fine, but we should not alter the core logic.
Patch
diff --git a/hgext/mq.py b/hgext/mq.py --- a/hgext/mq.py +++ b/hgext/mq.py @@ -2482,9 +2482,33 @@ raise util.Abort(_('option "-e" incompatible with "-m" or "-l"')) patch = q.applied[-1].name ph = patchheader(q.join(patch), q.plainmode) - message = ui.edit('\n'.join(ph.message), ph.user or ui.username()) + + (modified, added, removed, deleted) = repo.status()[:4] + msg = [] + + if ph.message: + msg.extend(ph.message) + + if not msg: + msg.append(_('(replace this line with the message)')) + + if msg[-1]: + msg.append("") + + msg.append(_("HG: Lines beginning with 'HG:' are removed.")) + + msg.extend([_('HG: added %s') % f for f in added]) + msg.extend([_('HG: changed %s') % f for f in modified]) + msg.extend([_('HG: removed %s') % f for f in removed]) + msg.extend([_('HG: deleted %s') % f for f in deleted]) + + message = ui.edit('\n'.join(msg), ph.user or ui.username()) # We don't want to lose the patch message if qrefresh fails (issue2062) + msg = [t for t in message.split('\n') if not t.startswith('HG: ')] + message = '\n'.join(msg) + ph.setmessage(message) repo.savecommitmessage(message) + setupheaderopts(ui, opts) wlock = repo.wlock() try: