Re: Cherry-picking commits with empty messages

Chris Webb [off-list ref] 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.

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.

   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

Junio C Hamano [off-list ref] 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.

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

On Wed, Aug 01, 2012 at 10:52:34AM -0700, Junio C Hamano wrote:

Chris Webb [off-list ref] 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

quoted

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

quoted

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.

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

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

Neil Horman [off-list ref] 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.

On Mon, Aug 06, 2012 at 12:00:16PM +0100, Chris Webb wrote:

Neil Horman [off-list ref] writes:

quoted

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>