Cherry-picking commits with empty messages

Cherry-picking commits with empty messages

[Chris Webb]

Whilst doing some extra sanity checking of my git-rebase--interactive.sh
patch yesterday, I came across a behaviour which has been present for some
time, but seems surprising. You can reproduce with

  $ git init -q foo && cd foo
  $ touch one && git add one && git commit -q -m one
  $ touch two && git add two && git commit -q -m two
  $ touch three && git add three && git commit -q -m '' --allow-empty-message
  $ touch four && git add four && git commit -q -m '' --allow-empty-message
  $ git rebase -i HEAD~3 # and swap the two commits with empty messages
  Aborting commit due to empty commit message.
  Could not apply 59a8fde... 

This happens on my ancient laptop which is apparently running 1.7.8.3, as well
as current master, so is unconnected to recent changes.

The reason is that git cherry-pick won't pick a commit with an empty commit
message, even when that message is unmodified from the original:

  $ git rebase --abort
  $ git checkout -q HEAD~2
  $ git cherry-pick 59a8fde
  Aborting commit due to empty commit message.

I can see that this check could make sense when the message has been
modified, but it seems strange when it hasn't, and isn't ideal behaviour
when called from rebase -i. (We otherwise make sure we call git commit with
--allow-empty-message to avoid problems with reordering or editing empty
commits.)

I could just remove the check in the 'message unmodified' case with
something like

diff --git a/sequencer.c b/sequencer.c
index bf078f2..cf8bc05 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -306,6 +306,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	if (!opts->edit) {
 		argv_array_push(&array, "-F");
 		argv_array_push(&array, defmsg);
+		argv_array_push(&array, "--allow-empty-message");
 	}
 
 	if (allow_empty)

but perhaps there are other users of the sequencer for whom this check is
desirable? If so, would an --allow-empty-message to git cherry-pick be a
better plan, which git rebase -i can use where appropriate?

Best wishes,

Chris.

Re: Cherry-picking commits with empty messages

[Junio C Hamano]

Chris Webb <chris@arachsys.com> writes:

[summary: this, when 59a8fde does not have any commit log message,
refuses to commit]

>   $ git cherry-pick 59a8fde
>   Aborting commit due to empty commit message.

> I can see that this check could make sense when the message has been
> modified, but it seems strange when it hasn't, and isn't ideal behaviour
> when called from rebase -i. (We otherwise make sure we call git commit with
> --allow-empty-message to avoid problems with reordering or editing empty
> commits.)
> 
> I could just remove the check in the 'message unmodified' case with
> something like
>
> diff --git a/sequencer.c b/sequencer.c
> index bf078f2..cf8bc05 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -306,6 +306,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  	if (!opts->edit) {
>  		argv_array_push(&array, "-F");
>  		argv_array_push(&array, defmsg);
> +		argv_array_push(&array, "--allow-empty-message");
>  	}
>  
>  	if (allow_empty)
>
> but perhaps there are other users of the sequencer for whom this check is
> desirable? If so, would an --allow-empty-message to git cherry-pick be a
> better plan, which git rebase -i can use where appropriate?

A few random thoughts.

 - Any Porcelain commands that implement the sequencing workflow, if
   they know what message to use when they internally run "commit"
   without allowing the user to edit the message, share the same
   issue.

 - We generally try to encourage users to describe commits, and
   commits with empty log messages are strongly frowned upon.

   In that sense, one could argue that cherry-pick did the right
   thing when it gave control back to you upon seeing an empty
   message.  The user is given a chance to fix the commit by running
   "git commit" at that point to give it a descriptive message.

 - These Porcelain programs, however, work from existing commits,
   and the reason why "git commit" invoked by them may be stopped
   due to empty log message is because the original commits had
   empty log message to begin with.  The user must have done so on
   purpose (e.g. by using "commit --allow-empty-message").

   In that sense, it is likely that the user will simply choose to
   run "git commit --allow-empty-message", even if given a chance by
   "cherry-pick" to correct the empty log message.  This is a
   counter-point to the "give the user a chance to fix" above.
   We _might_ not be adding much value to the system by giving the
   control back to the user.

 - We had a similar discussion on what should happen when one step
   in "cherry-pick" results in the same tree as the commit the
   'pick' builds on (i.e. an empty change).  The situation is a bit
   different from yours, because unlike the log message, an empty
   change can result by either (1) the original was an empty change,
   or (2) the change picked was already present in the updated base.
   We added "--keep-redundant-commits" and "--allow-empty" options
   to underlying "cherry-pick" to support this distinction.

   We may want to follow suit by triggering your change above only
   when "cherry-pick --allow-empty-message" was given.  This is
   siding with the "give the user a chance to fix" viewpoint to
   choose the default, and giving the users a way to overriding it.

 - Regarding the choice of default between "--allow-empty-message"
   vs "--no-allow-empty-message", one could argue that the best
   choice of the default depends on the Porcelain command.

   - A non-range cherry-pick (e.g. "cherry-pick A B C") is a strong
     hint from the user that the user wants to replay the specific
     commits that are named on the command line.  This fact may
     favor "the user must have done so on purpose" viewpoint over
     "give the user a chance to fix" viewpoint; defaulting to
     "--allow-empty-message" (and "--allow-empty", and perhaps
     "--keep-redundant-commits") might be more convenient for a
     non-range cherry-pick.

   - A range cherry-pick (e.g. "cherry-pick A..B") and "rebase -i",
     on the other hand, are primarily used to rebuild (and reorder
     in the case of "rebase -i") the history to clean it up, which
     may favor "give the user a chance to fix", i.e. defaulting not
     to enable "--allow-empty"-anything might be more convenient for
     a sequencing operation over a range in general.

   But from the bigger UI consistency point of view, it would be
   chaotic to change the default of some options for a single
   command depending on the nature of the operand, so I would
   recommend against going this route, and pick one view between
   "give the user a chance to fix" or "the user must have done so on
   purpose" and apply it consistently.

