Patchwork [4,of,4] ui: edit(): transplant: set HGREVISION environment variable for an editor

login
register
mail settings
Submitter Alexander Drozdov
Date Feb. 7, 2014, 4:26 a.m.
Message ID <8b9f2ec2dde1e08580bd.1391747161@localhost6.localdomain6>
Download mbox | patch
Permalink /patch/3509/
State Superseded
Headers show

Comments

Alexander Drozdov - Feb. 7, 2014, 4:26 a.m.
# HG changeset patch
# User Alexander Drozdov <al.drozdov@gmail.com>
# Date 1391746185 -14400
#      Fri Feb 07 08:09:45 2014 +0400
# Node ID 8b9f2ec2dde1e08580bd5893ce6253acc940a837
# Parent  01079ca49c16f7c0c3d53ba673f424368aea075e
ui: edit(): transplant: set HGREVISION environment variable for an editor

transplant command set 'transplant_source' extra for the revision.
Allow an editor to access the extra using HGREVISION environment variable.

This may be useful when an editor is actually a script which modifies a commit
message. Transplant filters is an alternative way to do it.
Mads Kiilerich - Feb. 7, 2014, 1:24 p.m.
On 02/07/2014 05:26 AM, Alexander Drozdov wrote:
> # HG changeset patch
> # User Alexander Drozdov <al.drozdov@gmail.com>
> # Date 1391746185 -14400
> #      Fri Feb 07 08:09:45 2014 +0400
> # Node ID 8b9f2ec2dde1e08580bd5893ce6253acc940a837
> # Parent  01079ca49c16f7c0c3d53ba673f424368aea075e
> ui: edit(): transplant: set HGREVISION environment variable for an editor
>
> transplant command set 'transplant_source' extra for the revision.
> Allow an editor to access the extra using HGREVISION environment variable.

But ... it _is_ not this HGREVISION. Calling it that would be confusing. 
It is more like what we in revsets refer to as 'origin'.

I think it would be better to follow the HG_ naming convention used when 
Mercurial passes values to hooks. That would be inconsistent with HGUSER 
... but HGUSER is primarily input to Mercurial, not output...

>
> This may be useful when an editor is actually a script which modifies a commit
> message. Transplant filters is an alternative way to do it.

In my opinion this patch set is split up in so small pieces that the 
individual pieces don't make sense.

Why take an extra parameter if it never is used? How can it be correct 
to pass any value for this parameter when it not is used and we don't 
know anything about the type or the semantics?

In this case I would prefer it all folded into one patch. Others might 
like it the way it is ...

> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -8,6 +8,7 @@
>   from i18n import _
>   import errno, getpass, os, socket, sys, tempfile, traceback
>   import config, scmutil, util, error, formatter
> +from node import hex
>   
>   class ui(object):
>       def __init__(self, src=None):
> @@ -721,6 +722,10 @@
>               f.close()
>   
>               environ = {'HGUSER': user}
> +            for label in ('transplant_source',):
> +                if label in extra:
> +                    environ.update({'HGREVISION': hex(extra[label])})
> +                    break

Keep it simple. Why not just

+            if 'transplant_source' in extra:
+                environ.update({'HGREVISION': hex(extra['transplant_source'])})


/Mads
Alexander Drozdov - Feb. 7, 2014, 5:39 p.m.
On 07.02.2014 19:51:03, Mads Kiilerich <mads@kiilerich.com> wrote:
> On 02/07/2014 05:26 AM, Alexander Drozdov wrote:
>> # HG changeset patch
>> # User Alexander Drozdov <al.drozdov@gmail.com>
>> # Date 1391746185 -14400
>> #      Fri Feb 07 08:09:45 2014 +0400
>> # Node ID 8b9f2ec2dde1e08580bd5893ce6253acc940a837
>> # Parent  01079ca49c16f7c0c3d53ba673f424368aea075e
>> ui: edit(): transplant: set HGREVISION environment variable for an editor
>>
>> transplant command set 'transplant_source' extra for the revision.
>> Allow an editor to access the extra using HGREVISION environment variable.
>
> But ... it _is_ not this HGREVISION. Calling it that would be confusing. It is more like 
> what we in revsets refer to as 'origin'.
>
> I think it would be better to follow the HG_ naming convention used when Mercurial 
> passes values to hooks. That would be inconsistent with HGUSER ... but HGUSER is 
> primarily input to Mercurial, not output...
I've decided to use HGREVISION to allow users to move easier from transplant filters to 
graft. Transplant already sets HGREVISION for
--filter scripts. With HGREVISION, no changes in filters have to be done.

