Patchwork commitextras: move fb's extension which add extras to a commit to core

login
register
mail settings
Submitter Pulkit Goyal
Date July 5, 2017, 5:44 p.m.
Message ID <64c39cf4b61475a133f8.1499276653@workspace>
Download mbox | patch
Permalink /patch/22012/
State Superseded
Headers show

Comments

Pulkit Goyal - July 5, 2017, 5:44 p.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1499274297 -19800
#      Wed Jul 05 22:34:57 2017 +0530
# Node ID 64c39cf4b61475a133f88fa6dd247ca4f3d61436
# Parent  8e6f4939a69ae8949e134d97de6b766799bca8de
# EXP-Topic fbext
commitextras: move fb's extension which add extras to a commit to core

Facebook has this as extension which adds a flag to `hg commit` to add extra
values to a commit from command line. This patch moves that extension to core by
adding that flag and marking that as ADVANCED. Also this patch adds a function
to check if any key which is used internally can't be used.
Yuya Nishihara - July 6, 2017, 1:36 p.m.
On Wed, 05 Jul 2017 23:14:13 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1499274297 -19800
> #      Wed Jul 05 22:34:57 2017 +0530
> # Node ID 64c39cf4b61475a133f88fa6dd247ca4f3d61436
> # Parent  8e6f4939a69ae8949e134d97de6b766799bca8de
> # EXP-Topic fbext
> commitextras: move fb's extension which add extras to a commit to core
> 
> Facebook has this as extension which adds a flag to `hg commit` to add extra
> values to a commit from command line. This patch moves that extension to core by
> adding that flag and marking that as ADVANCED. Also this patch adds a function
> to check if any key which is used internally can't be used.

FYI, the last attempt was rejected.

https://www.mercurial-scm.org/pipermail/mercurial-devel/2014-March/056889.html

I don't know if ADVANCED flag can make the "high barrier" enough to discourage
an abuse of extras.

> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -175,6 +175,13 @@
>  def parsealiases(cmd):
>      return cmd.lstrip("^").split("|")
>  
> +def checkextras(extras):
> +    notallowed = set(['amend_source'])

This should include 'branch', at least.

> +    msg = _("key '%s' is used internally, can't be set manually")
> +    for key in extras:
> +        if key in notallowed:
> +            raise error.Abort(msg % key)

Perhaps we should have some constraints on key format, e.g. '',
'non&ascii*string'.

>  def setupwrapcolorwrite(ui):
>      # wrap ui.write so diff output can be labeled/colorized
>      def wrapwrite(orig, *args, **kw):
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -1420,6 +1420,8 @@
>      ('s', 'secret', None, _('use the secret phase for committing')),
>      ('e', 'edit', None, _('invoke editor on commit messages')),
>      ('i', 'interactive', None, _('use interactive mode')),
> +    ('', 'extra', [], _('set a changeset\'s extra values (ADVANCED)'),
> +        _('KEY=VALUE')),
>      ] + walkopts + commitopts + commitopts2 + subrepoopts,
>      _('[OPTION]... [FILE]...'),
>      inferrepo=True)
> @@ -1488,6 +1490,16 @@
>          release(lock, wlock)
>  
>  def _docommit(ui, repo, *pats, **opts):
> +    extrals = opts.get('extra')
> +    extra = {}
> +    # extrals can be none, so checking before iterating on it
> +    if extrals:
> +        for raws in extrals:
> +            k, v = raws.split('=', 1)

Need to check if '=' exists.

