[PATCH 2 of 2 v2] commitextras: check the format of the arguments and no internal key is used

[Durham Goode]

On 7/13/17 6:08 PM, Matt Harbison wrote:
> On Thu, 13 Jul 2017 20:57:24 -0400, Pulkit Goyal <7895pulkit at gmail.com>
> wrote:
>
>> On Fri, Jul 14, 2017 at 6:22 AM, Matt Harbison <mharbison72 at gmail.com>
>> wrote:
>>> On Thu, 13 Jul 2017 16:44:35 -0400, Sean Farley <sean at farley.io> wrote:
>>>
>>>>
>>>> Pulkit Goyal <7895pulkit at gmail.com> writes:
>>>>
>>>>> # HG changeset patch
>>>>> # User Pulkit Goyal <7895pulkit at gmail.com>
>>>>> # Date 1499856010 -19800
>>>>> #      Wed Jul 12 16:10:10 2017 +0530
>>>>> # Node ID e8200f6a0783798ad61cd84f73c1721264966f02
>>>>> # Parent  3d742534fa70a38a2b540cfb0bf68610ecc63d0e
>>>>> # EXP-Topic fbext
>>>>> commitextras: check the format of the arguments and no internal key is
>>>>> used
>>>>>
>>>>> This patch adds check to make the arguments are passed as KEY=VALUE
>>>>> and
>>>>> no key
>>>>> which is used internally is passed.
>>>>>
>>>>> This patch also adds test for the extension.
>>>>
>>>>
>>>> I'm fine with this as an extension to add --extra to commit (without
>>>> needing debugcommit). Any others have a problem with it?
>>>
>>>
>>> The only thing I question is if the named key should be automatically
>>> prefixed with 'user:' or similar.  It seems like it tries to prevent
>>> naming
>>> internally used keys, but only two at that.  It's not clear to me why
>>> those
>>> two get special protection.
>>
>> Well that's because I know only just two of them yet. We can have
>> restriction on keys used, I guess Yuya has more thoughts on this.
>
> Unfortunately, they aren't in a centralized place.  Here are the ones in
> my hg repo:
>
> $ hg log -T '{extras % "{extra}\n"}' | cut -f 1 -d '=' | sort | uniq
> __touch-noise__
> amend_source
> branch
> histedit_source
> intermediate-source
> rebase_source
> source
> topic
> transplant_source
>
> There could be others ('convert_revision' comes to mind).
>
> But even if you were able to blacklist them all now, what happens when
> core needs to start using another one?  Blocking that in future releases
> seems like a gratuitous BC.  Not doing so means the behavior is
> inconsistent.  I don't feel too strongly, but it seems like a policy to
> protect internal keys or not should be decided up front.

For most of those, if the user did want to override them, there's not 
much harm in it. Branch and topic are probably the only important ones, 
since they have caches that need to be updated.

The problem with prefixing it is that now consumers of that information 
need to be aware of the prefix. For instance, one of our early use cases 
was for writing a converter between repositories.  We would put the 
original hash in the extras and other services could read that metadata. 
  If it was prefixed, now other services need to be aware of the fact 
that the metadata was created via --extras and prepend the appropriate 
prefix.

Since commitextras is an extension, and a fairly specific one at that, I 
say we go with a basic blacklist to cover the known conflicts, and leave 
it at that for now.