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

[Matt Harbison]

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.