> +            extra[k] = v
> +    # check if any extra key is passed which is used internally
> +    cmdutil.checkextras(extra)
Sean Farley - July 6, 2017, 5:58 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> On Wed, 05 Jul 2017 23:14:13 +0530, Pulkit Goyal wrote:
>> # HG changeset patch
>> # User Pulkit Goyal <7895pulkit@gmail.com>
>> # Date 1499274297 -19800
>> #      Wed Jul 05 22:34:57 2017 +0530
>> # Node ID 64c39cf4b61475a133f88fa6dd247ca4f3d61436
>> # Parent  8e6f4939a69ae8949e134d97de6b766799bca8de
>> # EXP-Topic fbext
>> commitextras: move fb's extension which add extras to a commit to core
>> 
>> Facebook has this as extension which adds a flag to `hg commit` to add extra
>> values to a commit from command line. This patch moves that extension to core by
>> adding that flag and marking that as ADVANCED. Also this patch adds a function
>> to check if any key which is used internally can't be used.
>
> FYI, the last attempt was rejected.
>
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2014-March/056889.html

Ha, I was going to dig that message up as well.

> I don't know if ADVANCED flag can make the "high barrier" enough to discourage
> an abuse of extras.

I agree with this and with Matt's original criticism. Can this not be an
extension instead?
via Mercurial-devel - July 6, 2017, 9:24 p.m.
On Thu, Jul 6, 2017 at 10:58 AM, Sean Farley <sean@farley.io> wrote:
>
> Yuya Nishihara <yuya@tcha.org> writes:
>
>> On Wed, 05 Jul 2017 23:14:13 +0530, Pulkit Goyal wrote:
>>> # HG changeset patch
>>> # User Pulkit Goyal <7895pulkit@gmail.com>
>>> # Date 1499274297 -19800
>>> #      Wed Jul 05 22:34:57 2017 +0530
>>> # Node ID 64c39cf4b61475a133f88fa6dd247ca4f3d61436
>>> # Parent  8e6f4939a69ae8949e134d97de6b766799bca8de
>>> # EXP-Topic fbext
>>> commitextras: move fb's extension which add extras to a commit to core
>>>
>>> Facebook has this as extension which adds a flag to `hg commit` to add extra
>>> values to a commit from command line. This patch moves that extension to core by
>>> adding that flag and marking that as ADVANCED. Also this patch adds a function
>>> to check if any key which is used internally can't be used.
>>
>> FYI, the last attempt was rejected.
>>
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2014-March/056889.html
>
> Ha, I was going to dig that message up as well.
>
>> I don't know if ADVANCED flag can make the "high barrier" enough to discourage
>> an abuse of extras.
>
> I agree with this and with Matt's original criticism. Can this not be an
> extension instead?

How about make a debugcommit command that adds this flag and delegates
everything else to the regular commit command?

>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Matt Harbison - July 7, 2017, 3:01 a.m.
On Thu, 06 Jul 2017 09:36:56 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 05 Jul 2017 23:14:13 +0530, Pulkit Goyal wrote:
>> # HG changeset patch
>> # User Pulkit Goyal <7895pulkit@gmail.com>
>> # Date 1499274297 -19800
>> #      Wed Jul 05 22:34:57 2017 +0530
>> # Node ID 64c39cf4b61475a133f88fa6dd247ca4f3d61436
>> # Parent  8e6f4939a69ae8949e134d97de6b766799bca8de
>> # EXP-Topic fbext
>> commitextras: move fb's extension which add extras to a commit to core
>>
>> Facebook has this as extension which adds a flag to `hg commit` to add  
>> extra
>> values to a commit from command line. This patch moves that extension  
>> to core by
>> adding that flag and marking that as ADVANCED. Also this patch adds a  
>> function
>> to check if any key which is used internally can't be used.
>
> FYI, the last attempt was rejected.
>
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2014-March/056889.html
>
> I don't know if ADVANCED flag can make the "high barrier" enough to  
> discourage
> an abuse of extras.
>
>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>> --- a/mercurial/cmdutil.py
>> +++ b/mercurial/cmdutil.py
>> @@ -175,6 +175,13 @@
>>  def parsealiases(cmd):
>>      return cmd.lstrip("^").split("|")
>>
>> +def checkextras(extras):
>> +    notallowed = set(['amend_source'])
>
> This should include 'branch', at least.

