[PATCH] commit: add --allow-empty flag

Durham Goode durham at fb.com
Fri May 8 11:04:20 CDT 2015



On 5/7/15, 4:51 PM, "Jordi GutiƩrrez Hermoso" <jordigh at octave.org> wrote:

>On Thu, 2015-05-07 at 15:10 -0700, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham at fb.com>
>> # Date 1431034538 25200
>> #      Thu May 07 14:35:38 2015 -0700
>> # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
>> # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
>> commit: add --allow-empty flag
>> 
>> This adds a flag that enables a user to make empty commits. This is
>>useful in a
>> number of cases.
>> 
>> For instance, automation that creates release branches via
>> bookmarks may want to make empty commits to that release bookmark so
>>that it
>> can't be fast forwarded
>
>You mean advanced?
>
>> and so it can record information about the release bookmark's
>> creation. This is already possible with named branches, so making it
>> possible for bookmarks makes sense.
>> 
>> Another case we've wanted it is for mirroring repositories into
>>Mercurial. We
>> have automation that syncs commits into hg by running things from the
>>command
>> line. The ability to produce empty commits is useful for syncing
>>unusual commits
>> from other VCS's.
>> 
>> In general, allowing the user to create the DAG as they see fit seems
>>useful,
>> and when I mentioned this in IRC more than one person piped up and said
>>they
>> were already hacking around this limitation by using mq, import, and
>> commit-dummy-change-then-amend-the-content-away style solutions.
>
>I want to argue against this change because of the extra UI clutter,
>but I can't really justify it. It's just one more option and there are
>several situations in which yes, we really do want an empty
>placeholder commit. So against my natural impulses, I'm in favour of
>adding an option.

If we want to balance out the UI clutter, let's hide --secret.

>
>> 
>> diff --git a/hgext/largefiles/reposetup.py
>>b/hgext/largefiles/reposetup.py
>> --- a/hgext/largefiles/reposetup.py
>> +++ b/hgext/largefiles/reposetup.py
>> @@ -260,7 +260,7 @@ def reposetup(ui, repo):
>>          # contents updated to reflect the hash of their largefile.
>>          # Do that here.
>>          def commit(self, text="", user=None, date=None, match=None,
>> -                force=False, editor=False, extra={}):
>> +                force=False, editor=False, extra={}, **kwargs):
>>              orig = super(lfilesrepo, self).commit
>>  
>>              wlock = self.wlock()
>> @@ -268,7 +268,8 @@ def reposetup(ui, repo):
>>                  lfcommithook = self._lfcommithooks[-1]
>>                  match = lfcommithook(self, match)
>>                  result = orig(text=text, user=user, date=date,
>>match=match,
>> -                                force=force, editor=editor,
>>extra=extra)
>> +                                force=force, editor=editor,
>>extra=extra,
>> +                                **kwargs)
>>                  return result
>>              finally:
>>                  wlock.release()
>> diff --git a/hgext/mq.py b/hgext/mq.py
>> --- a/hgext/mq.py
>> +++ b/hgext/mq.py
>> @@ -3399,13 +3399,13 @@ def reposetup(ui, repo):
>>                      raise util.Abort(errmsg)
>>  
>>          def commit(self, text="", user=None, date=None, match=None,
>> -                   force=False, editor=False, extra={}):
>> +                   force=False, editor=False, extra={}, **kwargs):
>>              self.abortifwdirpatched(
>>                  _('cannot commit over an applied mq patch'),
>>                  force)
>>  
>>              return super(mqrepo, self).commit(text, user, date, match,
>>force,
>> -                                              editor, extra)
>> +                                              editor, extra, **kwargs)
>>  
>>          def checkpush(self, pushop):
>>              if self.mq.applied and self.mq.checkapplied and not
>>pushop.force:
>
>I don't understand. Is there a reason why all of these must be
>**kwargs instead of an explicit extra allowempty option?

**kwargs makes these extensions more robust to future api changes.  It's
kind of dumb that I have to go back and manually add each new arg to every
override of localrepo.commit when all the implementations are doing is
passing the value on through.

>
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -1418,6 +1418,7 @@ def clone(ui, source, dest=None, **opts)
>>      ('s', 'secret', None, _('use the secret phase for committing')),
>>      ('e', 'edit', None, _('invoke editor on commit messages')),
>>      ('i', 'interactive', None, _('use interactive mode')),
>> +    ('', 'allow-empty', None, _('allow commiting without pending
>>changes')),
>>      ] + walkopts + commitopts + commitopts2 + subrepoopts,
>>      _('[OPTION]... [FILE]...'),
>>      inferrepo=True)
>> @@ -1537,7 +1538,8 @@ def commit(ui, repo, *pats, **opts):
>>                  return repo.commit(message, opts.get('user'),
>>opts.get('date'),
>>                                     match,
>>                                     editor=editor,
>> -                                   extra=extra)
>> +                                   extra=extra,
>> +                                   allowempty=opts.get('allow_empty'))
>>              finally:
>>                  ui.restoreconfig(backup)
>>                  repo.baseui.restoreconfig(basebackup)
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -1356,7 +1356,7 @@ class localrepository(object):
>>  
>>      @unfilteredmethod
>>      def commit(self, text="", user=None, date=None, match=None,
>>force=False,
>> -               editor=False, extra={}):
>> +               editor=False, extra={}, allowempty=False):
>>          """Add a new revision to current repository.
>>  
>>          Revision information is gathered from the working directory,
>> @@ -1465,8 +1465,8 @@ class localrepository(object):
>>              cctx = context.workingcommitctx(self, status,
>>                                              text, user, date, extra)
>>  
>> -            if (not force and not extra.get("close") and not merge
>> -                and not cctx.files()
>> +            if (not force and not extra.get("close") and not merge and
>>not
>> +                allowempty and not cctx.files()
>>                  and wctx.branch() == wctx.p1().branch()):
>>                  return None
>
>Wow, I must say, this check for when empty commits are allowed is
>quite difficult to read now. Could you reformat so that "and not
>allowempty" is on its own line? If I didn't know to look for it within
>this cset, I wouldn't have found it. Perhaps add a comment about what
>this complicated condition is?
>
>Or perhaps the whole cset could be simplified by simply calling
>commit(..., force=(force or allowempty), ...)

You're right.  I can pretty this up in a V2



More information about the Mercurial-devel mailing list