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

login
register
mail settings
Submitter Pulkit Goyal
Date July 12, 2017, 11:43 a.m.
Message ID <e8200f6a0783798ad61c.1499859815@workspace>
Download mbox | patch
Permalink /patch/22252/
State Superseded
Headers show

Comments

Pulkit Goyal - July 12, 2017, 11:43 a.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@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.
Sean Farley - July 13, 2017, 8:44 p.m.
Pulkit Goyal <7895pulkit@gmail.com> writes:

> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@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?
Matt Harbison - July 14, 2017, 12:52 a.m.
On Thu, 13 Jul 2017 16:44:35 -0400, Sean Farley <sean@farley.io> wrote:

>
> Pulkit Goyal <7895pulkit@gmail.com> writes:
>
>> # HG changeset patch
>> # User Pulkit Goyal <7895pulkit@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.
Pulkit Goyal - July 14, 2017, 12:57 a.m.
On Fri, Jul 14, 2017 at 6:22 AM, Matt Harbison <mharbison72@gmail.com> wrote:
> On Thu, 13 Jul 2017 16:44:35 -0400, Sean Farley <sean@farley.io> wrote:
>
>>
>> Pulkit Goyal <7895pulkit@gmail.com> writes:
>>
>>> # HG changeset patch
>>> # User Pulkit Goyal <7895pulkit@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.
Matt Harbison - July 14, 2017, 1:08 a.m.
On Thu, 13 Jul 2017 20:57:24 -0400, Pulkit Goyal <7895pulkit@gmail.com>  
wrote:

> On Fri, Jul 14, 2017 at 6:22 AM, Matt Harbison <mharbison72@gmail.com>  
> wrote:
>> On Thu, 13 Jul 2017 16:44:35 -0400, Sean Farley <sean@farley.io> wrote:
>>
>>>
>>> Pulkit Goyal <7895pulkit@gmail.com> writes:
>>>
>>>> # HG changeset patch
>>>> # User Pulkit Goyal <7895pulkit@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.
Durham Goode - July 14, 2017, 3:58 p.m.
On 7/13/17 6:08 PM, Matt Harbison wrote:
> On Thu, 13 Jul 2017 20:57:24 -0400, Pulkit Goyal <7895pulkit@gmail.com>
> wrote:
>
>> On Fri, Jul 14, 2017 at 6:22 AM, Matt Harbison <mharbison72@gmail.com>
>> wrote:
>>> On Thu, 13 Jul 2017 16:44:35 -0400, Sean Farley <sean@farley.io> wrote:
>>>
>>>>
>>>> Pulkit Goyal <7895pulkit@gmail.com> writes:
>>>>
>>>>> # HG changeset patch
>>>>> # User Pulkit Goyal <7895pulkit@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.

Patch

diff --git a/hgext/commitextras.py b/hgext/commitextras.py
--- a/hgext/commitextras.py
+++ b/hgext/commitextras.py
@@ -12,6 +12,7 @@ 
 from mercurial.i18n import _
 from mercurial import (
     commands,
+    error,
     extensions,
     registrar,
 )
@@ -20,6 +21,8 @@ 
 command = registrar.command(cmdtable)
 testedwith = 'ships-with-hg-core'
 
+usedinternally = set(['amend_source', 'branch'])
+
 def extsetup(ui):
     entry = extensions.wrapcommand(commands.table, 'commit', _commit)
     options = entry[1]
@@ -33,7 +36,15 @@ 
             extras = opts.get('extra')
             if extras:
                 for raw in extras:
+                    if '=' not in raw:
+                        msg = _("unable to parse '%s', should follow "
+                                "KEY=VALUE format")
+                        raise error.Abort(msg % raw)
                     k, v = raw.split('=', 1)
+                    if k in usedinternally:
+                        msg = _("key '%s' is used internally, can't be set "
+                                "manually")
+                        raise error.Abort(msg % k)
                     inneropts['extra'][k] = v
             return origcommit(*innerpats, **inneropts)
 
diff --git a/tests/test-commit.t b/tests/test-commit.t
--- a/tests/test-commit.t
+++ b/tests/test-commit.t
@@ -124,6 +124,23 @@ 
   $ hg tip --template '{date|isodate}\n' | grep '1970'
   [1]
 
+Using the advanced --extra flag
+
+  $ echo "[extensions]\ncommitextras=" >> $HGRCPATH
+  $ hg status
+  ? baz
+  ? quux
+  $ hg add baz
+  $ hg commit -m "adding extras" --extra source=foo --extra oldhash=bar
+  $ hg log -r . -T '{extras % "{extra}\n"}'
+  branch=default
+  oldhash=bar
+  source=foo
+  $ hg add quux
+  $ hg commit -m "adding internal used extras" --extra amend_source=hash
+  abort: key 'amend_source' is used internally, can't be set manually
+  [255]
+
 Make sure we do not obscure unknown requires file entries (issue2649)
 
   $ echo foo >> foo