Namespacing user supplied keys with a prefix was mentioned here, but I  
thought there was a bigger discussion about that (which I can't find).

   https://www.mercurial-scm.org/pipermail/mercurial-devel/2014-March/056774.html

We don't know now what future keys will be used in core, so checking a  
known set of items seems less effective than desired.
Yuya Nishihara - July 7, 2017, 12:33 p.m.
On Thu, 6 Jul 2017 14:24:08 -0700, Martin von Zweigbergk wrote:
> On Thu, Jul 6, 2017 at 10:58 AM, Sean Farley <sean@farley.io> wrote:
> > Yuya Nishihara <yuya@tcha.org> writes:
> >> On Wed, 05 Jul 2017 23:14:13 +0530, Pulkit Goyal wrote:
> >>> # HG changeset patch
> >>> # User Pulkit Goyal <7895pulkit@gmail.com>
> >>> # Date 1499274297 -19800
> >>> #      Wed Jul 05 22:34:57 2017 +0530
> >>> # Node ID 64c39cf4b61475a133f88fa6dd247ca4f3d61436
> >>> # Parent  8e6f4939a69ae8949e134d97de6b766799bca8de
> >>> # EXP-Topic fbext
> >>> commitextras: move fb's extension which add extras to a commit to core
> >>>
> >>> Facebook has this as extension which adds a flag to `hg commit` to add extra
> >>> values to a commit from command line. This patch moves that extension to core by
> >>> adding that flag and marking that as ADVANCED. Also this patch adds a function
> >>> to check if any key which is used internally can't be used.
> >>
> >> FYI, the last attempt was rejected.
> >>
> >> https://www.mercurial-scm.org/pipermail/mercurial-devel/2014-March/056889.html
> >
> > Ha, I was going to dig that message up as well.
> >
> >> I don't know if ADVANCED flag can make the "high barrier" enough to discourage
> >> an abuse of extras.
> >
> > I agree with this and with Matt's original criticism. Can this not be an
> > extension instead?
> 
> How about make a debugcommit command that adds this flag and delegates
> everything else to the regular commit command?

debugcommit seems fine.

FWIW, --extra value will need some encoding handling depending on its
type, e.g. convert labels to utf-8, but keep file path as bytes.
Sean Farley - July 7, 2017, 6:23 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> On Thu, 6 Jul 2017 14:24:08 -0700, Martin von Zweigbergk wrote:
>> On Thu, Jul 6, 2017 at 10:58 AM, Sean Farley <sean@farley.io> wrote:
>> > Yuya Nishihara <yuya@tcha.org> writes:
>> >> On Wed, 05 Jul 2017 23:14:13 +0530, Pulkit Goyal wrote:
>> >>> # HG changeset patch
>> >>> # User Pulkit Goyal <7895pulkit@gmail.com>
>> >>> # Date 1499274297 -19800
>> >>> #      Wed Jul 05 22:34:57 2017 +0530
>> >>> # Node ID 64c39cf4b61475a133f88fa6dd247ca4f3d61436
>> >>> # Parent  8e6f4939a69ae8949e134d97de6b766799bca8de
>> >>> # EXP-Topic fbext
>> >>> commitextras: move fb's extension which add extras to a commit to core
>> >>>
>> >>> Facebook has this as extension which adds a flag to `hg commit` to add extra
>> >>> values to a commit from command line. This patch moves that extension to core by
>> >>> adding that flag and marking that as ADVANCED. Also this patch adds a function
>> >>> to check if any key which is used internally can't be used.
>> >>
>> >> FYI, the last attempt was rejected.
>> >>
>> >> https://www.mercurial-scm.org/pipermail/mercurial-devel/2014-March/056889.html
>> >
>> > Ha, I was going to dig that message up as well.
>> >
>> >> I don't know if ADVANCED flag can make the "high barrier" enough to discourage
>> >> an abuse of extras.
>> >
>> > I agree with this and with Matt's original criticism. Can this not be an
>> > extension instead?
>> 
>> How about make a debugcommit command that adds this flag and delegates
>> everything else to the regular commit command?
>
> debugcommit seems fine.
>
> FWIW, --extra value will need some encoding handling depending on its
> type, e.g. convert labels to utf-8, but keep file path as bytes.

Yeah, seems fine to me too. But was there any reason we're avoiding the
path of new-thing-as-extension first, then into core second?
via Mercurial-devel - July 7, 2017, 6:25 p.m.
On Fri, Jul 7, 2017 at 11:23 AM, Sean Farley <sean@farley.io> wrote:
>
> Yuya Nishihara <yuya@tcha.org> writes:
>
>> On Thu, 6 Jul 2017 14:24:08 -0700, Martin von Zweigbergk wrote:
>>> On Thu, Jul 6, 2017 at 10:58 AM, Sean Farley <sean@farley.io> wrote:
>>> > Yuya Nishihara <yuya@tcha.org> writes:
>>> >> On Wed, 05 Jul 2017 23:14:13 +0530, Pulkit Goyal wrote:
>>> >>> # HG changeset patch
>>> >>> # User Pulkit Goyal <7895pulkit@gmail.com>
>>> >>> # Date 1499274297 -19800
>>> >>> #      Wed Jul 05 22:34:57 2017 +0530
>>> >>> # Node ID 64c39cf4b61475a133f88fa6dd247ca4f3d61436
>>> >>> # Parent  8e6f4939a69ae8949e134d97de6b766799bca8de
>>> >>> # EXP-Topic fbext
>>> >>> commitextras: move fb's extension which add extras to a commit to core
>>> >>>
>>> >>> Facebook has this as extension which adds a flag to `hg commit` to add extra
>>> >>> values to a commit from command line. This patch moves that extension to core by
>>> >>> adding that flag and marking that as ADVANCED. Also this patch adds a function
>>> >>> to check if any key which is used internally can't be used.
>>> >>
>>> >> FYI, the last attempt was rejected.
>>> >>
>>> >> https://www.mercurial-scm.org/pipermail/mercurial-devel/2014-March/056889.html
>>> >
>>> > Ha, I was going to dig that message up as well.
>>> >
>>> >> I don't know if ADVANCED flag can make the "high barrier" enough to discourage
>>> >> an abuse of extras.
>>> >
>>> > I agree with this and with Matt's original criticism. Can this not be an
>>> > extension instead?
>>>
>>> How about make a debugcommit command that adds this flag and delegates
>>> everything else to the regular commit command?
>>
>> debugcommit seems fine.
>>
>> FWIW, --extra value will need some encoding handling depending on its
>> type, e.g. convert labels to utf-8, but keep file path as bytes.
>
> Yeah, seems fine to me too. But was there any reason we're avoiding the
> path of new-thing-as-extension first, then into core second?

If we decide to put it in debugcommit, we provide no BC guarantee
(AFAIK), so we can remove it later if we see a reason. That buys us
the same flexibility as having it in an extensions, it seems.
Sean Farley - July 7, 2017, 6:33 p.m.
Kevin Bullock <kbullock+mercurial@ringworld.org> writes:

>> On Jul 7, 2017, at 13:25, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
>> 
>> On Fri, Jul 7, 2017 at 11:23 AM, Sean Farley <sean@farley.io> wrote:
>>> 
>>> Yuya Nishihara <yuya@tcha.org> writes:
>>>> 
>>>> debugcommit seems fine.
>>>> 
>>>> FWIW, --extra value will need some encoding handling depending on its
>>>> type, e.g. convert labels to utf-8, but keep file path as bytes.
>>> 
>>> Yeah, seems fine to me too. But was there any reason we're avoiding the
>>> path of new-thing-as-extension first, then into core second?
>> 
>> If we decide to put it in debugcommit, we provide no BC guarantee
>> (AFAIK), so we can remove it later if we see a reason. That buys us
>> the same flexibility as having it in an extensions, it seems.
>
> Hmm, I'm not sure we throw out BC guarantees for debug commands. Per <https://www.mercurial-scm.org/wiki/CompatibilityRules#Commands>, there is somewhat of a gradient to the strength of BC guarantees, but as I read it, debug commands aren't exempt.
>
> Also just as a reminder for those following along, we only suspend the BC guarantees for extensions that are marked EXPERIMENTAL.

Ah, yeah. Though, I might disagree a bit with debug* being BC that's
another discussion entirely.

For this case, it would seem that marking this as an EXPERIMENTAL
extension that adds the 'debugcommit' command would cover all these
points / issues?
via Mercurial-devel - July 7, 2017, 6:37 p.m.
On Fri, Jul 7, 2017 at 11:33 AM, Sean Farley <sean@farley.io> wrote:
>
> Kevin Bullock <kbullock+mercurial@ringworld.org> writes:
>
>>> On Jul 7, 2017, at 13:25, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
>>>
>>> On Fri, Jul 7, 2017 at 11:23 AM, Sean Farley <sean@farley.io> wrote:
>>>>
>>>> Yuya Nishihara <yuya@tcha.org> writes:
>>>>>
>>>>> debugcommit seems fine.
>>>>>
>>>>> FWIW, --extra value will need some encoding handling depending on its
>>>>> type, e.g. convert labels to utf-8, but keep file path as bytes.
>>>>
>>>> Yeah, seems fine to me too. But was there any reason we're avoiding the
>>>> path of new-thing-as-extension first, then into core second?
>>>
>>> If we decide to put it in debugcommit, we provide no BC guarantee
>>> (AFAIK), so we can remove it later if we see a reason. That buys us
>>> the same flexibility as having it in an extensions, it seems.
>>
>> Hmm, I'm not sure we throw out BC guarantees for debug commands. Per <https://www.mercurial-scm.org/wiki/CompatibilityRules#Commands>, there is somewhat of a gradient to the strength of BC guarantees, but as I read it, debug commands aren't exempt.
>>
>> Also just as a reminder for those following along, we only suspend the BC guarantees for extensions that are marked EXPERIMENTAL.
>
> Ah, yeah. Though, I might disagree a bit with debug* being BC that's
> another discussion entirely.

That's unclear to me too.

>
> For this case, it would seem that marking this as an EXPERIMENTAL
> extension that adds the 'debugcommit' command would cover all these
> points / issues?

I agree, that sounds like our safest bet.
Durham Goode - July 11, 2017, 6:16 p.m.
On 7/7/17 11:37 AM, Martin von Zweigbergk via Mercurial-devel wrote:
> On Fri, Jul 7, 2017 at 11:33 AM, Sean Farley <sean@farley.io> wrote:
>>
>> Kevin Bullock <kbullock+mercurial@ringworld.org> writes:
>>
>>>> On Jul 7, 2017, at 13:25, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
>>>>
>>>> On Fri, Jul 7, 2017 at 11:23 AM, Sean Farley <sean@farley.io> wrote:
>>>>>
>>>>> Yuya Nishihara <yuya@tcha.org> writes:
>>>>>>
>>>>>> debugcommit seems fine.
>>>>>>
>>>>>> FWIW, --extra value will need some encoding handling depending on its
>>>>>> type, e.g. convert labels to utf-8, but keep file path as bytes.
>>>>>
>>>>> Yeah, seems fine to me too. But was there any reason we're avoiding the
>>>>> path of new-thing-as-extension first, then into core second?
>>>>
>>>> If we decide to put it in debugcommit, we provide no BC guarantee
>>>> (AFAIK), so we can remove it later if we see a reason. That buys us
>>>> the same flexibility as having it in an extensions, it seems.
>>>
>>> Hmm, I'm not sure we throw out BC guarantees for debug commands. Per <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_wiki_CompatibilityRules-23Commands&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=_pMWbdg-fz5RQSQRthyoxFuAzNIkj6NupzvxLqCheMI&s=CcMcfcG2-x3ih73Z9U1OcgyMFSqwA3DJgcrik0n0u2U&e= >, there is somewhat of a gradient to the strength of BC guarantees, but as I read it, debug commands aren't exempt.
>>>
>>> Also just as a reminder for those following along, we only suspend the BC guarantees for extensions that are marked EXPERIMENTAL.
>>
>> Ah, yeah. Though, I might disagree a bit with debug* being BC that's
>> another discussion entirely.
>
> That's unclear to me too.
>
>>
>> For this case, it would seem that marking this as an EXPERIMENTAL
>> extension that adds the 'debugcommit' command would cover all these
>> points / issues?
>
> I agree, that sounds like our safest bet.

Having the option be on 'hg debugcommit' is kinda awkward for us.  Our 
main use of --extras is from services and automation. Having to teach 
the people writing these services to use debugcommit in one situation 
and normal commit in another will just cause confusion and bugs. If it's 
an extension anyways, users (or sys-admins) have to opt-in to the option 
anyway, so I think opt'ing in to 'hg commit --extras' once via config is 
better than having them opt-in to using 'hg debugcommit' for every 
invocation of commit that could be affected.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -175,6 +175,13 @@ 
 def parsealiases(cmd):
     return cmd.lstrip("^").split("|")
 
+def checkextras(extras):
+    notallowed = set(['amend_source'])
+    msg = _("key '%s' is used internally, can't be set manually")
+    for key in extras:
+        if key in notallowed:
+            raise error.Abort(msg % key)
+
 def setupwrapcolorwrite(ui):
     # wrap ui.write so diff output can be labeled/colorized
     def wrapwrite(orig, *args, **kw):
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1420,6 +1420,8 @@ 
     ('s', 'secret', None, _('use the secret phase for committing')),
     ('e', 'edit', None, _('invoke editor on commit messages')),
     ('i', 'interactive', None, _('use interactive mode')),
+    ('', 'extra', [], _('set a changeset\'s extra values (ADVANCED)'),
+        _('KEY=VALUE')),
     ] + walkopts + commitopts + commitopts2 + subrepoopts,
     _('[OPTION]... [FILE]...'),
     inferrepo=True)