My recommendation, backed by the above line of thought, is to add
support for the "--allow-empty-message" option to both "rebase [-i]"
and "cherry-pick", defaulting to false.

Re: Cherry-picking commits with empty messages

[Angus Hammond]

>    But from the bigger UI consistency point of view, it would be
>    chaotic to change the default of some options for a single
>    command depending on the nature of the operand, so I would
>    recommend against going this route, and pick one view between
>    "give the user a chance to fix" or "the user must have done so on
>    purpose" and apply it consistently.
>
> My recommendation, backed by the above line of thought, is to add
> support for the "--allow-empty-message" option to both "rebase [-i]"
> and "cherry-pick", defaulting to false.

Though I completely agree regarding having a consistent UI that
doesn't change it's behaviour based on the operand, I'd argue that
--allow-empty-message should default to true on cherry-pick for a
couple or reasons. Firstly, in the case that git perpetuates an empty
commit message that the user does not want, it is only damaging a
repository in a way that it is already damaged, clearly this still
isn't ideal, but it's certainly not as bad as damaging a repository
that's pristine. Arguably it's the user's responsibility to ensure
they don't TELL git to perpetuate their own bad commit.

Secondly, I'd don't like the idea of a command that 99.9% of the time
will run completely independently, but then every so often will become
interactive. This is probably a rare enough scenario that script
writers would reasonably assume that cherry-pick (without the
--allow-empty-message flag) is not an interactive command and write
their scripts accordingly. A user who made use of empty commit
messages would find any such scripts crashing on them or producing
strange results. Even if this is the fringe case, it seems to be a
substantially worse fringe case than that where we make a commit that
has no message at the user's instruction.

Thanks
Angus

Re: Cherry-picking commits with empty messages

[Junio C Hamano]

Angus Hammond <angusgh@gmail.com> writes:

>>    But from the bigger UI consistency point of view, it would be
>>    chaotic to change the default of some options for a single
>>    command depending on the nature of the operand, so I would
>>    recommend against going this route, and pick one view between
>>    "give the user a chance to fix" or "the user must have done so on
>>    purpose" and apply it consistently.
>>
>> My recommendation, backed by the above line of thought, is to add
>> support for the "--allow-empty-message" option to both "rebase [-i]"
>> and "cherry-pick", defaulting to false.
>
> Though I completely agree regarding having a consistent UI that
> doesn't change it's behaviour based on the operand, I'd argue that
> --allow-empty-message should default to true on cherry-pick for a
> couple or reasons.

I've read your entire response three times, and I am having a hard
time deciding if you are against my suggestion, or you misread my
suggestion.

> Firstly, in the case that git perpetuates an empty
> commit message that the user does not want, it is only damaging a
> repository in a way that it is already damaged, clearly this still
> isn't ideal, but it's certainly not as bad as damaging a repository
> that's pristine. Arguably it's the user's responsibility to ensure
> they don't TELL git to perpetuate their own bad commit.

I guess by "perpetuates" you meant there was already a commit with
an empty message, by "the user does not want" you consider such a
commit is a bad thing, and by "to ensure they don't TELL git", you
meant it is the user's responsibility not to give an extra option to
cause Git to replay a bad (= having an empty message) commit and
leave it in the resulting history.

