Patchwork commit: add --allow-empty flag

login
register
mail settings
Submitter Durham Goode
Date May 7, 2015, 10:10 p.m.
Message ID <9f90e53f8e810db2d6b0.1431036614@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/8958/
State Rejected
Headers show

Comments

Durham Goode - May 7, 2015, 10:10 p.m.
# HG changeset patch
# User Durham Goode <durham@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 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.
Jordi Gutiérrez Hermoso - May 7, 2015, 11:51 p.m.
On Thu, 2015-05-07 at 15:10 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@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), ...)
Gilles Moris - May 8, 2015, 6:24 a.m.
Le 08/05/2015 00:10, Durham Goode a écrit :
> # HG changeset patch
> # User Durham Goode<durham@fb.com>
> # Date 1431034538 25200
> #      Thu May 07 14:35:38 2015 -0700
> # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
> # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
> commit: add --allow-empty flag
>
bikeshedding, but from a user perspective --empty seems sufficient. 
allow-empty seems more a dev view.

Regards.
Gilles.
Augie Fackler - May 8, 2015, 2:34 p.m.
On Thu, May 07, 2015 at 07:51:21PM -0400, Jordi Gutiérrez Hermoso wrote:
> On Thu, 2015-05-07 at 15:10 -0700, Durham Goode wrote:
> > # HG changeset patch
> > # User Durham Goode <durham@fb.com>
> > # Date 1431034538 25200
> > #      Thu May 07 14:35:38 2015 -0700
> > # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
> > # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
> > commit: add --allow-empty flag

[snip]

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

I feel the same way. I guess I'm +0 on this, especially since some
folks are (ab)using mq to make this kind of commit anyway.

We should probably spend a little time on rebase to make sure that it
doesn't elide empty changes when the source change is also empty.
Durham Goode - May 8, 2015, 4:04 p.m.
On 5/7/15, 4:51 PM, "Jordi Gutiérrez Hermoso" <jordigh@octave.org> wrote:

>On Thu, 2015-05-07 at 15:10 -0700, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@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
Durham Goode - May 8, 2015, 4:17 p.m.
On 5/7/15 11:24 PM, Gilles Moris wrote:
> Le 08/05/2015 00:10, Durham Goode a écrit :
>> # HG changeset patch
>> # User Durham Goode<durham@fb.com>
>> # Date 1431034538 25200
>> #      Thu May 07 14:35:38 2015 -0700
>> # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
>> # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
>> commit: add --allow-empty flag
>>
> bikeshedding, but from a user perspective --empty seems sufficient. 
> allow-empty seems more a dev view.
I think --empty implies that it will make an empty commit, which isn't 
always the case.
Pierre-Yves David - May 8, 2015, 5:41 p.m.
On 05/07/2015 03:10 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1431034538 25200
> #      Thu May 07 14:35:38 2015 -0700
> # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
> # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
> commit: add --allow-empty flag

I'm -1 on adding the new flag and +1 on the ability in core.

Given that apparently multiple users exists for that and that conversion 
from other VCS requires it, it makes sense to make it somewhat available.

However it is still far too much a corner case to be a first class 
citizen. Moreover, the most probable usage of that are automation who do 
not really care about the extra hurdle of going through something more 
complicated.
Commit is an important user facing command and its option tend to 
trickle down to a lot of other commands. In my opinion. This feature is 
not generic/useful for normal user enough to justify a dedicated flag.

I would go for a config option probably in the [ui] section.

As Augie mentioned, we also have to make sure rebase/graft/histedit/etc 
preserve them.
Jordi Gutiérrez Hermoso - May 8, 2015, 6:07 p.m.
On Fri, 2015-05-08 at 16:04 +0000, Durham Goode wrote:

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

> >On Thu, 2015-05-07 at 15:10 -0700, Durham Goode wrote:

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

No. This is the only way secret commits can be created without
--force. I think it should stay visible.

Just one more option isn't a lot of clutter, though. But perhaps
*this* --allow-empty option should be hidden? Do we have precedent for
hiding some advanced options under --verbose?

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

Fine, but then this is an unrelated change, right? Split the patch.

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

Or just expose the force option as a --force flag? I don't think
anything else can be forced other than empty commits, can it?

