[PATCH 4 of 4] ui: edit(): transplant: set HGREVISION environmentvariable for an editorr

Sean Farley sean.michael.farley at gmail.com
Fri Feb 7 14:47:08 CST 2014


al.drozdov at gmail.com writes:

> On 07.02.2014 19:51:03, Mads Kiilerich <mads at kiilerich.com> wrote:
>> On 02/07/2014 05:26 AM, Alexander Drozdov wrote:
>>> # HG changeset patch
>>> # User Alexander Drozdov <al.drozdov at 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 ;-)


More information about the Mercurial-devel mailing list