It sounds to me that you are advocating for "git cherry-pick"
without any flags to stop and do not commit when given a commit with
an empty message.

And that is what I thought I was suggesting.  Give users a support
to say "git cherry-pick --allow-empty", but do not by default enable
it.  Perhaps I sounded as if I was suggesting the opposite?

> Secondly, I'd don't like the idea of a command that 99.9% of the time
> will run completely independently, but then every so often will become
> interactive.

As "cherry-pick" is expected to stop and give control back whenever
there is conflicts, this does not apply.  Any script that uses
cherry-pick to replay an existing commit has to be prepared to see
it stop and give control back to the script already, or the script
is unusable.  Note that the script would not be buggy even if the
only thing it does when it sees cherry-pick stop and give control to
it is to abort and give control back to the user.

Re: Cherry-picking commits with empty messages

[Angus Hammond]

On 1 August 2012 23:26, Junio C Hamano <gitster@pobox.com> wrote:
> I've read your entire response three times, and I am having a hard
> time deciding if you are against my suggestion, or you misread my
> suggestion.

My apologies, I can see how my message wasn't as clear as it could have been.

> I guess by "perpetuates" you meant there was already a commit with
> an empty message, by "the user does not want" you consider such a
> commit is a bad thing, and by "to ensure they don't TELL git", you
> meant it is the user's responsibility not to give an extra option to
> cause Git to replay a bad (= having an empty message) commit and
> leave it in the resulting history.

I was just trying to say that cherry-pick will only ever create a
blank commit message where one already exists in the repository, so
git shouldn't worry about copying that blank commit message,
especially since the user has explicitly told git (by running
cherry-pick) that they want those commits copied.

> It sounds to me that you are advocating for "git cherry-pick"
> without any flags to stop and do not commit when given a commit with
> an empty message.
>
> And that is what I thought I was suggesting.  Give users a support
> to say "git cherry-pick --allow-empty", but do not by default enable
> it.  Perhaps I sounded as if I was suggesting the opposite?

I meant the opposite, that without any flags it should just copy the
blank commit silently.

>> Secondly, I'd don't like the idea of a command that 99.9% of the time
>> will run completely independently, but then every so often will become
>> interactive.
>
> As "cherry-pick" is expected to stop and give control back whenever
> there is conflicts, this does not apply.  Any script that uses
> cherry-pick to replay an existing commit has to be prepared to see
> it stop and give control back to the script already, or the script
> is unusable.  Note that the script would not be buggy even if the
> only thing it does when it sees cherry-pick stop and give control to
> it is to abort and give control back to the user.

Fair enough, I don't make heavy use of cherry-pick, so that didn't occur to me.

Since you've pointed out that the scripting argument is invalid, I'm
now inclined to support what you originally proposed (by default
refuse to create an empty commit message, offer a flag to override
that default), but just wanted to clear up my original point since it
wasn't written very clearly.

Thanks
Angus

Re: Cherry-picking commits with empty messages

[Chris Webb]

Junio C Hamano <gitster@pobox.com> writes:

> My recommendation, backed by the above line of thought, is to add
> support for the "--allow-empty-message" option to both "rebase [-i]"
> and "cherry-pick", defaulting to false.

Thanks for the very detailed analysis and advice Junio. I like your
suggested --allow-empty-message option for cherry-pick because it's
consistent with the same option in standard commit, and doesn't change the
behaviour for existing users who might rely on cherry-pick catching blank
messages.

With rebase -i, the fix might be slightly more involved than just passing
through --allow-empty-message (if given) to cherry-pick, especially given
that sometimes we git cherry-pick -n && git commit --allow-empty-message,
and at other times we do standard git cherry-pick which refuses to pick a
commit without a message.

Given a history with empty commits, as a general principle it feels like it
should be possible to edit or reword those commits to make them non-empty
without giving --allow-empty-message, but that to generate new history
containing empty messages, --allow-empty-message should be required, whether
to commit [--amend] during rebase, or to the rebase -i command itself.

Cheers,

Chris.

[PATCH] cherry-pick: add --allow-empty-message option

[Chris Webb]

Scripts such as git rebase -i cannot currently cherry-pick commits which
have an empty commit message, as git cherry-pick calls git commit
without the --allow-empty-message option.

Add an --allow-empty-message option to git cherry-pick which is passed
through to git commit, so this behaviour can be overridden.