No, wait, yes it does. The force option also has consequences for
subrepos or for committing files that weren't specified in the CLI. I
guess it really needs a whole new option.
Ryan McElroy - May 8, 2015, 6:45 p.m.
On 5/8/2015 10:41 AM, Pierre-Yves David wrote:
>
>
> On 05/07/2015 03:10 PM, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1431034538 25200
>> #      Thu May 07 14:35:38 2015 -0700
>> # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
>> # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
>> commit: add --allow-empty flag
>
> I'm -1 on adding the new flag and +1 on the ability in core.
>
> Given that apparently multiple users exists for that and that 
> conversion from other VCS requires it, it makes sense to make it 
> somewhat available.
>
> However it is still far too much a corner case to be a first class 
> citizen. Moreover, the most probable usage of that are automation who 
> do not really care about the extra hurdle of going through something 
> more complicated.
> Commit is an important user facing command and its option tend to 
> trickle down to a lot of other commands. In my opinion. This feature 
> is not generic/useful for normal user enough to justify a dedicated flag.
>
> I would go for a config option probably in the [ui] section.
>
> As Augie mentioned, we also have to make sure 
> rebase/graft/histedit/etc preserve them.
>
>
I don't like the idea of having it as a config option, and [ui] doesn't 
seem like that right place for it anyway. Reasons against a config 
option: as a user, this is something that you want to enable on a 
per-command basis. Having it as a ui option implies stickiness is 
desired, but that is generally not the case here (in my experience using 
this functionality in git -- which I used not-as-rarely-as-you-would-think).

The preservation of empty commits after rebases seems like an 
interesting concern, and it's absolutely unclear to me what the right 
choice there is. What are the arguments towards changing the current 
behavior? I think it would be more confusing to have some commits that 
stick around when empty after rebase and others that don't.

