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

[Phillip Wood <phillip.wood123@gmail.com>]

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