Signed-off-by: Chris Webb <chris@arachsys.com>
---
 Documentation/git-cherry-pick.txt | 5 +++++
 builtin/revert.c                  | 2 ++
 sequencer.c                       | 3 +++
 sequencer.h                       | 1 +
 t/t3505-cherry-pick-empty.sh      | 5 +++++
 5 files changed, 16 insertions(+)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 0e170a5..c205d23 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -118,6 +118,11 @@ effect to your index in a row.
 	previous commit are dropped.  To force the inclusion of those commits
 	use `--keep-redundant-commits`.
 
+--allow-empty-message::
+	By default, cherry-picking a commit with an empty message will fail.
+	This option overrides that behaviour, allowing commits with empty
+	messages to be cherry picked.
+
 --keep-redundant-commits::
 	If a commit being cherry picked duplicates a commit already in the
 	current history, it will become empty.  By default these
diff --git a/builtin/revert.c b/builtin/revert.c
index 82d1bf8..5652f23 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -117,6 +117,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
+		OPT_END(),
 	};
 
 	if (opts->action == REPLAY_PICK) {
@@ -124,6 +125,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
 			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve initially empty commits"),
+			OPT_BOOLEAN(0, "allow-empty-message", &opts->allow_empty_message, "allow commits with empty messages"),
 			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_redundant_commits, "keep redundant, empty commits"),
 			OPT_END(),
 		};
diff --git a/sequencer.c b/sequencer.c
index bf078f2..1ea5293 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -311,6 +311,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	if (allow_empty)
 		argv_array_push(&array, "--allow-empty");
 
+	if (opts->allow_empty_message)
+		argv_array_push(&array, "--allow-empty-message");
+
 	rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
 	argv_array_clear(&array);
 	return rc;
