[PATCH v4 0/5] 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 missing test coverage
(which catch breakages from prior incorrect rounds of this series). The
fourth fixes a test interaction with another in-flight topic. The last
holds the interesting bits.

Changes in v4:
• Drop merge verbosity changes altogether. I was going to
  save-and-restore, but when looking at the index-merge test case (more
  below) closer, I noticed that "git apply --cached" reports conflicts
  on stderr. That is, "git stash apply --index" would report conflicts,
  and silencing the merge takes that away. So instead let's leave the
  configured verbosity alone.
• Only copy resulting index merge tree OID when successful
• Fix interaction with t5520 (new patch 4/5)
• Squash test from 3/5 into 5/5, since it requires actually merging
  trees. I've elected to keep it a separate test for now (contrary to
  Phillip's suggestion) since it's written and working. Adapting
  existing tests requires quite a bit more digging into implicit context
  assumptions ;)

Changes in v3:

• Change conflict label for current index
• Fix memory leak of merge_result
• Fix order of trees to make the correct merge (cherry-pick)
    • New test (3/5) to validate this
• Fix test in 4/5 to assert more details of expected state

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>
v2: <cover.1790168285.git.ben.knoble@gmail.com>
v3: <cover.1790425008.git.ben.knoble@gmail.com>

[1/5] builtin/stash: remove unused header
[2/5] stash: prepare merge options earlier
[3/5] t3903: test failed "stash apply --index"
[4/5] t5520: don't expire reflogs where it matters
[5/5] builtin/stash: merge index in-core

 builtin/stash.c  | 91 ++++++++++++------------------------------------
 t/t3903-stash.sh | 42 ++++++++++++++++++++++
 t/t5520-pull.sh  |  6 ++++
 t/t7600-merge.sh |  9 +++++
 4 files changed, 79 insertions(+), 69 deletions(-)