>>
>> This may be useful when an editor is actually a script which modifies a commit
>> message. Transplant filters is an alternative way to do it.
>
> In my opinion this patch set is split up in so small pieces that the individual pieces 
> don't make sense.
>
> Why take an extra parameter if it never is used? How can it be correct to pass any value 
> for this parameter when it not is used and we don't know anything about the type or the 
> semantics?
>
> In this case I would prefer it all folded into one patch. Others might like it the way 
> it is ...
Ok, I'll resend it all folded.
>> +            for label in ('transplant_source',):
>> +                if label in extra:
>> +                    environ.update({'HGREVISION': hex(extra[label])})
>> +                    break
>
> Keep it simple. Why not just
>
> +            if 'transplant_source' in extra:
> +                environ.update({'HGREVISION': hex(extra['transplant_source'])})

Ok, I'll replace that with your code.
Sean Farley - Feb. 7, 2014, 8:47 p.m.
al.drozdov@gmail.com writes:

> On 07.02.2014 19:51:03, Mads Kiilerich <mads@kiilerich.com> wrote:
>> On 02/07/2014 05:26 AM, Alexander Drozdov wrote:
>>> # HG changeset patch
>>> # User Alexander Drozdov <al.drozdov@gmail.com>
>>> # Date 1391746185 -14400
>>> #      Fri Feb 07 08:09:45 2014 +0400
>>> # Node ID 8b9f2ec2dde1e08580bd5893ce6253acc940a837
>>> # Parent  01079ca49c16f7c0c3d53ba673f424368aea075e
>>> ui: edit(): transplant: set HGREVISION environment variable for an editor
>>>
>>> transplant command set 'transplant_source' extra for the revision.
>>> Allow an editor to access the extra using HGREVISION environment variable.
>>
>> But ... it _is_ not this HGREVISION. Calling it that would be confusing. It is more like 
>> what we in revsets refer to as 'origin'.
>>
>> I think it would be better to follow the HG_ naming convention used when Mercurial 
>> passes values to hooks. That would be inconsistent with HGUSER ... but HGUSER is 
>> primarily input to Mercurial, not output...
> I've decided to use HGREVISION to allow users to move easier from transplant filters to 
> graft. Transplant already sets HGREVISION for
> --filter scripts. With HGREVISION, no changes in filters have to be done.

That's a good point. If you could, please make a note about it in the
patch description.

>>> This may be useful when an editor is actually a script which modifies a commit
>>> message. Transplant filters is an alternative way to do it.
>>
>> In my opinion this patch set is split up in so small pieces that the individual pieces 
>> don't make sense.

I would disagree with Mads here. They are small and simple to read as
they currently are.

>> Why take an extra parameter if it never is used? How can it be correct to pass any value 
>> for this parameter when it not is used and we don't know anything about the type or the 
>> semantics?

But these kinds of things are done all the time on the mercurial list?
In fact, I did it during GSoC last year. At most, I would just suggest
Alexander add a message like, "this parameter will be used in an
upcoming patch" or something similar to that.

>> In this case I would prefer it all folded into one patch. Others might like it the way 
>> it is ...
> Ok, I'll resend it all folded.

Before you do that, I would wait for Matt Mackall (mpm) to reply since
he usually prefers patches be smaller rather than folded together.

>>> +            for label in ('transplant_source',):
>>> +                if label in extra:
>>> +                    environ.update({'HGREVISION': hex(extra[label])})
>>> +                    break
>>
>> Keep it simple. Why not just
>>
>> +            if 'transplant_source' in extra:
>> +                environ.update({'HGREVISION': hex(extra['transplant_source'])})
>
> Ok, I'll replace that with your code.

I agree with Mads here (for once ;-)
Matt Mackall - Feb. 7, 2014, 10:23 p.m.
On Fri, 2014-02-07 at 14:47 -0600, Sean Farley wrote:
> >> In this case I would prefer it all folded into one patch. Others might like it the way 
> >> it is ...
> > Ok, I'll resend it all folded.
> 
> Before you do that, I would wait for Matt Mackall (mpm) to reply since
> he usually prefers patches be smaller rather than folded together.

Looked fine to me.

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -8,6 +8,7 @@ 
 from i18n import _
 import errno, getpass, os, socket, sys, tempfile, traceback
 import config, scmutil, util, error, formatter
+from node import hex
 
 class ui(object):
     def __init__(self, src=None):
@@ -721,6 +722,10 @@ 
             f.close()
 
             environ = {'HGUSER': user}
+            for label in ('transplant_source',):
+                if label in extra:
+                    environ.update({'HGREVISION': hex(extra[label])})
+                    break
             for label in ('source', 'rebase_source'):
                 if label in extra:
                     environ.update({'HGREVISION': extra[label]})