diff --git a/sequencer.h b/sequencer.h
index aa5f17c..d849420 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -30,6 +30,7 @@ struct replay_opts {
 	int allow_ff;
 	int allow_rerere_auto;
 	int allow_empty;
+	int allow_empty_message;
 	int keep_redundant_commits;
 
 	int mainline;
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index 5a1340c..a0c6e30 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -53,6 +53,11 @@ test_expect_success 'index lockfile was removed' '
 
 '
 
+test_expect_success 'cherry-pick a commit with an empty message with --allow-empty-message' '
+	git checkout -f master &&
+	git cherry-pick --allow-empty-message empty-branch
+'
+
 test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '
 	git checkout master &&
 	echo fourth >>file2 &&

Re: [PATCH] cherry-pick: add --allow-empty-message option

[Neil Horman]

On Thu, Aug 02, 2012 at 11:38:51AM +0100, Chris Webb wrote:
> Scripts such as git rebase -i cannot currently cherry-pick commits which
> have an empty commit message, as git cherry-pick calls git commit
> without the --allow-empty-message option.
> 
> Add an --allow-empty-message option to git cherry-pick which is passed
> through to git commit, so this behaviour can be overridden.
> 
> Signed-off-by: Chris Webb <chris@arachsys.com>
Sorry for the late response, but I just pulled back into town.

Having read over this thread, I think this is definately the way to go.  As
discussed having cherry-pick stop and give the user a chance to fix empty
history messages by default, and providing a switch to override that behavior
makes sense to me.  That said, shouldn't there be extra code here in the rebase
scripts to automate commit migration in that path as well?
Neil

> 

Re: [PATCH] cherry-pick: add --allow-empty-message option

[Chris Webb]

Neil Horman <nhorman@tuxdriver.com> writes:

> Having read over this thread, I think this is definately the way to go.  As
> discussed having cherry-pick stop and give the user a chance to fix empty
> history messages by default, and providing a switch to override that behavior
> makes sense to me.  That said, shouldn't there be extra code here in the rebase
> scripts to automate commit migration in that path as well?

Yes, this patch just adds the support to the low-level git cherry-pick as
you say. I'll follow up with a patch to use the new feature in rebase [-i]
when I get some free time, hopefully later this week.

Cheers,

Chris.

Re: [PATCH] cherry-pick: add --allow-empty-message option

[Neil Horman]

On Mon, Aug 06, 2012 at 12:00:16PM +0100, Chris Webb wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > Having read over this thread, I think this is definately the way to go.  As
> > discussed having cherry-pick stop and give the user a chance to fix empty
> > history messages by default, and providing a switch to override that behavior
> > makes sense to me.  That said, shouldn't there be extra code here in the rebase
> > scripts to automate commit migration in that path as well?
> 
> Yes, this patch just adds the support to the low-level git cherry-pick as
> you say. I'll follow up with a patch to use the new feature in rebase [-i]
> when I get some free time, hopefully later this week.
> 
> Cheers,
> 
> Chris.
> 
Ok, then
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: Cherry-picking commits with empty messages

[Neil Horman]

On Wed, Aug 01, 2012 at 10:52:34AM -0700, Junio C Hamano wrote:
> Chris Webb <chris@arachsys.com> writes:
> 
> [summary: this, when 59a8fde does not have any commit log message,
> refuses to commit]
> 
Thanks for CC'ing me on this.  I'm on vacation currently, but will look at this
in detail as soon as I'm back next week
Neil

> >   $ git cherry-pick 59a8fde
> >   Aborting commit due to empty commit message.
> 
> > I can see that this check could make sense when the message has been
> > modified, but it seems strange when it hasn't, and isn't ideal behaviour
> > when called from rebase -i. (We otherwise make sure we call git commit with
> > --allow-empty-message to avoid problems with reordering or editing empty
> > commits.)
> > 
> > I could just remove the check in the 'message unmodified' case with
> > something like
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index bf078f2..cf8bc05 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -306,6 +306,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> >  	if (!opts->edit) {
> >  		argv_array_push(&array, "-F");
> >  		argv_array_push(&array, defmsg);
> > +		argv_array_push(&array, "--allow-empty-message");
> >  	}
> >  
> >  	if (allow_empty)
> >
> > but perhaps there are other users of the sequencer for whom this check is
> > desirable? If so, would an --allow-empty-message to git cherry-pick be a
> > better plan, which git rebase -i can use where appropriate?
> 
> A few random thoughts.
> 
>  - Any Porcelain commands that implement the sequencing workflow, if
>    they know what message to use when they internally run "commit"
>    without allowing the user to edit the message, share the same
>    issue.
> 
>  - We generally try to encourage users to describe commits, and
>    commits with empty log messages are strongly frowned upon.
> 
>    In that sense, one could argue that cherry-pick did the right
>    thing when it gave control back to you upon seeing an empty
>    message.  The user is given a chance to fix the commit by running
>    "git commit" at that point to give it a descriptive message.
> 
>  - These Porcelain programs, however, work from existing commits,
>    and the reason why "git commit" invoked by them may be stopped
>    due to empty log message is because the original commits had
>    empty log message to begin with.  The user must have done so on
>    purpose (e.g. by using "commit --allow-empty-message").
> 
>    In that sense, it is likely that the user will simply choose to
>    run "git commit --allow-empty-message", even if given a chance by
>    "cherry-pick" to correct the empty log message.  This is a
>    counter-point to the "give the user a chance to fix" above.
>    We _might_ not be adding much value to the system by giving the
>    control back to the user.
> 
>  - We had a similar discussion on what should happen when one step
>    in "cherry-pick" results in the same tree as the commit the
>    'pick' builds on (i.e. an empty change).  The situation is a bit
>    different from yours, because unlike the log message, an empty
>    change can result by either (1) the original was an empty change,
>    or (2) the change picked was already present in the updated base.
>    We added "--keep-redundant-commits" and "--allow-empty" options
>    to underlying "cherry-pick" to support this distinction.
> 
>    We may want to follow suit by triggering your change above only
>    when "cherry-pick --allow-empty-message" was given.  This is
>    siding with the "give the user a chance to fix" viewpoint to
>    choose the default, and giving the users a way to overriding it.
> 
>  - Regarding the choice of default between "--allow-empty-message"
>    vs "--no-allow-empty-message", one could argue that the best
>    choice of the default depends on the Porcelain command.
> 
>    - A non-range cherry-pick (e.g. "cherry-pick A B C") is a strong
>      hint from the user that the user wants to replay the specific
>      commits that are named on the command line.  This fact may
>      favor "the user must have done so on purpose" viewpoint over
>      "give the user a chance to fix" viewpoint; defaulting to
>      "--allow-empty-message" (and "--allow-empty", and perhaps
>      "--keep-redundant-commits") might be more convenient for a
>      non-range cherry-pick.
> 
>    - A range cherry-pick (e.g. "cherry-pick A..B") and "rebase -i",
>      on the other hand, are primarily used to rebuild (and reorder
>      in the case of "rebase -i") the history to clean it up, which
>      may favor "give the user a chance to fix", i.e. defaulting not
>      to enable "--allow-empty"-anything might be more convenient for
>      a sequencing operation over a range in general.
> 
>    But from the bigger UI consistency point of view, it would be
>    chaotic to change the default of some options for a single
>    command depending on the nature of the operand, so I would
>    recommend against going this route, and pick one view between
>    "give the user a chance to fix" or "the user must have done so on
>    purpose" and apply it consistently.
> 
> My recommendation, backed by the above line of thought, is to add
> support for the "--allow-empty-message" option to both "rebase [-i]"
> and "cherry-pick", defaulting to false.
>