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

[Jordi Gutiérrez Hermoso]

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.

> 
> 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?

> 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), ...)