@@ -1488,6 +1490,16 @@ 
         release(lock, wlock)
 
 def _docommit(ui, repo, *pats, **opts):
+    extrals = opts.get('extra')
+    extra = {}
+    # extrals can be none, so checking before iterating on it
+    if extrals:
+        for raws in extrals:
+            k, v = raws.split('=', 1)
+            extra[k] = v
+    # check if any extra key is passed which is used internally
+    cmdutil.checkextras(extra)
+
     if opts.get(r'interactive'):
         opts.pop(r'interactive')
         ret = cmdutil.dorecord(ui, repo, commit, None, False,
@@ -1509,7 +1521,6 @@ 
     branch = repo[None].branch()
     bheads = repo.branchheads(branch)
 
-    extra = {}
     if opts.get('close_branch'):
         extra['close'] = 1
 
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,22 @@ 
   $ hg tip --template '{date|isodate}\n' | grep '1970'
   [1]
 
+Using the advanced --extra flag
+
+  $ 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
diff --git a/tests/test-completion.t b/tests/test-completion.t
--- a/tests/test-completion.t
+++ b/tests/test-completion.t
@@ -219,7 +219,7 @@ 
   add: include, exclude, subrepos, dry-run
   annotate: rev, follow, no-follow, text, user, file, date, number, changeset, line-number, skip, ignore-all-space, ignore-space-change, ignore-blank-lines, include, exclude, template
   clone: noupdate, updaterev, rev, branch, pull, uncompressed, ssh, remotecmd, insecure
-  commit: addremove, close-branch, amend, secret, edit, interactive, include, exclude, message, logfile, date, user, subrepos
+  commit: addremove, close-branch, amend, secret, edit, interactive, extra, include, exclude, message, logfile, date, user, subrepos
   diff: rev, change, text, git, binary, nodates, noprefix, show-function, reverse, ignore-all-space, ignore-space-change, ignore-blank-lines, unified, stat, root, include, exclude, subrepos
   export: output, switch-parent, rev, text, git, binary, nodates
   forget: include, exclude