[PATCH v5 0/8] Fix merge restore state

["Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>]

This started as a simple series to fix restore_state() in builtin/merge.c,
fixing an issue reported by ZheNing Hu[3]. It now fixes several bugs and has
grown so much it's hard to call it simple. Anyway...

Changes since v4:

 * Made use of the error() function in another place to simplify code
   (should have caught this in v3)
 * Split the fixes for 'resolve' and the trivial merge into separate
   patches, and make sure one doesn't mask the other but both codepaths are
   exercises in the testsuite
 * better test descriptions
 * use strvec to simplify some code

[1]
https://lore.kernel.org/git/CAOLTT8R7QmpvaFPTRs3xTpxr7eiuxF-ZWtvUUSC0-JOo9Y+SqA@mail.gmail.com/

Elijah Newren (8):
  merge-ort-wrappers: make printed message match the one from recursive
  merge-resolve: abort if index does not match HEAD
  merge: abort if index does not match HEAD for trivial merges
  merge: do not abort early if one strategy fails to handle the merge
  merge: fix save_state() to work when there are stat-dirty files
  merge: make restore_state() restore staged state too
  merge: ensure we can actually restore pre-merge state
  merge: do not exit restore_state() prematurely

 builtin/merge.c                          | 57 ++++++++++++++++-----
 git-merge-resolve.sh                     | 10 ++++
 merge-ort-wrappers.c                     |  4 +-
 t/t6402-merge-rename.sh                  |  2 +-
 t/t6424-merge-unrelated-index-changes.sh | 65 ++++++++++++++++++++++++
 t/t6439-merge-co-error-msgs.sh           |  1 +
 t/t7607-merge-state.sh                   | 32 ++++++++++++
 7 files changed, 154 insertions(+), 17 deletions(-)
 create mode 100755 t/t7607-merge-state.sh


base-commit: e72d93e88cb20b06e88e6e7d81bd1dc4effe453f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1231%2Fnewren%2Ffix-merge-restore-state-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1231/newren/fix-merge-restore-state-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1231

Range-diff vs v4:

 1:  bd36d16c8d9 = 1:  bd36d16c8d9 merge-ort-wrappers: make printed message match the one from recursive
 2:  b79f44e54b9 ! 2:  b656756fd37 merge-resolve: abort if index does not match HEAD
     @@ Commit message
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     - ## builtin/merge.c ##
     -@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
     - 		 */
     - 		refresh_cache(REFRESH_QUIET);
     - 		if (allow_trivial && fast_forward != FF_ONLY) {
     -+			/*
     -+			 * Must first ensure that index matches HEAD before
     -+			 * attempting a trivial merge.
     -+			 */
     -+			struct tree *head_tree = get_commit_tree(head_commit);
     -+			struct strbuf sb = STRBUF_INIT;
     -+
     -+			if (repo_index_has_changes(the_repository, head_tree,
     -+						   &sb)) {
     -+				struct strbuf err = STRBUF_INIT;
     -+				strbuf_addstr(&err, "error: ");
     -+				strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
     -+					    sb.buf);
     -+				strbuf_addch(&err, '\n');
     -+				fputs(err.buf, stderr);
     -+				strbuf_release(&err);
     -+				strbuf_release(&sb);
     -+				return -1;
     -+			}
     -+
     - 			/* See if it is really trivial. */
     - 			git_committer_info(IDENT_STRICT);
     - 			printf(_("Trying really trivial in-index merge...\n"));
     -
       ## git-merge-resolve.sh ##
      @@
       #
     @@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'resolve, non-triv
       	test_path_is_missing .git/MERGE_HEAD
       '
       
     -+test_expect_success 'resolve, trivial, related file removed' '
     -+	git reset --hard &&
     -+	git checkout B^0 &&
     -+
     -+	git rm a &&
     -+	test_path_is_missing a &&
     -+
     -+	test_must_fail git merge -s resolve C^0 &&
     -+
     -+	test_path_is_missing a &&
     -+	test_path_is_missing .git/MERGE_HEAD
     -+'
     -+
      +test_expect_success 'resolve, non-trivial, related file removed' '
      +	git reset --hard &&
      +	git checkout B^0 &&
 -:  ----------- > 3:  3adfd921995 merge: abort if index does not match HEAD for trivial merges
 3:  02930448ea1 ! 4:  c5755271cf1 merge: do not abort early if one strategy fails to handle the merge
     @@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'subtree' '
       	test_path_is_missing .git/MERGE_HEAD
       '
       
     -+test_expect_success 'resolve && recursive && ort' '
     ++test_expect_success 'with multiple strategies, recursive or ort failure do not early abort' '
      +	git reset --hard &&
      +	git checkout B^0 &&
      +
     @@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'subtree' '
      +	git add a &&
      +
      +	sane_unset GIT_TEST_MERGE_ALGORITHM &&
     -+	test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&
     ++	test_must_fail git merge -s recursive -s ort -s octopus C^0 >output 2>&1 &&
      +
     -+	grep "Trying merge strategy resolve..." output &&
      +	grep "Trying merge strategy recursive..." output &&
      +	grep "Trying merge strategy ort..." output &&
     ++	grep "Trying merge strategy octopus..." output &&
      +	grep "No merge strategy handled the merge." output
      +'
      +
 4:  daf8d224160 ! 5:  e7c6de9e0c1 merge: fix save_state() to work when there are stat-dirty files
     @@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'subtree' '
      +	git merge -s resolve -s recursive D^0
      +'
      +
     - test_expect_success 'resolve && recursive && ort' '
     + test_expect_success 'with multiple strategies, recursive or ort failure do not early abort' '
       	git reset --hard &&
       	git checkout B^0 &&
 5:  f401bd5ad0d ! 6:  d39d6472455 merge: make restore_state() restore staged state too
     @@ Commit message
          changes.  Fix this by adding the "--index" option to "git stash apply".
          While at it, also squelch the stash apply output; we already report
          "Rewinding the tree to pristine..." and don't need a detailed `git
     -    status` report afterwards.
     +    status` report afterwards.  Also while at it, switch to using strvec
     +    so folks don't have to count the arguments to ensure we avoided an
     +    off-by-one error, and so it's easier to add additional arguments to
     +    the command.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     @@ builtin/merge.c: static void reset_hard(const struct object_id *oid, int verbose
       			  const struct object_id *stash)
       {
      -	const char *args[] = { "stash", "apply", NULL, NULL };
     -+	const char *args[] = { "stash", "apply", "--index", "--quiet",
     -+			       NULL, NULL };
     ++	struct strvec args = STRVEC_INIT;
       
       	if (is_null_oid(stash))
       		return;
     @@ builtin/merge.c: static void reset_hard(const struct object_id *oid, int verbose
       	reset_hard(head, 1);
       
      -	args[2] = oid_to_hex(stash);
     -+	args[4] = oid_to_hex(stash);
     ++	strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
     ++	strvec_push(&args, oid_to_hex(stash));
       
       	/*
       	 * It is OK to ignore error here, for example when there was
     + 	 * nothing to restore.
     + 	 */
     +-	run_command_v_opt(args, RUN_GIT_CMD);
     ++	run_command_v_opt(args.v, RUN_GIT_CMD);
     ++	strvec_clear(&args);
     + 
     + 	refresh_cache(REFRESH_QUIET);
     + }
      
       ## t/t6424-merge-unrelated-index-changes.sh ##
     -@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'resolve && recursive && ort' '
     +@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'with multiple strategies, recursive or ort failure do not e
       
       	test_seq 0 10 >a &&
       	git add a &&
      +	git rev-parse :a >expect &&
       
       	sane_unset GIT_TEST_MERGE_ALGORITHM &&
     - 	test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&
     -@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'resolve && recursive && ort' '
     - 	grep "Trying merge strategy resolve..." output &&
     + 	test_must_fail git merge -s recursive -s ort -s octopus C^0 >output 2>&1 &&
     +@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'with multiple strategies, recursive or ort failure do not e
       	grep "Trying merge strategy recursive..." output &&
       	grep "Trying merge strategy ort..." output &&
     + 	grep "Trying merge strategy octopus..." output &&
      -	grep "No merge strategy handled the merge." output
      +	grep "No merge strategy handled the merge." output &&
      +
 6:  ad5354c219c = 7:  7f5c6884d68 merge: ensure we can actually restore pre-merge state
 7:  6212d572604 ! 8:  954dec526a2 merge: do not exit restore_state() prematurely
     @@ Commit message
      
       ## builtin/merge.c ##
      @@ builtin/merge.c: static void restore_state(const struct object_id *head,
     - 	const char *args[] = { "stash", "apply", "--index", "--quiet",
     - 			       NULL, NULL };
     + {
     + 	struct strvec args = STRVEC_INIT;
       
      -	if (is_null_oid(stash))
      -		return;
     @@ builtin/merge.c: static void restore_state(const struct object_id *head,
      +	if (is_null_oid(stash))
      +		goto refresh_cache;
      +
     - 	args[4] = oid_to_hex(stash);
     + 	strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
     + 	strvec_push(&args, oid_to_hex(stash));
       
     - 	/*
      @@ builtin/merge.c: static void restore_state(const struct object_id *head,
     - 	 */
     - 	run_command_v_opt(args, RUN_GIT_CMD);
     + 	run_command_v_opt(args.v, RUN_GIT_CMD);
     + 	strvec_clear(&args);
       
      -	refresh_cache(REFRESH_QUIET);
      +refresh_cache:
     @@ t/t7607-merge-state.sh (new)
      +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
      +. ./test-lib.sh
      +
     -+test_expect_success 'set up custom strategy' '
     ++test_expect_success 'Ensure we restore original state if no merge strategy handles it' '
      +	test_commit --no-tag "Initial" base base &&
      +
      +	for b in branch1 branch2 branch3

-- 
gitgitgadget