Diff-intervalle contre v3 :
1:  6a165c4df4 = 1:  6a165c4df4 builtin/stash: remove unused header
2:  d9a9e18f3a ! 2:  35b64ae321 stash: prepare merge options earlier
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
      		return error(_("cannot apply a stash in the middle of a merge"));
      
     +	init_ui_merge_options(&o, the_repository);
    ++
    ++	if (quiet)
    ++		o.verbosity = 0;
     +
      	if (index) {
      		if (oideq(&info->b_tree, &info->i_tree) ||
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
      	o.branch1 = label_ours ? label_ours : "Updated upstream";
      	o.branch2 = label_theirs ? label_theirs : "Stashed changes";
      	o.ancestor = label_base ? label_base : "Stash base";
    +@@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefix,
    + 	if (oideq(&info->b_tree, &c_tree))
    + 		o.branch1 = "Version stash was based on";
    + 
    +-	if (quiet)
    +-		o.verbosity = 0;
    +-
    + 	if (o.verbosity >= 3)
    + 		printf_ln(_("Merging %s with %s"), o.branch1, o.branch2);
    + 
4:  d39e16905d ! 3:  7b0b317ce0 t3903: test failed "stash apply --index"
    @@ Commit message
     
      ## t/t3903-stash.sh ##
     @@ t/t3903-stash.sh: setup_stash() {
    - 	test_cmp expect file
    + 	test_cmp expect actual
      '
      
     +test_expect_success 'stash apply --index leaves everything untouched on failure' '
3:  8b5ea5e6f4 ! 4:  2ac371d2dc t3903: test stash --index merges
    @@
      ## Metadata ##
    -Author: D. Ben Knoble <ben.knoble@gmail.com>
    +Author: Thomas Bachem <mail@thomasbachem.com>
     
      ## Commit message ##
    -    t3903: test stash --index merges
    +    t5520: don't expire reflogs where it matters
     
    -    A future commit will refactor index handling for applied stashes, and we
    -    need to take care to get the order of trees right when merging. Add a
    -    test that covers this case.
    +    The "--rebase -f with rebased upstream" test computes its fork point
    +    from the reflog of refs/remotes/me/copy, and the entry it needs is
    +    the one that the fetch of the test before it wrote. Like every reflog
    +    entry the suite writes after test_tick, it is dated 2005, so the
    +    first "git reflog expire --all" after that fetch removes it. Pull
    +    then finds no fork point and rebases onto the merge head with the
    +    merge head as the upstream, and the rewound commits come back as a
    +    conflict.
     
    -    Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
    +    Since 452b12c2e0 (builtin/maintenance: use "geometric" strategy by
    +    default, 2026-02-24) auto maintenance runs that expiry once the reflog
    +    of HEAD holds a hundred entries it would remove, the default of
    +    maintenance.reflog-expire.auto. Which run crosses the threshold
    +    depends on the entries and maintenance runs before it, so the script
    +    passed by chance: a stash topic that no longer runs "git reset" from
    +    "stash apply --index" and a rebase topic that runs auto maintenance
    +    at the end of "git rebase" together move the expiry between the two
    +    tests.
     
    - ## t/t3903-stash.sh ##
    -@@ t/t3903-stash.sh: setup_stash() {
    - 	test_cmp expect actual
    - '
    +    Pin the expiry as ea7d894f44 (t34xx: don't expire reflogs where it
    +    matters, 2026-02-24) did for the rebase tests. That covers a "git gc"
    +    as well, which expires reflogs on its own, where turning off the auto
    +    trigger of the reflog-expire task alone would not.
    +
    +    Reported-by: Junio C Hamano <gitster@pobox.com>
    +    Helped-by: D. Ben Knoble <ben.knoble@gmail.com>
    +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
    +    Assisted-by: Claude Fable 5.1
    +    Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
    +
    + ## t/t5520-pull.sh ##
    +@@ t/t5520-pull.sh: test_pull_autostash_fail () {
    + }
      
    -+# the later "stash -k" test is not expecting us to muck with file so much, so
    -+# reset when finished
    -+test_expect_success 'stash apply --index merges the correct trees' '
    -+	head=$(git rev-parse HEAD) &&
    -+	test_when_finished "git reset --hard $head" &&
    -+	test_write_lines A B C >file &&
    -+	git commit -m setup file &&
    -+	test_write_lines A B staged >file &&
    -+	git add file &&
    -+	test_write_lines A B unstaged >file &&
    -+	git stash &&
    -+	test_write_lines committed B C >file &&
    -+	git commit -m to-be-merged file &&
    -+	git stash pop --index &&
    -+	git show :file >actual &&
    -+	test_write_lines committed B staged >expect &&
    -+	test_cmp expect actual &&
    -+	test_write_lines committed B unstaged >expect &&
    -+	test_cmp expect file
    -+'
    + test_expect_success setup '
    ++	# Commit dates are hardcoded to 2005, and the reflog entries will have
    ++	# a matching timestamp. Maintenance may thus immediately expire
    ++	# reflogs if it was running.
    ++	git config set gc.reflogExpire never &&
    ++	git config set gc.reflogExpireUnreachable never &&
     +
    - test_expect_success 'stash -k' '
    - 	echo bar3 >file &&
    - 	echo bar4 >file2 &&
    + 	echo file >file &&
    + 	git add file &&
    + 	git commit -a -m original
5:  fde7fb7988 ! 5:  e21b832a6e builtin/stash: merge index in-core
    @@ Commit message
         we don't see the usual branch and ancestor labels, but the merge
         subroutines insist on their presence, so use something simple.
     
    +    We need to take care to get the order of trees right when merging. Add a
    +    test that covers this case.
    +
         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
         prefer to clean up these vestiges of the shell-based git-stash, though.
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
     -			ret = apply_cached(&out);
     -			strbuf_release(&out);
     -			if (ret)
    -+			o.verbosity = 0;
    -+
     +			head = lookup_tree(o.repo, &c_tree);
     +			merge = lookup_tree(o.repo, &info->i_tree);
     +			merge_base = lookup_tree(o.repo, &info->b_tree);
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
     +			merge_incore_nonrecursive(&o, merge_base, head, merge,
     +						  &result);
     +
    -+			oidcpy(&index_tree, &result.tree->object.oid);
    -+			merge_finalize(&o, &result);
    -+
    -+			if (!result.clean)
    ++			if (!result.clean) {
    ++				merge_finalize(&o, &result);
      				return error(_("conflicts in index. "
      					       "Try without --index."));
     -
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
     -			reset_head();
     -			discard_index(the_repository->index);
     -			repo_read_index(the_repository);
    ++			} else {
    ++				oidcpy(&index_tree, &result.tree->object.oid);
    ++				merge_finalize(&o, &result);
    ++			}
      		}
      	}
      
     
    + ## t/t3903-stash.sh ##
    +@@ t/t3903-stash.sh: setup_stash() {
    + 	test_cmp expect-index actual-index
    + '
    + 
    ++# the later "stash -k" test is not expecting us to muck with file so much, so
    ++# reset when finished
    ++test_expect_success 'stash apply --index merges the correct trees' '
    ++	head=$(git rev-parse HEAD) &&
    ++	test_when_finished "git reset --hard $head" &&
    ++	test_write_lines A B C >file &&
    ++	git commit -m setup file &&
    ++	test_write_lines A B staged >file &&
    ++	git add file &&
    ++	test_write_lines A B unstaged >file &&
    ++	git stash &&
    ++	test_write_lines committed B C >file &&
    ++	git commit -m to-be-merged file &&
    ++	git stash pop --index &&
    ++	git show :file >actual &&
    ++	test_write_lines committed B staged >expect &&
    ++	test_cmp expect actual &&
    ++	test_write_lines committed B unstaged >expect &&
    ++	test_cmp expect file
    ++'
    ++
    + test_expect_success 'stash -k' '
    + 	echo bar3 >file &&
    + 	echo bar4 >file2 &&
    +
      ## t/t7600-merge.sh ##
     @@ t/t7600-merge.sh: verify_no_mergehead () {
      	test_cmp result.1-5 file

base-commit: d38352cd43ab9745686d697872408bc3249a153f
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty