From: D. Ben Knoble Date: Tue, 29 Sep 2026 12:18:26 GMT Subject: [PATCH v4 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: 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: v2: v3: [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 +Author: Thomas Bachem ## 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 + 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 + Helped-by: D. Ben Knoble + Helped-by: Phillip Wood + Assisted-by: Claude Fable 5.1 + Signed-off-by: Thomas Bachem + + ## 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