[PATCH v2 0/4] stash: clean up index-mode test merge

["D. Ben Knoble" <ben.knoble@gmail.com>]

Hi all,

This small patch series fixes a bug reported by Eli Barzilay in the
interaction between autostashing, staged index entries, and
stash.index=true.

The first patch is an incidental cleanup, and the second re-arranges one
line to make the change easier. The third adds a new test, while the
fourth holds the interesting bits.

Changes in v2:

• Do give branch labels for the incore merge, although they are never
  seen (and clarify commit message as a result, also keeping the
  merge-ort asserts). Phillip was right: without those, we do segfault
  on conflicts.
• Use the ui merge options to keep the same diff algorithm.
• Use merge_finalize instead of clear_merge_options, and reuse the
  options between merge calls if they are already initialized.
• Add a new 2/4 to simplify merge options initialization.
• Add a new 3/4 with a test case for conflicted index merges.

v1: <cover.1789853192.git.ben.knoble@gmail.com>

[1/4] builtin/stash: remove unused header
[2/4] stash: prepare merge options earlier
[3/4] t: test failed "stash apply --index"
[4/4] builtin/stash: merge index in-core

 builtin/stash.c  | 83 +++++++++++-------------------------------------
 t/t3903-stash.sh | 18 +++++++++++
 t/t7600-merge.sh |  9 ++++++
 3 files changed, 45 insertions(+), 65 deletions(-)

Diff-intervalle contre v1 :
1:  b6798c8a25 = 1:  b6798c8a25 builtin/stash: remove unused header
-:  ---------- > 2:  1e2343c7fc stash: prepare merge options earlier
-:  ---------- > 3:  5bd4b78cac t: test failed "stash apply --index"
2:  782fe91251 ! 4:  e49936ee12 builtin/stash: merge index in-core
    @@ Commit message
     
         Fortunately, we can achieve 2 goals at once: avoid round-tripping to the
         file-system (and invoking expensive subprocesses) by performing the
    -    merge in-core. Since the results are never seen, we don't need to set
    -    the usual branch and ancestor labels.
    +    merge in-core. If there are conflicts, we discard the resulting tree, so
    +    we don't see the usual branch and ancestor labels, but the merge
    +    subroutines insist on their presence, so use something simple.
     
         We *could* swap just the git-reset(1) subprocess with our internal
         reset_tree() and refresh_index(), which would fix the bug. We'd much
    @@ Commit message
         Reported-by: Eli Barzilay <eli@barzilay.org>
         Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
    -
    - ## Notes (benknoble/commits) ##
    -    We *could* leave the asserts in, but then we somewhat uselessly set the
    -    conflict labels, which I did in the original patch [1]. Phillip
    -    suggested we don't need them, and I otherwise agree.
    -
    -    [1]: https://lore.kernel.org/git/CALnO6CDfwscMWZktBu3FtXOQVbcBRo76nqK07kMnrzC5cPyZiQ@mail.gmail.com/
    -
    -    In all the versions of 231e2dd49d (merge-ort: add some high-level
    -    algorithm structure, 2020-12-13) I could find on the mailing list, the
    -    "assert(opt->ancestor)" is present without explanation or comment, so
    -    I'm not in a good place to assess the impact of removing it and its
    -    compatriots.
    -
    -    Cc: Elijah Newren <newren@gmail.com>
    -
      ## builtin/stash.c ##
     @@ builtin/stash.c: static int create_index_from_tree(const struct object_id *tree_id,
      	return ret;
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
     -				return error(_("could not generate diff %s^!."),
     -					     oid_to_hex(&info->w_commit));
     -			}
    -+			init_basic_merge_options(&o, the_repository);
    ++			o.branch1 = "Upstream index";
    ++			o.branch2 = "Stashed index changes";
    ++			o.ancestor = "Stash base";
      
     -			ret = apply_cached(&out);
     -			strbuf_release(&out);
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
     -			discard_index(the_repository->index);
     -			repo_read_index(the_repository);
     +			oidcpy(&index_tree, &result.tree->object.oid);
    -+			clear_merge_options(&o);
    ++			merge_finalize(&o, &result);
      		}
      	}
      
     
    - ## merge-ort.c ##
    -@@ merge-ort.c: static void merge_start(struct merge_options *opt, struct merge_result *result)
    - 	trace2_region_enter("merge", "sanity checks", opt->repo);
    - 	assert(opt->repo);
    - 
    --	assert(opt->branch1 && opt->branch2);
    --
    - 	assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&
    - 	       opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);
    - 	assert(opt->rename_limit >= -1);
    -@@ merge-ort.c: void merge_incore_nonrecursive(struct merge_options *opt,
    - 	trace2_region_enter("merge", "incore_nonrecursive", opt->repo);
    - 
    - 	trace2_region_enter("merge", "merge_start", opt->repo);
    --	assert(opt->ancestor != NULL);
    - 	merge_check_renames_reusable(opt, result, merge_base, side1, side2);
    - 	merge_start(opt, result);
    - 	/*
    -
      ## t/t7600-merge.sh ##
     @@ t/t7600-merge.sh: verify_no_mergehead () {
      	test_cmp result.1-5 file

base-commit: 339ab2a8f14c0c304ae2f28df1a859f3d2cf610c
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty