Patchwork mq: include involved files in qrefresh -e constructed message (issue3647)

login
register
mail settings
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

Davide Bolcioni - Dec. 4, 2012, 12:45 a.m.
# 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)
Kevin Bullock - Dec. 4, 2012, 5 a.m.
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
Pierre-Yves David - Dec. 4, 2012, 7:26 a.m.
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
Davide Bolcioni - Dec. 7, 2012, 9:10 p.m.
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.
Davide Bolcioni - Dec. 7, 2012, 9:13 p.m.
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.
Kevin Bullock - Dec. 7, 2012, 9:45 p.m.
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
Pierre-Yves David - Dec. 8, 2012, 12:37 a.m.
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: