Cherry-picking commits with empty messages

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

index bf078f2..cf8bc05 100644
@@ -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.

Chris Webb <[email protected]> writes:

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

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.

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.

---
 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(+)

index 0e170a5..c205d23 100644
@@ -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

index 82d1bf8..5652f23 100644
@@ -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(),
 		};

index bf078f2..1ea5293 100644
@@ -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;

index aa5f17c..d849420 100644
@@ -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;

index 5a1340c..a0c6e30 100755
@@ -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 &&

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

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

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.

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?

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.

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

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.

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

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

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

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