From: D. Ben Knoble Date: Wed, 23 Sep 2026 12:58:03 GMT Subject: [PATCH v2 0/4] 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 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: [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 Helped-by: Phillip Wood - - ## 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 - ## 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