From: Phillip Wood Date: Thu, 01 Oct 2026 15:52:19 GMT Subject: Re: [PATCH v5 0/4] stash: clean up index-mode test merge Message-ID: In-Reply-To: Hi Ben On 30/09/2026 22:24, D. Ben Knoble wrote: > > Changes in v5: > • Rebase on synthetic merge for the test interaction with t5520 > (dropping old 4/5) [59d1ce1b6e (Merge branch 'tb/t5520-reflog-expire' > into dk/stash-apply-index-incore, 2026-09-29)] > • Fix handling of tri-state merge_result.clean The range-diff below looks as expected, thanks for working on this, I'm really pleased to see us removing some subprocesses from "git stash". Thanks Phillip > 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: > v4: > > [1/4] builtin/stash: remove unused header > [2/4] stash: prepare merge options earlier > [3/4] t3903: test failed "stash apply --index" > [4/4] builtin/stash: merge index in-core > > builtin/stash.c | 94 +++++++++++++----------------------------------- > t/t3903-stash.sh | 42 ++++++++++++++++++++++ > t/t7600-merge.sh | 9 +++++ > 3 files changed, 76 insertions(+), 69 deletions(-) > > Diff-intervalle contre v4 : > 1: 6a165c4df4 = 1: d8f4c36459 builtin/stash: remove unused header > 2: 35b64ae321 = 2: 8e99033ef0 stash: prepare merge options earlier > 3: 7b0b317ce0 = 3: ee28d0a840 t3903: test failed "stash apply --index" > 4: 2ac371d2dc < -: ---------- t5520: don't expire reflogs where it matters > 5: e21b832a6e ! 4: ca3de1d4a3 builtin/stash: merge index in-core > @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi > + merge_incore_nonrecursive(&o, merge_base, head, merge, > + &result); > + > -+ if (!result.clean) { > ++ if (result.clean < 0) { > ++ merge_finalize(&o, &result); > ++ return error(_("index merge failed")); > ++ } else if (!result.clean) { > + merge_finalize(&o, &result); > return error(_("conflicts in index. " > "Try without --index.")); > > base-commit: a018953688f1b10bddf91bff8747068f5f4746a4 > prerequisite-patch-id: 601853fa5478b0dbfb260ba02632418e90338219