cherry-pick: regression fix for empty commits

The earlier "--keep-redundant-commit" series broke "cherry-pick"
that is given a commit whose change is already in the current
history. Such a cherry-pick would result in an empty change, and
should stop with an error, telling the user that conflict resolution
may have made the result empty (which is exactly what is happening),
but we silently dropped the change on the floor without any message
nor non-zero exit code.

Signed-off-by: Junio C Hamano <gitster@pobox.com>

2 files changed, 75 insertions, 28 deletions

diff --git a/sequencer.c b/sequencer.c
index 72cb4ff82c..1b2168c1c2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -291,7 +291,8 @@ static int is_index_unchanged(void)
  * If we are revert, or if our cherry-pick results in a hand merge,
  * we had better say that the current user is responsible for that.
  */
-static int run_git_commit(const char *defmsg, struct replay_opts *opts)
+static int run_git_commit(const char *defmsg, struct replay_opts *opts,
+			  int allow_empty)
 {
 	struct argv_array array;
 	int rc;
@@ -307,7 +308,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 		argv_array_push(&array, defmsg);
 	}
 
-	if (opts->allow_empty)
+	if (allow_empty)
 		argv_array_push(&array, "--allow-empty");
 
 	rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
@@ -335,6 +336,44 @@ static int is_original_commit_empty(struct commit *commit)
 	return !hashcmp(ptree_sha1, commit->tree->object.sha1);
 }
 
+/*
+ * Do we run "git commit" with "--allow-empty"?
+ */
+static int allow_empty(struct replay_opts *opts, struct commit *commit)
+{
+	int index_unchanged, empty_commit;
+
+	/*
+	 * Three cases:
+	 *
+	 * (1) we do not allow empty at all and error out.
+	 *
+	 * (2) we allow ones that were initially empty, but
+	 * forbid the ones that become empty;
+	 *
+	 * (3) we allow both.
+	 */
+	if (!opts->allow_empty)
+		return 0; /* let "git commit" barf as necessary */
+
+	index_unchanged = is_index_unchanged();
+	if (index_unchanged < 0)
+		return index_unchanged;
+	if (!index_unchanged)
+		return 0; /* we do not have to say --allow-empty */
+
+	if (opts->keep_redundant_commits)
+		return 1;
+
+	empty_commit = is_original_commit_empty(commit);
+	if (empty_commit < 0)
+		return empty_commit;
+	if (!empty_commit)
+		return 0;
+	else
+		return 1;
+}
+
 static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 {
 	unsigned char head[20];
@@ -344,8 +383,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res;
-	int empty_commit;
-	int index_unchanged;
 
 	if (opts->no_commit) {
 		/*
@@ -471,10 +508,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		free_commit_list(remotes);
 	}
 
-	empty_commit = is_original_commit_empty(commit);
-	if (empty_commit < 0)
-		return empty_commit;
-
 	/*
 	 * If the merge was clean or if it failed due to conflict, we write
 	 * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
@@ -495,27 +528,11 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		print_advice(res == 1, opts);
 		rerere(opts->allow_rerere_auto);
 	} else {
-		index_unchanged = is_index_unchanged();
-		/*
-		 * If index_unchanged is less than 0, that indicates we either
-		 * couldn't parse HEAD or the index, so error out here.
-		 */
-		if (index_unchanged < 0)
-			return index_unchanged;
-
-		if (!empty_commit && !opts->keep_redundant_commits && index_unchanged)
-			/*
-			 * The head tree and the index match
-			 * meaning the commit is empty.  Since it wasn't created
-			 * empty (based on the previous test), we can conclude
-			 * the commit has been made redundant.  Since we don't
-			 * want to keep redundant commits, we can just return
-			 * here, skipping this commit
-			 */
-			return 0;
-
+		int allow = allow_empty(opts, commit);
+		if (allow < 0)
+			return allow;
 		if (!opts->no_commit)
-			res = run_git_commit(defmsg, opts);
+			res = run_git_commit(defmsg, opts, allow);
 	}
 
 	free_message(&msg);
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index 92f00cdf84..5a1340cee6 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -71,4 +71,34 @@ test_expect_success 'cherry pick with --keep-redundant-commits' '
 	git cherry-pick --keep-redundant-commits HEAD^
 '
 
+test_expect_success 'cherry-pick a commit that becomes no-op (prep)' '
+	git checkout master &&
+	git branch fork &&
+	echo foo >file2 &&
+	git add file2 &&
+	test_tick &&
+	git commit -m "add file2 on master" &&
+
+	git checkout fork &&
+	echo foo >file2 &&
+	git add file2 &&
+	test_tick &&
+	git commit -m "add file2 on the side"
+'
+
+test_expect_success 'cherry-pick a no-op without --keep-redundant' '
+	git reset --hard &&
+	git checkout fork^0 &&
+	test_must_fail git cherry-pick master
+'
+
+test_expect_success 'cherry-pick a no-op with --keep-redundant' '
+	git reset --hard &&
+	git checkout fork^0 &&
+	git cherry-pick --keep-redundant-commits master &&
+	git show -s --format='%s' >actual &&
+	echo "add file2 on master" >expect &&
+	test_cmp expect actual
+'
+
 test_done