I guess this is all a long way of saying that I don't think we should 
block on rebase/histedit supporting this because it's non-obvious that 
they should. And I think the feature should be a flag, perhaps hidden 
from default help (I don't care about that so much right now).
Pierre-Yves David - May 8, 2015, 7:30 p.m.
On 05/08/2015 11:45 AM, Ryan McElroy wrote:
> On 5/8/2015 10:41 AM, Pierre-Yves David wrote:
>>
>>
>> On 05/07/2015 03:10 PM, Durham Goode wrote:
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1431034538 25200
>>> #      Thu May 07 14:35:38 2015 -0700
>>> # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
>>> # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
>>> commit: add --allow-empty flag
>>
>> I'm -1 on adding the new flag and +1 on the ability in core.
>>
>> Given that apparently multiple users exists for that and that
>> conversion from other VCS requires it, it makes sense to make it
>> somewhat available.
>>
>> However it is still far too much a corner case to be a first class
>> citizen. Moreover, the most probable usage of that are automation who
>> do not really care about the extra hurdle of going through something
>> more complicated.
>> Commit is an important user facing command and its option tend to
>> trickle down to a lot of other commands. In my opinion. This feature
>> is not generic/useful for normal user enough to justify a dedicated flag.
>>
>> I would go for a config option probably in the [ui] section.
>>
>> As Augie mentioned, we also have to make sure
>> rebase/graft/histedit/etc preserve them.
>>
>>
> I don't like the idea of having it as a config option, and [ui] doesn't
> seem like that right place for it anyway. Reasons against a config
> option: as a user, this is something that you want to enable on a
> per-command basis. Having it as a ui option implies stickiness is
> desired, but that is generally not the case here (in my experience using
> this functionality in git -- which I used
> not-as-rarely-as-you-would-think).

Okay, this is new informations. The only creators of such changesets in 
usecase have heard about so far are automated. But you seems to implies 
normal user may want to do so. What is your human usecase for it and why 
is it so frequent ?

> The preservation of empty commits after rebases seems like an
> interesting concern, and it's absolutely unclear to me what the right
> choice there is. What are the arguments towards changing the current
> behavior? I think it would be more confusing to have some commits that
> stick around when empty after rebase and others that don't.

There is a clear way to go here. Rebase drop changesets -emptied- by the 
merge. IF there changesets was empty before the rebase, it must be 
preserved.

> I guess this is all a long way of saying that I don't think we should
> block on rebase/histedit supporting this because it's non-obvious that
> they should. And I think the feature should be a flag, perhaps hidden
> from default help (I don't care about that so much right now).

If we easily allow empty changeset but silently drop them whenever 
history is rewritten I expect angry user mob on your door step by the 
end of the month.
Ryan McElroy - May 9, 2015, 12:54 a.m.
On 5/8/2015 12:30 PM, Pierre-Yves David wrote:
>
>
> On 05/08/2015 11:45 AM, Ryan McElroy wrote:
>> On 5/8/2015 10:41 AM, Pierre-Yves David wrote:
>>>
>>>
>>> On 05/07/2015 03:10 PM, Durham Goode wrote:
>>>> # HG changeset patch
>>>> # User Durham Goode <durham@fb.com>
>>>> # Date 1431034538 25200
>>>> #      Thu May 07 14:35:38 2015 -0700
>>>> # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
>>>> # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
>>>> commit: add --allow-empty flag
>>>
>>> I'm -1 on adding the new flag and +1 on the ability in core.
>>>
>>> Given that apparently multiple users exists for that and that
>>> conversion from other VCS requires it, it makes sense to make it
>>> somewhat available.
>>>
>>> However it is still far too much a corner case to be a first class
>>> citizen. Moreover, the most probable usage of that are automation who
>>> do not really care about the extra hurdle of going through something
>>> more complicated.
>>> Commit is an important user facing command and its option tend to
>>> trickle down to a lot of other commands. In my opinion. This feature
>>> is not generic/useful for normal user enough to justify a dedicated 
>>> flag.
>>>
>>> I would go for a config option probably in the [ui] section.
>>>
>>> As Augie mentioned, we also have to make sure
>>> rebase/graft/histedit/etc preserve them.
>>>
>>>
>> I don't like the idea of having it as a config option, and [ui] doesn't
>> seem like that right place for it anyway. Reasons against a config
>> option: as a user, this is something that you want to enable on a
>> per-command basis. Having it as a ui option implies stickiness is
>> desired, but that is generally not the case here (in my experience using
>> this functionality in git -- which I used
>> not-as-rarely-as-you-would-think).
>
> Okay, this is new informations. The only creators of such changesets 
> in usecase have heard about so far are automated. But you seems to 
> implies normal user may want to do so. What is your human usecase for 
> it and why is it so frequent ?

Now that I think through it, the most common time I would do this is at 
the beginning of the new git repo, since git doesn't have an implicit 
null commit like hg does. So that argument is actually moot, and indeed, 
it might be okay just having it for automation in the regular sense. But 
if other people have worked around this, it would be interesting to see 
why they wanted it.

>
>> The preservation of empty commits after rebases seems like an
>> interesting concern, and it's absolutely unclear to me what the right
>> choice there is. What are the arguments towards changing the current
>> behavior? I think it would be more confusing to have some commits that
>> stick around when empty after rebase and others that don't.
>
> There is a clear way to go here. Rebase drop changesets -emptied- by 
> the merge. IF there changesets was empty before the rebase, it must be 
> preserved.

That makes sense, thanks for clarifying.

>
>> I guess this is all a long way of saying that I don't think we should
>> block on rebase/histedit supporting this because it's non-obvious that
>> they should. And I think the feature should be a flag, perhaps hidden
>> from default help (I don't care about that so much right now).
>
> If we easily allow empty changeset but silently drop them whenever 
> history is rewritten I expect angry user mob on your door step by the 
> end of the month.
>
You have convinced me. Thanks for saving me from the mobs.
Gregory Szorc - May 9, 2015, 4:50 p.m.
On Fri, May 8, 2015 at 12:30 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 05/08/2015 11:45 AM, Ryan McElroy wrote:
>
>> On 5/8/2015 10:41 AM, Pierre-Yves David wrote:
>>
>>>
>>>
>>> On 05/07/2015 03:10 PM, Durham Goode wrote:
>>>
>>>> # HG changeset patch
>>>> # User Durham Goode <durham@fb.com>
>>>> # Date 1431034538 25200
>>>> #      Thu May 07 14:35:38 2015 -0700
>>>> # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
>>>> # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
>>>> commit: add --allow-empty flag
>>>>
>>>
>>> I'm -1 on adding the new flag and +1 on the ability in core.
>>>
>>> Given that apparently multiple users exists for that and that
>>> conversion from other VCS requires it, it makes sense to make it
>>> somewhat available.
>>>
>>> However it is still far too much a corner case to be a first class
>>> citizen. Moreover, the most probable usage of that are automation who
>>> do not really care about the extra hurdle of going through something
>>> more complicated.
>>> Commit is an important user facing command and its option tend to
>>> trickle down to a lot of other commands. In my opinion. This feature
>>> is not generic/useful for normal user enough to justify a dedicated flag.
>>>
>>> I would go for a config option probably in the [ui] section.
>>>
>>> As Augie mentioned, we also have to make sure
>>> rebase/graft/histedit/etc preserve them.
>>>
>>>
>>>  I don't like the idea of having it as a config option, and [ui] doesn't
>> seem like that right place for it anyway. Reasons against a config
>> option: as a user, this is something that you want to enable on a
>> per-command basis. Having it as a ui option implies stickiness is
>> desired, but that is generally not the case here (in my experience using
>> this functionality in git -- which I used
>> not-as-rarely-as-you-would-think).
>>
>
> Okay, this is new informations. The only creators of such changesets in
> usecase have heard about so far are automated. But you seems to implies
> normal user may want to do so. What is your human usecase for it and why is
> it so frequent ?


At Mozilla, pushes to our "try" server contain a special "trychooser"
commit that specifies what jobs to run. This is an empty commit: all
metadata is conveyed via the commit message.

While we have some tools for managing this commit automatically, many users
rely on MQ for this role. As it stands, MQ is the best available mechanism
in the Mercurial distribution for doing this. I'd love to see one less
legitimate use of MQ.

For the record, if we were starting from scratch, we probably wouldn't use
empty trychooser commits. But it's been part of our workflow for years and
it isn't changing any time soon.
Durham Goode - May 11, 2015, 7:44 p.m.
On 5/8/15 12:30 PM, Pierre-Yves David wrote:
>
>
> On 05/08/2015 11:45 AM, Ryan McElroy wrote:
>> On 5/8/2015 10:41 AM, Pierre-Yves David wrote:
>>>
>>>
>>> On 05/07/2015 03:10 PM, Durham Goode wrote:
>>>> # HG changeset patch
>>>> # User Durham Goode <durham@fb.com>
>>>> # Date 1431034538 25200
>>>> #      Thu May 07 14:35:38 2015 -0700
>>>> # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
>>>> # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
>>>> commit: add --allow-empty flag
>>>
>>> I'm -1 on adding the new flag and +1 on the ability in core.
>>>
>>> Given that apparently multiple users exists for that and that
>>> conversion from other VCS requires it, it makes sense to make it
>>> somewhat available.
>>>
>>> However it is still far too much a corner case to be a first class
>>> citizen. Moreover, the most probable usage of that are automation who
>>> do not really care about the extra hurdle of going through something
>>> more complicated.
>>> Commit is an important user facing command and its option tend to
>>> trickle down to a lot of other commands. In my opinion. This feature
>>> is not generic/useful for normal user enough to justify a dedicated 
>>> flag.
>>>
>>> I would go for a config option probably in the [ui] section.
>>>
>>> As Augie mentioned, we also have to make sure
>>> rebase/graft/histedit/etc preserve them.
>>>
>>>
>> I don't like the idea of having it as a config option, and [ui] doesn't
>> seem like that right place for it anyway. Reasons against a config
>> option: as a user, this is something that you want to enable on a
>> per-command basis. Having it as a ui option implies stickiness is
>> desired, but that is generally not the case here (in my experience using
>> this functionality in git -- which I used
>> not-as-rarely-as-you-would-think).
>
> Okay, this is new informations. The only creators of such changesets 
> in usecase have heard about so far are automated. But you seems to 
> implies normal user may want to do so. What is your human usecase for 
> it and why is it so frequent ?
If the problem is UI clutter, then we should solve that problem (put it 
behind -v).  If the feature is hidden down in some config option, we 
might as well not introduce it to core since it will never be discovered 
by anyone except people who see this thread.
>
>> The preservation of empty commits after rebases seems like an
>> interesting concern, and it's absolutely unclear to me what the right
>> choice there is. What are the arguments towards changing the current
>> behavior? I think it would be more confusing to have some commits that
>> stick around when empty after rebase and others that don't.
>
> There is a clear way to go here. Rebase drop changesets -emptied- by 
> the merge. IF there changesets was empty before the rebase, it must be 
> preserved.
If everyone feels rebase and histedit support are a must before this 
feature can be accepted, I'll probably just drop the feature and do it 
as an internal extension.  The additional logic to make rebase and 
histedit work will likely be more complicated than the actual feature is 
and will likely cause more bugs than adding the feature solves.

I don't think rebase and histedit support are required.  If a user 
opts-in to making an empty commit, I think they can expect a few 
oddities with future commands.
>
>> I guess this is all a long way of saying that I don't think we should
>> block on rebase/histedit supporting this because it's non-obvious that
>> they should. And I think the feature should be a flag, perhaps hidden
>> from default help (I don't care about that so much right now).
>
> If we easily allow empty changeset but silently drop them whenever 
> history is rewritten I expect angry user mob on your door step by the 
> end of the month.
I think the mob would be smaller than the one clambering for 
--allow-empty (that is to say, negligible).
Durham Goode - May 11, 2015, 7:52 p.m.
On 5/11/15, 12:44 PM, "Durham Goode" <durham@fb.com> wrote:

>
>
>On 5/8/15 12:30 PM, Pierre-Yves David wrote:
>>
>>
>> On 05/08/2015 11:45 AM, Ryan McElroy wrote:
>>> On 5/8/2015 10:41 AM, Pierre-Yves David wrote:
>>>>
>>>>
>>>> On 05/07/2015 03:10 PM, Durham Goode wrote:
>>>>> # HG changeset patch
>>>>> # User Durham Goode <durham@fb.com>
>>>>> # Date 1431034538 25200
>>>>> #      Thu May 07 14:35:38 2015 -0700
>>>>> # Node ID 9f90e53f8e810db2d6b0e4cf66433166c4d42934
>>>>> # Parent  c5d4f9cc8da7bb2068457e96e4f74ff694514ced
>>>>> commit: add --allow-empty flag
>>>>
>>>> I'm -1 on adding the new flag and +1 on the ability in core.
>>>>
>>>> Given that apparently multiple users exists for that and that
>>>> conversion from other VCS requires it, it makes sense to make it
>>>> somewhat available.
>>>>
>>>> However it is still far too much a corner case to be a first class
>>>> citizen. Moreover, the most probable usage of that are automation who
>>>> do not really care about the extra hurdle of going through something
>>>> more complicated.
>>>> Commit is an important user facing command and its option tend to
>>>> trickle down to a lot of other commands. In my opinion. This feature
>>>> is not generic/useful for normal user enough to justify a dedicated
>>>> flag.
>>>>
>>>> I would go for a config option probably in the [ui] section.
>>>>
>>>> As Augie mentioned, we also have to make sure
>>>> rebase/graft/histedit/etc preserve them.
>>>>
>>>>
>>> I don't like the idea of having it as a config option, and [ui] doesn't
>>> seem like that right place for it anyway. Reasons against a config
>>> option: as a user, this is something that you want to enable on a
>>> per-command basis. Having it as a ui option implies stickiness is
>>> desired, but that is generally not the case here (in my experience
>>>using
>>> this functionality in git -- which I used
>>> not-as-rarely-as-you-would-think).
>>
>> Okay, this is new informations. The only creators of such changesets
>> in usecase have heard about so far are automated. But you seems to
>> implies normal user may want to do so. What is your human usecase for
>> it and why is it so frequent ?
>If the problem is UI clutter, then we should solve that problem (put it
>behind -v).  If the feature is hidden down in some config option, we
>might as well not introduce it to core since it will never be discovered
>by anyone except people who see this thread.

Actually, I take that back.  If it's a config option, then at least it
will be easy to make an extension to exposes it as a command line option.
Augie Fackler - May 11, 2015, 8:14 p.m.
> On May 11, 2015, at 3:44 PM, Durham Goode <durham@fb.com> wrote:
> 
>> There is a clear way to go here. Rebase drop changesets -emptied- by the merge. IF there changesets was empty before the rebase, it must be preserved.
> If everyone feels rebase and histedit support are a must before this feature can be accepted, I'll probably just drop the feature and do it as an internal extension.  The additional logic to make rebase and histedit work will likely be more complicated than the actual feature is and will likely cause more bugs than adding the feature solves.
> 
> I don't think rebase and histedit support are required.  If a user opts-in to making an empty commit, I think they can expect a few oddities with future commands.

Spoken like an expert in the field of VCS tooling. If you go this route, I expect you'll get an angry or confused bug report inside of two months.

Please don't make users understand their tools all the way to the bottom of the stack. That's what I fear you're doing if you don't make histedit/rebase work sanely with empty commits.
Mike Hommey - May 11, 2015, 11:14 p.m.
On Mon, May 11, 2015 at 07:52:38PM +0000, Durham Goode wrote:
> Actually, I take that back.  If it's a config option, then at least it
> will be easy to make an extension to exposes it as a command line option.

If it's a config option, anyone can do hg --config foo=bar command
without an extension.

Mike

Patch

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:
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
 
diff --git a/tests/test-commit.t b/tests/test-commit.t
--- a/tests/test-commit.t
+++ b/tests/test-commit.t
@@ -544,6 +544,18 @@  commit copy
        0         0       6  .....       0 26d3ca0dfd18 000000000000 000000000000 (re)
        1         6       7  .....       1 d267bddd54f7 26d3ca0dfd18 000000000000 (re)
 
+Test making empty commits
+  $ hg commit --allow-empty -m "empty commit"
+  $ hg log -r . -v --stat
+  changeset:   2:d809f3644287
+  tag:         tip
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  description:
+  empty commit
+  
+  
+  
 verify pathauditor blocks evil filepaths
   $ cat > evil-commit.py <<EOF
   > from mercurial import ui, hg, context, node
@@ -568,7 +580,7 @@  verify pathauditor blocks evil filepaths
 #endif
 
   $ hg rollback -f
-  repository tip rolled back to revision 1 (undo commit)
+  repository tip rolled back to revision 2 (undo commit)
   $ cat > evil-commit.py <<EOF
   > from mercurial import ui, hg, context, node
   > notrc = "HG~1/hgrc"
@@ -586,7 +598,7 @@  verify pathauditor blocks evil filepaths
   [255]
 
   $ hg rollback -f
-  repository tip rolled back to revision 1 (undo commit)
+  repository tip rolled back to revision 2 (undo commit)
   $ cat > evil-commit.py <<EOF
   > from mercurial import ui, hg, context, node
   > notrc = "HG8B6C~2/hgrc"
diff --git a/tests/test-completion.t b/tests/test-completion.t
--- a/tests/test-completion.t
+++ b/tests/test-completion.t
@@ -202,7 +202,7 @@  Show all commands + options
   add: include, exclude, subrepos, dry-run
   annotate: rev, follow, no-follow, text, user, file, date, number, changeset, line-number, 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, allow-empty, include, exclude, message, logfile, date, user, subrepos
   diff: rev, change, text, git, 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, nodates
   forget: include, exclude
diff --git a/tests/test-qrecord.t b/tests/test-qrecord.t
--- a/tests/test-qrecord.t
+++ b/tests/test-qrecord.t
@@ -63,6 +63,7 @@  help record (record)
       --amend               amend the parent of the working directory
    -s --secret              use the secret phase for committing
    -e --edit                invoke editor on commit messages
+      --allow-empty         allow commiting without pending changes
    -I --include PATTERN [+] include names matching the given patterns
    -X --exclude PATTERN [+] exclude names matching the given patterns
    -m --message TEXT        use text as commit message
diff --git a/tests/test-record.t b/tests/test-record.t
--- a/tests/test-record.t
+++ b/tests/test-record.t
@@ -50,6 +50,7 @@  Record help
       --amend               amend the parent of the working directory
    -s --secret              use the secret phase for committing
    -e --edit                invoke editor on commit messages
+      --allow-empty         allow commiting without pending changes
    -I --include PATTERN [+] include names matching the given patterns
    -X --exclude PATTERN [+] exclude names matching the given patterns
    -m --message TEXT        use text as commit message