# [PATCH 0/2] Hi all, 78 messages from 2026-09-19 to 2026-10-01. Participants: D. Ben Knoble, Phillip Wood, Junio C Hamano, Thomas Bachem, Ben Knoble. Thread: https://gitlist.dev/t/66355 ## D. Ben Knoble, 2026-09-19 21:26 Subject: [PATCH 0/2] Hi all, Message-ID: ``` 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, while the second holds the interesting bits. Preferences on keeping or removing a few assert() calls in merge-ort.c are welcome. [1/2] builtin/stash: remove unused header [2/2] builtin/stash: merge index in-core builtin/stash.c | 77 +++++++++--------------------------------------- merge-ort.c | 3 -- t/t7600-merge.sh | 9 ++++++ 3 files changed, 23 insertions(+), 66 deletions(-) base-commit: 339ab2a8f14c0c304ae2f28df1a859f3d2cf610c -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-19 21:26 Subject: [PATCH 1/2] builtin/stash: remove unused header Message-ID: In-Reply-To: ``` Clang complains that oid-array.h is unused. Certainly none of the oid_array* functions, types, etc., are used, and the transitively-included hash.h declarations are used but covered by a pre-existing direct #include of hash.h. Signed-off-by: D. Ben Knoble --- builtin/stash.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index 7a9843413b..dfea2d2c4c 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -31,7 +31,6 @@ #include "reflog.h" #include "reflog-walk.h" #include "add-interactive.h" -#include "oid-array.h" #include "commit.h" #define INCLUDE_ALL_FILES 2 -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-19 21:26 Subject: [PATCH 2/2] builtin/stash: merge index in-core Message-ID: <782fe91251111fbb28359574d860e4a6d2e45fc0.1789853192.git.ben.knoble@gmail.com> In-Reply-To: ``` "git stash apply --index" does a 2-step dance to report index conflicts before carrying out the main unstash: first, attempt to merge the index (and remember the name of the resulting tree). If that succeeds, reset the index and carry on unstashing the working tree, then use the remembered index tree to unstash the index. The "merge the index" step is performed on the actual index by a combination of git-diff-tree(1) and git-apply(1), which incurs an extra cost to git-reset(1) to cleanup. This also introduces an autostash bug when stash.index is true: "git reset" eventually wants to remove_merge_branch_state(), which calls save_autostash() due to a03b55530a (merge: teach --autostash option, 2020-04-07). This can happen from a "git merge --autostash", which itself calls save_autostash(). Operating on the file-system in this way is not re-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH ref [1]. This bug has lurked for a while, but it would have been impossible to trigger without the availability of stash.index to force the autostash apply into index mode. [1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/ 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. 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. Reported-by: Eli Barzilay Helped-by: Phillip Wood Signed-off-by: D. Ben Knoble --- 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 | 76 +++++++++--------------------------------------- merge-ort.c | 3 -- t/t7600-merge.sh | 9 ++++++ 3 files changed, 23 insertions(+), 65 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index dfea2d2c4c..9fc1a25e3d 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id, return ret; } -static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit) -{ - struct child_process cp = CHILD_PROCESS_INIT; - const char *w_commit_hex = oid_to_hex(w_commit); - - /* - * Diff-tree would not be very hard to replace with a native function, - * however it should be done together with apply_cached. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL); - strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex); - - return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); -} - -static int apply_cached(struct strbuf *out) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Apply currently only reads either from stdin or a file, thus - * apply_all_patches would have to be updated to optionally take a - * buffer. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "apply", "--cached", NULL); - return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0); -} - -static int reset_head(void) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Reset is overall quite simple, however there is no current public - * API for resetting. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL); - - return run_command(&cp); -} - static int is_path_a_directory(const char *path) { /* @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix, oideq(&c_tree, &info->i_tree)) { has_index = 0; } else { - struct strbuf out = STRBUF_INIT; + struct merge_result result = { 0 }; - if (diff_tree_binary(&out, &info->w_commit)) { - strbuf_release(&out); - return error(_("could not generate diff %s^!."), - oid_to_hex(&info->w_commit)); - } + init_basic_merge_options(&o, the_repository); - 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); + + merge_incore_nonrecursive(&o, head, merge, merge_base, + &result); + + if (!result.clean) return error(_("conflicts in index. " "Try without --index.")); - discard_index(the_repository->index); - repo_read_index(the_repository); - if (write_index_as_tree(&index_tree, the_repository->index, - repo_get_index_file(the_repository), 0, NULL)) - return error(_("could not save index tree")); - - reset_head(); - discard_index(the_repository->index); - repo_read_index(the_repository); + oidcpy(&index_tree, &result.tree->object.oid); + clear_merge_options(&o); } } diff --git a/merge-ort.c b/merge-ort.c index c410a5d353..f69a49d48a 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -5035,8 +5035,6 @@ 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); @@ -5409,7 +5407,6 @@ 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); /* diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index 64fe21717d..8f6109fb91 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -801,6 +801,15 @@ verify_no_mergehead () { test_cmp result.1-5 file ' +test_expect_success 'fast-forward merge with --autostash, stash.index' ' + git reset --hard c0 && + git stash clear && + echo staged >>z && git add z && + git -c stash.index=true merge --autostash c1 2>err && + test_grep "Applied autostash." err && + test_stdout_line_count = 0 git stash list +' + test_expect_success 'failed fast-forward merge with --autostash' ' git reset --hard c0 && git merge-file file file.orig file.5 && -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-19 21:32 Subject: Re: [PATCH 0/2] Hi all, Message-ID: In-Reply-To: ``` My apologies for the strange subject; a little mishap when editing the branch description (I forgot the first line was special). ``` ## Phillip Wood, 2026-09-21 13:17 Subject: Re: [PATCH 2/2] builtin/stash: merge index in-core Message-ID: <2551b801-4cb3-4880-ac01-7d14a188ddd4@gmail.com> In-Reply-To: <782fe91251111fbb28359574d860e4a6d2e45fc0.1789853192.git.ben.knoble@gmail.com> ``` Hi Ben On 19/09/2026 22:26, D. Ben Knoble wrote: > "git stash apply --index" does a 2-step dance to report index conflicts > before carrying out the main unstash: first, attempt to merge the index > (and remember the name of the resulting tree). If that succeeds, reset > the index and carry on unstashing the working tree, then use the > remembered index tree to unstash the index. > > The "merge the index" step is performed on the actual index by a > combination of git-diff-tree(1) and git-apply(1), which incurs an extra > cost to git-reset(1) to cleanup. This also introduces an autostash bug > when stash.index is true: "git reset" eventually wants to > remove_merge_branch_state(), which calls save_autostash() due to > a03b55530a (merge: teach --autostash option, 2020-04-07). This can > happen from a "git merge --autostash", which itself calls > save_autostash(). Operating on the file-system in this way is not > re-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH > ref [1]. This bug has lurked for a while, but it would have been > impossible to trigger without the availability of stash.index to force > the autostash apply into index mode. > > [1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/ > > 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. When the merge succeeds without conflicts we use the result so it is seen. It would be clearer to say that "If there are conflicts we discard the result so ...". The rest of the commit message explains the problem nicely. > 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. Definitely > builtin/stash.c | 76 +++++++++--------------------------------------- Nice diffstat! > @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix, > oideq(&c_tree, &info->i_tree)) { > has_index = 0; > } else { > - struct strbuf out = STRBUF_INIT; > + struct merge_result result = { 0 }; > > - if (diff_tree_binary(&out, &info->w_commit)) { > - strbuf_release(&out); > - return error(_("could not generate diff %s^!."), > - oid_to_hex(&info->w_commit)); > - } > + init_basic_merge_options(&o, the_repository); This means we potentially use different diff algorithms when merging the index and when merging the work tree, let's use the _ui variant here instead. > > - ret = apply_cached(&out); > - strbuf_release(&out); > - if (ret) > + o.verbosity = 0; Looking at the code in merge-ort.c it appears the verbosity option was used by the recursive strategy but isn't used anymore so I think we could drop this. > + > + head = lookup_tree(o.repo, &c_tree); > + merge = lookup_tree(o.repo, &info->i_tree); > + merge_base = lookup_tree(o.repo, &info->b_tree); > + > + merge_incore_nonrecursive(&o, head, merge, merge_base, > + &result); > + > + if (!result.clean) > return error(_("conflicts in index. " > "Try without --index.")); > > - discard_index(the_repository->index); > - repo_read_index(the_repository); > - if (write_index_as_tree(&index_tree, the_repository->index, > - repo_get_index_file(the_repository), 0, NULL)) > - return error(_("could not save index tree")); > - > - reset_head(); > - discard_index(the_repository->index); > - repo_read_index(the_repository); > + oidcpy(&index_tree, &result.tree->object.oid); > + clear_merge_options(&o); Looking at replay.c:replay_revisions() I think this should be merge_finalize(&opts, &result); > } > } > > diff --git a/merge-ort.c b/merge-ort.c > index c410a5d353..f69a49d48a 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -5035,8 +5035,6 @@ 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); This, and the hunk below, make me nervous. Normally assertions like this exist because the pointers are unconditionally dereferenced later on. Looking at merge_3way() it asserts opt->ancestor is non-NULL and dereferences all three labels. t3903 does not appear to have test coverage for the index merge failing (if it did I think we'd see a SIGSEV), we should probably add a test that checks the command fails leaving the index and work tree untouched, and verifies the message on stderr. Lets set some simple, fixed, ancestor and branch names in do_apply_stash() above. > assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE && > opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE); > assert(opt->rename_limit >= -1); > @@ -5409,7 +5407,6 @@ 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); > /* > +test_expect_success 'fast-forward merge with --autostash, stash.index' ' > + git reset --hard c0 && > + git stash clear && > + echo staged >>z && git add z && > + git -c stash.index=true merge --autostash c1 2>err && > + test_grep "Applied autostash." err && > + test_stdout_line_count = 0 git stash list > +' We check the autostash is applied and is not saved - good Thanks for working on this, it is really good to get rid of those subprocesses. Phillip > test_expect_success 'failed fast-forward merge with --autostash' ' > git reset --hard c0 && > git merge-file file file.orig file.5 && ``` ## Junio C Hamano, 2026-09-21 15:10 Subject: Re: [PATCH 1/2] builtin/stash: remove unused header Message-ID: In-Reply-To: ``` "D. Ben Knoble" writes: > Clang complains that oid-array.h is unused. Certainly none of the > oid_array* functions, types, etc., are used, and the > transitively-included hash.h declarations are used but covered by a > pre-existing direct #include of hash.h. Good thing to make sure. And the correctness of the patch can easily be validated, which makes this kind of patch no-brainer to accept ;-) Thanks. > > Signed-off-by: D. Ben Knoble > --- > builtin/stash.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/builtin/stash.c b/builtin/stash.c > index 7a9843413b..dfea2d2c4c 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -31,7 +31,6 @@ > #include "reflog.h" > #include "reflog-walk.h" > #include "add-interactive.h" > -#include "oid-array.h" > #include "commit.h" > > #define INCLUDE_ALL_FILES 2 ``` ## D. Ben Knoble, 2026-09-22 12:43 Subject: Re: [PATCH 2/2] builtin/stash: merge index in-core Message-ID: In-Reply-To: <2551b801-4cb3-4880-ac01-7d14a188ddd4@gmail.com> ``` On Mon, Sep 21, 2026 at 9:17 AM Phillip Wood wrote: > > Hi Ben > > On 19/09/2026 22:26, D. Ben Knoble wrote: > > 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. > > When the merge succeeds without conflicts we use the result so it is > seen. It would be clearer to say that "If there are conflicts we discard > the result so ...". The rest of the commit message explains the problem > nicely. Indeed. This is what I get for (unusually) dashing off the commit message up against the clock. Thanks! > > @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix, > > oideq(&c_tree, &info->i_tree)) { > > has_index = 0; > > } else { > > - struct strbuf out = STRBUF_INIT; > > + struct merge_result result = { 0 }; > > > > - if (diff_tree_binary(&out, &info->w_commit)) { > > - strbuf_release(&out); > > - return error(_("could not generate diff %s^!."), > > - oid_to_hex(&info->w_commit)); > > - } > > + init_basic_merge_options(&o, the_repository); > > This means we potentially use different diff algorithms when merging the > index and when merging the work tree, let's use the _ui variant here > instead. Yep, you know I'd spotted that and wasn't expecting it to make a meaningful difference. It's an easy swap, but I thought that (like above, since we don't show the conflict results) the diff algorithm wouldn't matter too much. Maybe it affects the actual merge-ability, though, in which case I agree using the same is important? > > + o.verbosity = 0; > > Looking at the code in merge-ort.c it appears the verbosity option was > used by the recursive strategy but isn't used anymore so I think we > could drop this. Intriguing. (Assuming the default "2") There's a "< 5" check in path_msg() that wouldn't be affected by dropping this, and a "> 2" check in checkout() that… also wouldn't be affected? But it might matter if something is setting the verbosity elsewhere (config, GIT_MERGE_VERBOSITY), and I think we really want this merge to be quiet? I seem to remember reading commits in this area quieting "git reset" and so on to keep the noise down. So I'm inclined to leave it for now, especially in case it later does get used. > > + oidcpy(&index_tree, &result.tree->object.oid); > > + clear_merge_options(&o); > > Looking at replay.c:replay_revisions() I think this should be > > merge_finalize(&opts, &result); Hm, possibly. It does look like that does more with the "result," which is probably needed. But it doesn't actually clear the merge options. On one hand, I thought it could be important not to reuse that struct between merges. But if we do use the "ui" init, it might be ok? replay_revisions() does use the same struct between calls to merge_incore_nonrecursive(). Oh, but one other thing: we unconditionally reinit the merge options later on in do_apply_stash(). We could conditionally initialize there ("if (has_index)"), I suppose? > > diff --git a/merge-ort.c b/merge-ort.c > > index c410a5d353..f69a49d48a 100644 > > --- a/merge-ort.c > > +++ b/merge-ort.c > > @@ -5035,8 +5035,6 @@ 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); > > This, and the hunk below, make me nervous. Normally assertions like this > exist because the pointers are unconditionally dereferenced later on. > Looking at merge_3way() it asserts opt->ancestor is non-NULL and > dereferences all three labels. t3903 does not appear to have test > coverage for the index merge failing (if it did I think we'd see a > SIGSEV), we should probably add a test that checks the command fails > leaving the index and work tree untouched, and verifies the message on > stderr. > > Lets set some simple, fixed, ancestor and branch names in > do_apply_stash() above. Funny, I was getting aborts before removing the asserts because I hadn't set the labels, aha. Looks like we've come back around to keeping the labels. I'll probably keep a similar structure as the working tree merge uses, I think. A fail-to-merge test also seems like a good idea. Let me mull on that. Thanks for the review. -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-22 12:51 Subject: Re: [PATCH 2/2] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` On Tue, Sep 22, 2026 at 8:43 AM D. Ben Knoble wrote: > > Oh, but one other thing: we unconditionally reinit the merge options > later on in do_apply_stash(). We could conditionally initialize there > ("if (has_index)"), I suppose? er, "!has_index" of course (which is what I originally typed and then, confused, edited). -- D. Ben Knoble ``` ## Phillip Wood, 2026-09-22 13:57 Subject: Re: [PATCH 2/2] builtin/stash: merge index in-core Message-ID: <41d28f9d-b86a-4d65-9a85-656ea9d216e9@gmail.com> In-Reply-To: ``` Hi Ben On 22/09/2026 13:43, D. Ben Knoble wrote: > On Mon, Sep 21, 2026 at 9:17 AM Phillip Wood wrote: >> On 19/09/2026 22:26, D. Ben Knoble wrote: >>> 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. >> >> When the merge succeeds without conflicts we use the result so it is >> seen. It would be clearer to say that "If there are conflicts we discard >> the result so ...". The rest of the commit message explains the problem >> nicely. > > Indeed. This is what I get for (unusually) dashing off the commit > message up against the clock. Thanks! > >>> @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix, >>> oideq(&c_tree, &info->i_tree)) { >>> has_index = 0; >>> } else { >>> - struct strbuf out = STRBUF_INIT; >>> + struct merge_result result = { 0 }; >>> >>> - if (diff_tree_binary(&out, &info->w_commit)) { >>> - strbuf_release(&out); >>> - return error(_("could not generate diff %s^!."), >>> - oid_to_hex(&info->w_commit)); >>> - } >>> + init_basic_merge_options(&o, the_repository); >> >> This means we potentially use different diff algorithms when merging the >> index and when merging the work tree, let's use the _ui variant here >> instead. > > Yep, you know I'd spotted that and wasn't expecting it to make a > meaningful difference. It's an easy swap, but I thought that (like > above, since we don't show the conflict results) the diff algorithm > wouldn't matter too much. > > Maybe it affects the actual merge-ability, though, in which case I > agree using the same is important? I think there are wierd cases where one diff algorithm results in conflicts and another doesn't because they generate different (but equally valid) diffs so allowing the user to tweak the algorithm we use via init_ui_merge_options() is probably a good idea. > >>> + o.verbosity = 0; >> >> Looking at the code in merge-ort.c it appears the verbosity option was >> used by the recursive strategy but isn't used anymore so I think we >> could drop this. > > Intriguing. (Assuming the default "2") There's a "< 5" check in > path_msg() that wouldn't be affected by dropping this, and a "> 2" > check in checkout() that… also wouldn't be affected? The former is not affected because we're cherry-picking so never have an inner merge from merging multiple merge bases. The latter is not affected because we don't checkout the result! > But it might matter if something is setting the verbosity elsewhere > (config, GIT_MERGE_VERBOSITY), and I think we really want this merge > to be quiet? I seem to remember reading commits in this area quieting > "git reset" and so on to keep the noise down. > > So I'm inclined to leave it for now, especially in case it later does get used. merge ort does not print anything - it just adds messages to an strmap in struct merge_result() which we ignore here. I guess setting it to zero might avoid a little work generating the messages. > >>> + oidcpy(&index_tree, &result.tree->object.oid); >>> + clear_merge_options(&o); >> >> Looking at replay.c:replay_revisions() I think this should be >> >> merge_finalize(&opts, &result); > > Hm, possibly. It does look like that does more with the "result," > which is probably needed. Oh, we definitely want to free the strmap in the merge result. > But it doesn't actually clear the merge options. Isn't that because there are no allocations in that struct? (obuf is unused - it looks like we could clean up the struct by removing the members that were used by merge-recursive but are ignored by merge-ort) > On one hand, I thought it could be important not to reuse that struct > between merges. But if we do use the "ui" init, it might be ok? > replay_revisions() does use the same struct between calls to > merge_incore_nonrecursive(). > > Oh, but one other thing: we unconditionally reinit the merge options > later on in do_apply_stash(). We could conditionally initialize there > ("if (has_index)"), I suppose? I'd just move the call to init_ui_merge_options() above "if (index)". As far as I know it should be fine to reuse it - any state is stored in the result > >>> diff --git a/merge-ort.c b/merge-ort.c >>> index c410a5d353..f69a49d48a 100644 >>> --- a/merge-ort.c >>> +++ b/merge-ort.c >>> @@ -5035,8 +5035,6 @@ 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); >> >> This, and the hunk below, make me nervous. Normally assertions like this >> exist because the pointers are unconditionally dereferenced later on. >> Looking at merge_3way() it asserts opt->ancestor is non-NULL and >> dereferences all three labels. t3903 does not appear to have test >> coverage for the index merge failing (if it did I think we'd see a >> SIGSEV), we should probably add a test that checks the command fails >> leaving the index and work tree untouched, and verifies the message on >> stderr. >> >> Lets set some simple, fixed, ancestor and branch names in >> do_apply_stash() above. > > Funny, I was getting aborts before removing the asserts because I > hadn't set the labels, aha. Looks like we've come back around to > keeping the labels. Sorry for that detour > I'll probably keep a similar structure as the > working tree merge uses, I think. I'd use fixed names and not bother with all the conditionals around the label text to keep it simple. > A fail-to-merge test also seems like a good idea. Let me mull on that. That's great Thanks Phillip > Thanks for the review. > ``` ## D. Ben Knoble, 2026-09-22 20:34 Subject: Re: [PATCH 2/2] builtin/stash: merge index in-core Message-ID: In-Reply-To: <41d28f9d-b86a-4d65-9a85-656ea9d216e9@gmail.com> ``` Thanks again, Philip :) On Tue, Sep 22, 2026 at 9:57 AM Phillip Wood wrote: > > Hi Ben > > On 22/09/2026 13:43, D. Ben Knoble wrote: > I think there are wierd cases where one diff algorithm results in > conflicts and another doesn't because they generate different (but > equally valid) diffs so allowing the user to tweak the algorithm we use > via init_ui_merge_options() is probably a good idea. Gotcha; I've already queued this locally. > >>> + o.verbosity = 0; > >> > >> Looking at the code in merge-ort.c it appears the verbosity option was > >> used by the recursive strategy but isn't used anymore so I think we > >> could drop this. > > > > Intriguing. (Assuming the default "2") There's a "< 5" check in > > path_msg() that wouldn't be affected by dropping this, and a "> 2" > > check in checkout() that… also wouldn't be affected? > > The former is not affected because we're cherry-picking so never have an > inner merge from merging multiple merge bases. The latter is not > affected because we don't checkout the result! That's very helpful; I find it challenging right now to navigate the various call-graphs here :) > > But it might matter if something is setting the verbosity elsewhere > > (config, GIT_MERGE_VERBOSITY), and I think we really want this merge > > to be quiet? I seem to remember reading commits in this area quieting > > "git reset" and so on to keep the noise down. > > > > So I'm inclined to leave it for now, especially in case it later does get used. > > merge ort does not print anything - it just adds messages to an strmap > in struct merge_result() which we ignore here. I guess setting it to > zero might avoid a little work generating the messages. Possibly! I still think it signals our intent to be quiet better this way, too. > >>> + oidcpy(&index_tree, &result.tree->object.oid); > >>> + clear_merge_options(&o); > >> > >> Looking at replay.c:replay_revisions() I think this should be > >> > >> merge_finalize(&opts, &result); > > > > Hm, possibly. It does look like that does more with the "result," > > which is probably needed. > > Oh, we definitely want to free the strmap in the merge result. > > > But it doesn't actually clear the merge options. > > Isn't that because there are no allocations in that struct? (obuf is > unused - it looks like we could clean up the struct by removing the > members that were used by merge-recursive but are ignored by merge-ort) Maybe---I was more worried about un-reusable state, but it's true that the clear function is a no-op right now, heh. So it was a bit of "in case one day this is mandatory," perhaps. > > On one hand, I thought it could be important not to reuse that struct > > between merges. But if we do use the "ui" init, it might be ok? > > replay_revisions() does use the same struct between calls to > > merge_incore_nonrecursive(). > > > > Oh, but one other thing: we unconditionally reinit the merge options > > later on in do_apply_stash(). We could conditionally initialize there > > ("if (has_index)"), I suppose? > > I'd just move the call to init_ui_merge_options() above "if (index)". As > far as I know it should be fine to reuse it - any state is stored in the > result Yeah, that's smarter. Locally I got tripped by the case where we said --index but skip some work; but it should be fine to unconditionally initialize those options earlier. > > Funny, I was getting aborts before removing the asserts because I > > hadn't set the labels, aha. Looks like we've come back around to > > keeping the labels. > > Sorry for that detour No worries. > > I'll probably keep a similar structure as the > > working tree merge uses, I think. > > I'd use fixed names and not bother with all the conditionals around the > label text to keep it simple. That's what I ended up with locally, yeah. I finally decided it was too complicated to do anything else for labels that would be really hard to find. I'll get v2 out in the morning, probably. -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-23 12:58 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 ``` ## D. Ben Knoble, 2026-09-23 12:58 Subject: [PATCH v2 1/4] builtin/stash: remove unused header Message-ID: In-Reply-To: ``` Clang complains that oid-array.h is unused. Certainly none of the oid_array* functions, types, etc., are used, and the transitively-included hash.h declarations are used but covered by a pre-existing direct #include of hash.h. Signed-off-by: D. Ben Knoble --- builtin/stash.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index 7a9843413b..dfea2d2c4c 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -31,7 +31,6 @@ #include "reflog.h" #include "reflog-walk.h" #include "add-interactive.h" -#include "oid-array.h" #include "commit.h" #define INCLUDE_ALL_FILES 2 -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-23 12:58 Subject: [PATCH v2 2/4] stash: prepare merge options earlier Message-ID: <1e2343c7fcb17137389d740701336f6c885ba928.1790168285.git.ben.knoble@gmail.com> In-Reply-To: ``` In a future commit, we will reuse these options for the index merge of "apply --index", not just for the worktree. Signed-off-by: D. Ben Knoble --- builtin/stash.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index dfea2d2c4c..043a38cc6d 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -664,6 +664,8 @@ static enum stash_apply_result do_apply_stash(const char *prefix, repo_get_index_file(the_repository), 0, NULL)) return error(_("cannot apply a stash in the middle of a merge")); + init_ui_merge_options(&o, the_repository); + if (index) { if (oideq(&info->b_tree, &info->i_tree) || oideq(&c_tree, &info->i_tree)) { @@ -695,8 +697,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix, } } - init_ui_merge_options(&o, the_repository); - o.branch1 = label_ours ? label_ours : "Updated upstream"; o.branch2 = label_theirs ? label_theirs : "Stashed changes"; o.ancestor = label_base ? label_base : "Stash base"; -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-23 12:58 Subject: [PATCH v2 3/4] t: test failed "stash apply --index" Message-ID: <5bd4b78cace8ba8c8887c78f739bde3513dfda28.1790168285.git.ben.knoble@gmail.com> In-Reply-To: ``` The next commit will refactor index handling for applied stashes, so let's make sure we cover conflicted index merging, too. Signed-off-by: D. Ben Knoble --- t/t3903-stash.sh | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 721158606f..3958ab3c8d 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -374,6 +374,24 @@ setup_stash() { test_cmp expect actual ' +test_expect_success 'stash apply --index leaves everything untouched on failure' ' + git reset --hard && + echo test >other-file && + git add other-file && + git stash && + echo unrelated >file && + echo unrelated >another-file && + git add another-file && + git diff-files >expect && + + echo conflict >other-file && + git add other-file && + test_must_fail git stash apply --index 2>err && + test_grep "conflicts in index. Try without --index" err && + git diff-files >actual && + test_cmp expect actual +' + test_expect_success 'stash -k' ' echo bar3 >file && echo bar4 >file2 && -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-23 12:58 Subject: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "git stash apply --index" does a 2-step dance to report index conflicts before carrying out the main unstash: first, attempt to merge the index (and remember the name of the resulting tree). If that succeeds, reset the index and carry on unstashing the working tree, then use the remembered index tree to unstash the index. The "merge the index" step is performed on the actual index by a combination of git-diff-tree(1) and git-apply(1), which incurs an extra cost to git-reset(1) to cleanup. This also introduces an autostash bug when stash.index is true: "git reset" eventually wants to remove_merge_branch_state(), which calls save_autostash() due to a03b55530a (merge: teach --autostash option, 2020-04-07). This can happen from a "git merge --autostash", which itself calls save_autostash(). Operating on the file-system in this way is not re-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH ref [1]. This bug has lurked for a while, but it would have been impossible to trigger without the availability of stash.index to force the autostash apply into index mode. [1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/ 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. 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 prefer to clean up these vestiges of the shell-based git-stash, though. Reported-by: Eli Barzilay Helped-by: Phillip Wood Signed-off-by: D. Ben Knoble --- builtin/stash.c | 78 ++++++++++-------------------------------------- t/t7600-merge.sh | 9 ++++++ 2 files changed, 25 insertions(+), 62 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index 043a38cc6d..219ca457be 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id, return ret; } -static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit) -{ - struct child_process cp = CHILD_PROCESS_INIT; - const char *w_commit_hex = oid_to_hex(w_commit); - - /* - * Diff-tree would not be very hard to replace with a native function, - * however it should be done together with apply_cached. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL); - strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex); - - return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); -} - -static int apply_cached(struct strbuf *out) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Apply currently only reads either from stdin or a file, thus - * apply_all_patches would have to be updated to optionally take a - * buffer. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "apply", "--cached", NULL); - return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0); -} - -static int reset_head(void) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Reset is overall quite simple, however there is no current public - * API for resetting. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL); - - return run_command(&cp); -} - static int is_path_a_directory(const char *path) { /* @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix, oideq(&c_tree, &info->i_tree)) { has_index = 0; } else { - struct strbuf out = STRBUF_INIT; + struct merge_result result = { 0 }; - if (diff_tree_binary(&out, &info->w_commit)) { - strbuf_release(&out); - return error(_("could not generate diff %s^!."), - oid_to_hex(&info->w_commit)); - } + o.branch1 = "Upstream index"; + o.branch2 = "Stashed index changes"; + o.ancestor = "Stash base"; - 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); + + merge_incore_nonrecursive(&o, head, merge, merge_base, + &result); + + if (!result.clean) return error(_("conflicts in index. " "Try without --index.")); - discard_index(the_repository->index); - repo_read_index(the_repository); - if (write_index_as_tree(&index_tree, the_repository->index, - repo_get_index_file(the_repository), 0, NULL)) - return error(_("could not save index tree")); - - reset_head(); - discard_index(the_repository->index); - repo_read_index(the_repository); + oidcpy(&index_tree, &result.tree->object.oid); + merge_finalize(&o, &result); } } diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index 64fe21717d..8f6109fb91 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -801,6 +801,15 @@ verify_no_mergehead () { test_cmp result.1-5 file ' +test_expect_success 'fast-forward merge with --autostash, stash.index' ' + git reset --hard c0 && + git stash clear && + echo staged >>z && git add z && + git -c stash.index=true merge --autostash c1 2>err && + test_grep "Applied autostash." err && + test_stdout_line_count = 0 git stash list +' + test_expect_success 'failed fast-forward merge with --autostash' ' git reset --hard c0 && git merge-file file file.orig file.5 && -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## Phillip Wood, 2026-09-24 09:42 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: <68e83baa-6ccb-4ca8-a1df-f09d51749c67@gmail.com> In-Reply-To: ``` Hi Ben I've spotted a memory leak that I missed last time, apart from that this looks good. On 23/09/2026 13:58, D. Ben Knoble wrote: > @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix, > oideq(&c_tree, &info->i_tree)) { > has_index = 0; > } else { > - struct strbuf out = STRBUF_INIT; > + struct merge_result result = { 0 }; > > - if (diff_tree_binary(&out, &info->w_commit)) { > - strbuf_release(&out); > - return error(_("could not generate diff %s^!."), > - oid_to_hex(&info->w_commit)); > - } > + o.branch1 = "Upstream index"; This is the current index, calling it "upstream" is a bit confusing to me but that's not worth a re-roll on its own. > + o.branch2 = "Stashed index changes"; > + o.ancestor = "Stash base"; > > - 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); > + > + merge_incore_nonrecursive(&o, head, merge, merge_base, > + &result); > + > + if (!result.clean) > return error(_("conflicts in index. " > "Try without --index.")); Sorry, I missed this last time, but we should finalize the merge before returning to ensure the allocations in result are freed. Everything else looks fine. Thanks Phillip > - discard_index(the_repository->index); > - repo_read_index(the_repository); > - if (write_index_as_tree(&index_tree, the_repository->index, > - repo_get_index_file(the_repository), 0, NULL)) > - return error(_("could not save index tree")); > - > - reset_head(); > - discard_index(the_repository->index); > - repo_read_index(the_repository); > + oidcpy(&index_tree, &result.tree->object.oid); > + merge_finalize(&o, &result); > } > } > > diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh > index 64fe21717d..8f6109fb91 100755 > --- a/t/t7600-merge.sh > +++ b/t/t7600-merge.sh > @@ -801,6 +801,15 @@ verify_no_mergehead () { > test_cmp result.1-5 file > ' > > +test_expect_success 'fast-forward merge with --autostash, stash.index' ' > + git reset --hard c0 && > + git stash clear && > + echo staged >>z && git add z && > + git -c stash.index=true merge --autostash c1 2>err && > + test_grep "Applied autostash." err && > + test_stdout_line_count = 0 git stash list > +' > + > test_expect_success 'failed fast-forward merge with --autostash' ' > git reset --hard c0 && > git merge-file file file.orig file.5 && ``` ## Phillip Wood, 2026-09-24 09:42 Subject: Re: [PATCH v2 3/4] t: test failed "stash apply --index" Message-ID: <232f2bf6-04d8-4a54-b4e9-51b5ee79799f@gmail.com> In-Reply-To: <5bd4b78cace8ba8c8887c78f739bde3513dfda28.1790168285.git.ben.knoble@gmail.com> ``` Hi Ben On 23/09/2026 13:58, D. Ben Knoble wrote: > The next commit will refactor index handling for applied stashes, so > let's make sure we cover conflicted index merging, too. > > Signed-off-by: D. Ben Knoble > --- > t/t3903-stash.sh | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 721158606f..3958ab3c8d 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -374,6 +374,24 @@ setup_stash() { > test_cmp expect actual > ' > > +test_expect_success 'stash apply --index leaves everything untouched on failure' ' > + git reset --hard && > + echo test >other-file && > + git add other-file && > + git stash && > + echo unrelated >file && > + echo unrelated >another-file && > + git add another-file && > + git diff-files >expect && diff-files shows the worktree blobs as null object ids, so comparing this before and after stashing only tells us that the same set of files have unstaged changes, not that the unstaged changes are the same. Adding "-p" would check the worktree files are unchanged. > + echo conflict >other-file && > + git add other-file && I wonder if we should to add "git diff-index --cached HEAD >expect-index" here so we can check the index is unchanged as well. For the paths that have unstaged changes we're already checking the index object ids via "diff-files", but I think in theory it would be possible to have an identical change in the index and worktree that is not picked up by that. Thanks for adding this test, it is a useful improvement in our coverage. Phillip > + test_must_fail git stash apply --index 2>err && > + test_grep "conflicts in index. Try without --index" err && > + git diff-files >actual && > + test_cmp expect actual > +' > + > test_expect_success 'stash -k' ' > echo bar3 >file && > echo bar4 >file2 && ``` ## Junio C Hamano, 2026-09-24 21:59 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "D. Ben Knoble" writes: > @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix, > oideq(&c_tree, &info->i_tree)) { > has_index = 0; > } else { > - struct strbuf out = STRBUF_INIT; > + struct merge_result result = { 0 }; > > - if (diff_tree_binary(&out, &info->w_commit)) { > - strbuf_release(&out); > - return error(_("could not generate diff %s^!."), > - oid_to_hex(&info->w_commit)); > - } > + o.branch1 = "Upstream index"; > + o.branch2 = "Stashed index changes"; > + o.ancestor = "Stash base"; > > - ret = apply_cached(&out); > - strbuf_release(&out); > - if (ret) So, we used to take a diff between w_commit^2^ and w_commit^2 and then give the resulting patch to "apply --cached". w_commit is the working tree state, w_commit^1 is the HEAD (i.e. b_tree) when the stash was created (i.e., "diff HEAD w_commit" is the change in the working tree), w_commit^2 is the contents of the index (i.e. i_tree), so we are computing a patch that represents what "diff --cached HEAD" would have shown when we created the stash. And the goal is to reflect this change on the current HEAD to recreate the "staged" changes in the current index. IOW, we want to three-way merge the change that moves you from b_tree to i_tree into c_tree. > + 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); > + > + merge_incore_nonrecursive(&o, head, merge, merge_base, > + &result); We are using the merge machinery to perform a cherry-pick of the changes to go from b_tree to i_tree into c_tree. but the three trees involved in this cherry-pick is named unnecessarily confusingly. merge-incore-nonrecursive() takes the common ancestor ("merge_base") and two sides ("side1" and "side2") in this order. It takes the changes to go from common to side1 and computes the result of updating the remainder (side2) with such a change (or vice versa; a merge is symmetric). When cherry-picking, the first tree would be the b_tree, the second tree would be the i_tree, and the target tree would be the c_tree. - b_tree serves as merge_base - i_tree serves as side1 - c_tree serves as side2 Am I following what the code should be doing correctly? I am wondering if the order of the tree trees in the merge_incore_nonrecursive() call is correct. Shouldn't it be merge_incore_nonrecursive(&o, merge_base, merge, head, &result); (or merge and head swapped) if we want to update head (I would call it side2) in such a way that the change to go from it to the resulting tree is similar to the change between merge_base (b_tree) and merge (i_tree)? Ahh, or perhaps the trees are indeed given in a wrong order, but not in a random wrong order. merge_ort_nonrecursive(), which is *not* the function you are using, takes head, merge, and merge_base in this order, and that order matches what you wrote. Perhaps the true culprit in this confusion is that the order in which merge_ort_nonrecursive() takes its three trees (head, merge, and common) and the order in which merge_incore_nonrecursive() takes its trees (merge_base, side1, and side2) are different, and if we fix them to match, it would make it easier to work with? The new test in the attached patch will fail with this step but if we revert the changes to builtin/stash.c in this step, it passes. t/t3903-stash.sh | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh index 3958ab3c8d..0a87e62b11 100755 --- c/t/t3903-stash.sh +++ w/t/t3903-stash.sh @@ -374,6 +374,38 @@ test_expect_success 'stash apply -q --index refreshes the index' ' test_cmp expect actual ' + +test_expect_success 'stash apply --index does not revert unrelated upstream index changes' ' + test_when_finished "rm -fr playpen" && + mkdir playpen && + ( + cd playpen && + git init && + echo "base1" >file1 && + echo "base2" >file2 && + git add file1 file2 && + git commit -m "initial base" && + + # Make a staged change to file1 and stash it + echo "staged1" >file1 && + git add file1 && + git stash && + + # Upstream advances by modifying unrelated file2 + echo "upstream2" >file2 && + git add file2 && + git commit -m "upstream change to file2" && + + # Apply the stash with --index + git stash apply --index && + + # Verify working tree and index state + test "$(git show :file1)" = "staged1" && + test "$(git show :file2)" = "upstream2" && + test "$(git show HEAD:file2)" = "upstream2" + ) +' + test_expect_success 'stash apply --index leaves everything untouched on failure' ' git reset --hard && echo test >other-file && ``` ## Junio C Hamano, 2026-09-25 04:12 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` Junio C Hamano writes: > The new test in the attached patch will fail with this step but if > we revert the changes to builtin/stash.c in this step, it passes. Oh, and with the change to the code, it passes again. diff --git i/builtin/stash.c w/builtin/stash.c index 219ca457be..44d962cc5d 100644 --- i/builtin/stash.c +++ w/builtin/stash.c @@ -639,7 +639,7 @@ static enum stash_apply_result do_apply_stash(const char *prefix, merge = lookup_tree(o.repo, &info->i_tree); merge_base = lookup_tree(o.repo, &info->b_tree); - merge_incore_nonrecursive(&o, head, merge, merge_base, + merge_incore_nonrecursive(&o, merge_base, merge, head, &result); if (!result.clean) diff --git i/t/t3903-stash.sh w/t/t3903-stash.sh index 3958ab3c8d..0a87e62b11 100755 --- i/t/t3903-stash.sh +++ w/t/t3903-stash.sh @@ -374,6 +374,38 @@ test_expect_success 'stash apply -q --index refreshes the index' ' test_cmp expect actual ' + +test_expect_success 'stash apply --index does not revert unrelated upstream index changes' ' + test_when_finished "rm -fr playpen" && + mkdir playpen && + ( + cd playpen && + git init && + echo "base1" >file1 && + echo "base2" >file2 && + git add file1 file2 && + git commit -m "initial base" && + + # Make a staged change to file1 and stash it + echo "staged1" >file1 && + git add file1 && + git stash && + + # Upstream advances by modifying unrelated file2 + echo "upstream2" >file2 && + git add file2 && + git commit -m "upstream change to file2" && + + # Apply the stash with --index + git stash apply --index && + + # Verify working tree and index state + test "$(git show :file1)" = "staged1" && + test "$(git show :file2)" = "upstream2" && + test "$(git show HEAD:file2)" = "upstream2" + ) +' + test_expect_success 'stash apply --index leaves everything untouched on failure' ' git reset --hard && echo test >other-file && ``` ## D. Ben Knoble, 2026-09-25 12:55 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: <68e83baa-6ccb-4ca8-a1df-f09d51749c67@gmail.com> ``` Hi Phillip, On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood wrote: > > Hi Ben > > I've spotted a memory leak that I missed last time, apart from that this > looks good. > > On 23/09/2026 13:58, D. Ben Knoble wrote: > > @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix, > > oideq(&c_tree, &info->i_tree)) { > > has_index = 0; > > } else { > > - struct strbuf out = STRBUF_INIT; > > + struct merge_result result = { 0 }; > > > > - if (diff_tree_binary(&out, &info->w_commit)) { > > - strbuf_release(&out); > > - return error(_("could not generate diff %s^!."), > > - oid_to_hex(&info->w_commit)); > > - } > > + o.branch1 = "Upstream index"; > > This is the current index, calling it "upstream" is a bit confusing to > me but that's not worth a re-roll on its own. Will fix. The "upstream" verbiage comes from the working tree labels. > > + o.branch2 = "Stashed index changes"; > > + o.ancestor = "Stash base"; > > > > - 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); > > + > > + merge_incore_nonrecursive(&o, head, merge, merge_base, > > + &result); > > + > > + if (!result.clean) > > return error(_("conflicts in index. " > > "Try without --index.")); > > Sorry, I missed this last time, but we should finalize the merge before > returning to ensure the allocations in result are freed. Yeah, I think CI caught this: https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:5:31 But I'm not sure I could have understood what it was telling me without your hint, thanks! -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-25 13:00 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano wrote: > > Ahh, or perhaps the trees are indeed given in a wrong order, but not > in a random wrong order. merge_ort_nonrecursive(), which is *not* > the function you are using, takes head, merge, and merge_base in > this order, and that order matches what you wrote. > > Perhaps the true culprit in this confusion is that the order in > which merge_ort_nonrecursive() takes its three trees (head, merge, > and common) and the order in which merge_incore_nonrecursive() takes > its trees (merge_base, side1, and side2) are different, and if we > fix them to match, it would make it easier to work with? Indeed, the confusion is that simple ;) Shamefully, we don't have enough test coverage to catch that regression, so I'm very glad indeed you spotted it. > The new test in the attached patch will fail with this step but if > we revert the changes to builtin/stash.c in this step, it passes. Any objection to me adding this test as a preparatory patch? There's no sign-off, so I don't want to mess up the DCO here. ``` ## D. Ben Knoble, 2026-09-25 13:36 Subject: Re: [PATCH v2 3/4] t: test failed "stash apply --index" Message-ID: In-Reply-To: <232f2bf6-04d8-4a54-b4e9-51b5ee79799f@gmail.com> ``` Hi Phillip, On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood wrote: > > Hi Ben > > On 23/09/2026 13:58, D. Ben Knoble wrote: > > The next commit will refactor index handling for applied stashes, so > > let's make sure we cover conflicted index merging, too. > > > > Signed-off-by: D. Ben Knoble > > --- > > t/t3903-stash.sh | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > > index 721158606f..3958ab3c8d 100755 > > --- a/t/t3903-stash.sh > > +++ b/t/t3903-stash.sh > > @@ -374,6 +374,24 @@ setup_stash() { > > test_cmp expect actual > > ' > > > > +test_expect_success 'stash apply --index leaves everything untouched on failure' ' > > + git reset --hard && > > + echo test >other-file && > > + git add other-file && > > + git stash && > > + echo unrelated >file && > > + echo unrelated >another-file && > > + git add another-file && > > + git diff-files >expect && > > diff-files shows the worktree blobs as null object ids, so comparing > this before and after stashing only tells us that the same set of files > have unstaged changes, not that the unstaged changes are the same. > Adding "-p" would check the worktree files are unchanged. I confess I played with diff-files and diff-index manually before trying to construct this test case, and I still don't totally understand how they're being used in the test just prior… Anway, it looks to me like "diff-files -p" is the same as "diff -p" (albeit without some niceties like color-moved applying automatically from config), so that would make the test quite a bit more complicated, no? (The "index $sha1..$sha2" line would change…) Since we know what the expected contents are, perhaps we can simply assert on those. Hm. I spent some time with test_pause in the previous test, and I think my concerns about that line changing are moot. But, asserting on the contents is also a bit silly (as that's what the blob IDs are doing for us in the output). > > + echo conflict >other-file && > > + git add other-file && > > I wonder if we should to add "git diff-index --cached HEAD > >expect-index" here so we can check the index is unchanged as well. For > the paths that have unstaged changes we're already checking the index > object ids via "diff-files", but I think in theory it would be possible > to have an identical change in the index and worktree that is not picked > up by that. So, this test sets up an intermediate state prior to attempting to unstash where - another-file is new in the index & working tree (content: "unrelated") - other-file is modified in the index & working tree (content: from "6" to "conflict") - file is modified in the working tree (content: from "bar" to "unrelated") And we should still be there when finished. (I wonder if, like the previous test, we should have a file that differs from itself in the index and working tree?) So overall, I'm thinking - (old) diff-files only shows file is changed - diff-files -p shows us changes for file, better (and won't show the other 2 files unless they've become unstaged) - diff-index --cached HEAD helps us check all the index changes Phew! Thanks for reading my rambling thinking aloud :) -- D. Ben Knoble ``` ## Phillip Wood, 2026-09-25 15:45 Subject: Re: [PATCH v2 3/4] t: test failed "stash apply --index" Message-ID: In-Reply-To: ``` Hi Ben On 25/09/2026 14:36, D. Ben Knoble wrote: > > So overall, I'm thinking > > - (old) diff-files only shows file is changed > - diff-files -p shows us changes for file, better (and won't show the > other 2 files unless they've become unstaged) > - diff-index --cached HEAD helps us check all the index changes I think that sounds reasonable, we can delete the index lines from the patch output with sed to make it easier to compare them. Thanks Phillip > Phew! Thanks for reading my rambling thinking aloud :) > ``` ## Phillip Wood, 2026-09-25 15:58 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: <36e1073e-fa55-4d7d-8b8b-ba9ac34976fa@gmail.com> In-Reply-To: ``` Hi Ben On 25/09/2026 13:55, D. Ben Knoble wrote: > On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood wrote: >> >> Sorry, I missed this last time, but we should finalize the merge before >> returning to ensure the allocations in result are freed. > > Yeah, I think CI caught this: > https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:5:31 > > But I'm not sure I could have understood what it was telling me > without your hint, thanks! Yes, that output is terrible - to see the leaks you have to scroll to line 28282 of "print test failures" which is ridiculous. See https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:10:28282 Thanks Phillip> ``` ## Phillip Wood, 2026-09-25 16:04 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: <6e6420e8-3cbd-4975-a781-645e1ffbc1d2@gmail.com> In-Reply-To: ``` Hi Junio On 24/09/2026 22:59, Junio C Hamano wrote: > "D. Ben Knoble" writes: > > Ahh, or perhaps the trees are indeed given in a wrong order, but not > in a random wrong order. merge_ort_nonrecursive(), which is *not* > the function you are using, takes head, merge, and merge_base in > this order, and that order matches what you wrote. Ouch that's nasty. Well spotted, I missed it when I read the code (because the arguments were in the same order as the call to merge_ort_nonrecursive()) and the tests we have use the same version of the file for "base" and "stage2" so do not notice if they'd been transposed. It is rather confusing that two functions that are so closely related take their arguments in a different order. > Perhaps the true culprit in this confusion is that the order in > which merge_ort_nonrecursive() takes its three trees (head, merge, > and common) and the order in which merge_incore_nonrecursive() takes > its trees (merge_base, side1, and side2) are different, and if we > fix them to match, it would make it easier to work with? I think it is definitely worth fixing them to take the trees in the same order. My preference would be "base", "stage1", "stage2" but so long as they match each other I dont object to "stage1", "stage2", "base". Thanks Phillip > The new test in the attached patch will fail with this step but if > we revert the changes to builtin/stash.c in this step, it passes. > > t/t3903-stash.sh | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh > index 3958ab3c8d..0a87e62b11 100755 > --- c/t/t3903-stash.sh > +++ w/t/t3903-stash.sh > @@ -374,6 +374,38 @@ test_expect_success 'stash apply -q --index refreshes the index' ' > test_cmp expect actual > ' > > + > +test_expect_success 'stash apply --index does not revert unrelated upstream index changes' ' > + test_when_finished "rm -fr playpen" && > + mkdir playpen && > + ( > + cd playpen && > + git init && > + echo "base1" >file1 && > + echo "base2" >file2 && > + git add file1 file2 && > + git commit -m "initial base" && > + > + # Make a staged change to file1 and stash it > + echo "staged1" >file1 && > + git add file1 && > + git stash && > + > + # Upstream advances by modifying unrelated file2 > + echo "upstream2" >file2 && > + git add file2 && > + git commit -m "upstream change to file2" && > + > + # Apply the stash with --index > + git stash apply --index && > + > + # Verify working tree and index state > + test "$(git show :file1)" = "staged1" && > + test "$(git show :file2)" = "upstream2" && > + test "$(git show HEAD:file2)" = "upstream2" > + ) > +' > + > test_expect_success 'stash apply --index leaves everything untouched on failure' ' > git reset --hard && > echo test >other-file && ``` ## D. Ben Knoble, 2026-09-25 16:16 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: <36e1073e-fa55-4d7d-8b8b-ba9ac34976fa@gmail.com> ``` On Fri, Sep 25, 2026 at 11:58 AM Phillip Wood wrote: > On 25/09/2026 13:55, D. Ben Knoble wrote: > > On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood wrote: > >> > >> Sorry, I missed this last time, but we should finalize the merge before > >> returning to ensure the allocations in result are freed. > > > > Yeah, I think CI caught this: > > https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:5:31 > > > > But I'm not sure I could have understood what it was telling me > > without your hint, thanks! > > Yes, that output is terrible - to see the leaks you have to scroll to > line 28282 of "print test failures" which is ridiculous. See > https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:10:28282 Ah, sorry. My link was sloppy. I did get that far, but the allocation backtrace doesn't make it obvious that merge_result is what leaked, and that's where I was saying an especial thank you ;) -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-25 16:17 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: <6e6420e8-3cbd-4975-a781-645e1ffbc1d2@gmail.com> ``` On Fri, Sep 25, 2026 at 12:04 PM Phillip Wood wrote: > > Hi Junio > > On 24/09/2026 22:59, Junio C Hamano wrote: [snip] > > Perhaps the true culprit in this confusion is that the order in > > which merge_ort_nonrecursive() takes its three trees (head, merge, > > and common) and the order in which merge_incore_nonrecursive() takes > > its trees (merge_base, side1, and side2) are different, and if we > > fix them to match, it would make it easier to work with? > > I think it is definitely worth fixing them to take the trees in the same > order. My preference would be "base", "stage1", "stage2" but so long as > they match each other I dont object to "stage1", "stage2", "base". > > Thanks > > Phillip FWIW, I concur with changing them (and Phillip's preference of order), but I'll elect to leave that out of scope for this series. -- D. Ben Knoble ``` ## Junio C Hamano, 2026-09-25 16:24 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "D. Ben Knoble" writes: > On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano wrote: >> >> Ahh, or perhaps the trees are indeed given in a wrong order, but not >> in a random wrong order. merge_ort_nonrecursive(), which is *not* >> the function you are using, takes head, merge, and merge_base in >> this order, and that order matches what you wrote. >> >> Perhaps the true culprit in this confusion is that the order in >> which merge_ort_nonrecursive() takes its three trees (head, merge, >> and common) and the order in which merge_incore_nonrecursive() takes >> its trees (merge_base, side1, and side2) are different, and if we >> fix them to match, it would make it easier to work with? > > Indeed, the confusion is that simple ;) Shamefully, we don't have > enough test coverage to catch that regression, so I'm very glad indeed > you spotted it. > >> The new test in the attached patch will fail with this step but if >> we revert the changes to builtin/stash.c in this step, it passes. > > Any objection to me adding this test as a preparatory patch? There's > no sign-off, so I don't want to mess up the DCO here. It was written merely as an illustration and is not something I am proud of. For example, creating a totally new playpen repository only for a single piece of test and remove the entire thing when the single test piece is done was done only to make sure the existing test that come later can never be affected. Also the test only uses the most trivial case (a file is added in the stashed change, nobody else involved in the stash application has touched the file so there is nothing to "merge" in the file). It was enough to demonstrate that the order of arguments given to the function was wrong, but we wouldn't catch problems in content-level merge with such a test. So, I wouldn't mind if you reused that as one in a series of tests, but I'd prefer to see those who are move invested in the topic to come up with a bit more realistic scenario. Thanks. ``` ## Junio C Hamano, 2026-09-25 16:49 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "D. Ben Knoble" writes: >> I think it is definitely worth fixing them to take the trees in the same >> order. My preference would be "base", "stage1", "stage2" but so long as >> they match each other I dont object to "stage1", "stage2", "base". >> >> Thanks >> >> Phillip > > FWIW, I concur with changing them (and Phillip's preference of order), > but I'll elect to leave that out of scope for this series. Oh, absolutely it is out of scope for this series. ``` ## Phillip Wood, 2026-09-26 09:51 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` On 25/09/2026 17:24, Junio C Hamano wrote: > "D. Ben Knoble" writes: > >> On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano wrote: >>> >>> Ahh, or perhaps the trees are indeed given in a wrong order, but not >>> in a random wrong order. merge_ort_nonrecursive(), which is *not* >>> the function you are using, takes head, merge, and merge_base in >>> this order, and that order matches what you wrote. >>> >>> Perhaps the true culprit in this confusion is that the order in >>> which merge_ort_nonrecursive() takes its three trees (head, merge, >>> and common) and the order in which merge_incore_nonrecursive() takes >>> its trees (merge_base, side1, and side2) are different, and if we >>> fix them to match, it would make it easier to work with? >> >> Indeed, the confusion is that simple ;) Shamefully, we don't have >> enough test coverage to catch that regression, so I'm very glad indeed >> you spotted it. >> >>> The new test in the attached patch will fail with this step but if >>> we revert the changes to builtin/stash.c in this step, it passes. >> >> Any objection to me adding this test as a preparatory patch? There's >> no sign-off, so I don't want to mess up the DCO here. > > It was written merely as an illustration and is not something I am > proud of. For example, creating a totally new playpen repository > only for a single piece of test and remove the entire thing when the > single test piece is done was done only to make sure the existing > test that come later can never be affected. Also the test only uses > the most trivial case (a file is added in the stashed change, nobody > else involved in the stash application has touched the file so there > is nothing to "merge" in the file). It was enough to demonstrate > that the order of arguments given to the function was wrong, but > we wouldn't catch problems in content-level merge with such a test. > > So, I wouldn't mind if you reused that as one in a series of tests, > but I'd prefer to see those who are move invested in the topic to > come up with a bit more realistic scenario. Maybe something like the test below (which I admit I haven't actually tested). That checks we merge the file contents and puts the changes in the file close enough together so that the old code would fail and has different contents for the three merged blobs. test_write_lines A B C >file && git commit -m xxx 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 yyy file && git stash pop --index && git show :file >actual && test_write_lines committed B staged >expect && text_cmp expect actual && test_write_lines committed B unstaged >expect && test_cmp expect file Thanks Phillip ``` ## Phillip Wood, 2026-09-26 09:53 Subject: Re: [PATCH v2 3/4] t: test failed "stash apply --index" Message-ID: In-Reply-To: ``` On 25/09/2026 16:45, Phillip Wood wrote: > > I think that sounds reasonable, we can delete the index lines from the > patch output with sed to make it easier to compare them. I just opened the test file and realized it has a diff_cmp() function to compare diffs ignoring the index lines Thanks Phillip ``` ## D. Ben Knoble, 2026-09-26 12:04 Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` On Sat, Sep 26, 2026 at 5:51 AM Phillip Wood wrote: > > On 25/09/2026 17:24, Junio C Hamano wrote: > > "D. Ben Knoble" writes: > > > >> On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano wrote: > >>> > >>> Ahh, or perhaps the trees are indeed given in a wrong order, but not > >>> in a random wrong order. merge_ort_nonrecursive(), which is *not* > >>> the function you are using, takes head, merge, and merge_base in > >>> this order, and that order matches what you wrote. > >>> > >>> Perhaps the true culprit in this confusion is that the order in > >>> which merge_ort_nonrecursive() takes its three trees (head, merge, > >>> and common) and the order in which merge_incore_nonrecursive() takes > >>> its trees (merge_base, side1, and side2) are different, and if we > >>> fix them to match, it would make it easier to work with? > >> > >> Indeed, the confusion is that simple ;) Shamefully, we don't have > >> enough test coverage to catch that regression, so I'm very glad indeed > >> you spotted it. > >> > >>> The new test in the attached patch will fail with this step but if > >>> we revert the changes to builtin/stash.c in this step, it passes. > >> > >> Any objection to me adding this test as a preparatory patch? There's > >> no sign-off, so I don't want to mess up the DCO here. > > > > It was written merely as an illustration and is not something I am > > proud of. For example, creating a totally new playpen repository > > only for a single piece of test and remove the entire thing when the > > single test piece is done was done only to make sure the existing > > test that come later can never be affected. Also the test only uses > > the most trivial case (a file is added in the stashed change, nobody > > else involved in the stash application has touched the file so there > > is nothing to "merge" in the file). It was enough to demonstrate > > that the order of arguments given to the function was wrong, but > > we wouldn't catch problems in content-level merge with such a test. > > > > So, I wouldn't mind if you reused that as one in a series of tests, > > but I'd prefer to see those who are move invested in the topic to > > come up with a bit more realistic scenario. > > Maybe something like the test below (which I admit I haven't actually > tested). That checks we merge the file contents and puts the changes in > the file close enough together so that the old code would fail and has > different contents for the three merged blobs. > > test_write_lines A B C >file && > git commit -m xxx 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 yyy file && > git stash pop --index && > git show :file >actual && > test_write_lines committed B staged >expect && > text_cmp expect actual && s/text/test ;) > test_write_lines committed B unstaged >expect && > test_cmp expect file This does fail on the original code (head, base, merge_base) because the index (git show :file) has "A B staged" lines instead of "committed B staged" lines. This test does pass on the new code, but needs some arrangement/cleanup for the later "stash -k" test to succeed, so I'll include that in the next round as well. -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-26 12:07 Subject: Re: [PATCH v2 3/4] t: test failed "stash apply --index" Message-ID: In-Reply-To: ``` On Sat, Sep 26, 2026 at 5:53 AM Phillip Wood wrote: > > On 25/09/2026 16:45, Phillip Wood wrote: > > > > I think that sounds reasonable, we can delete the index lines from the > > patch output with sed to make it easier to compare them. > > I just opened the test file and realized it has a diff_cmp() function to > compare diffs ignoring the index lines > > Thanks > > Phillip Doh! On the other hand, I don't think we need it. Those lines are showing blob IDs, which would be stable in our case (fixed hash algorithm over fixed contents), and we don't make the test dependent on the actual IDs? -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-26 12:16 Subject: [PATCH v3 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 and fourth add missing test coverage (which catch breakages from prior incorrect rounds of this series), while the last holds the interesting bits. 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: [1/5] builtin/stash: remove unused header [2/5] stash: prepare merge options earlier [3/5] t3903: test stash --index merges [4/5] t3903: test failed "stash apply --index" [5/5] builtin/stash: merge index in-core builtin/stash.c | 85 +++++++++++------------------------------------- t/t3903-stash.sh | 42 ++++++++++++++++++++++++ t/t7600-merge.sh | 9 +++++ 3 files changed, 70 insertions(+), 66 deletions(-) Diff-intervalle contre v2 : 1: b6798c8a25 = 1: 6a165c4df4 builtin/stash: remove unused header 2: 1e2343c7fc = 2: d9a9e18f3a stash: prepare merge options earlier -: ---------- > 3: 8b5ea5e6f4 t3903: test stash --index merges 3: 5bd4b78cac ! 4: d39e16905d t: test failed "stash apply --index" @@ Metadata Author: D. Ben Knoble ## Commit message ## - t: test failed "stash apply --index" + t3903: test failed "stash apply --index" The next commit will refactor index handling for applied stashes, so let's make sure we cover conflicted index merging, too. + Helped-by: Phillip Wood + ## t/t3903-stash.sh ## @@ t/t3903-stash.sh: setup_stash() { - test_cmp expect actual + test_cmp expect file ' +test_expect_success 'stash apply --index leaves everything untouched on failure' ' @@ t/t3903-stash.sh: setup_stash() { + echo unrelated >file && + echo unrelated >another-file && + git add another-file && -+ git diff-files >expect && -+ + echo conflict >other-file && + git add other-file && ++ git diff-files -p >expect && ++ git diff-index --cached HEAD >expect-index && ++ + test_must_fail git stash apply --index 2>err && + test_grep "conflicts in index. Try without --index" err && -+ git diff-files >actual && -+ test_cmp expect actual ++ git diff-files -p >actual && ++ test_cmp expect actual && ++ git diff-index --cached HEAD >actual-index && ++ test_cmp expect-index actual-index +' + test_expect_success 'stash -k' ' 4: e49936ee12 ! 5: fde7fb7988 builtin/stash: merge index in-core @@ Commit message Reported-by: Eli Barzilay Helped-by: Phillip Wood + Helped-by: Junio C Hamano ## builtin/stash.c ## @@ builtin/stash.c: static int create_index_from_tree(const struct object_id *tree_id, @@ 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)); - } -+ o.branch1 = "Upstream index"; ++ o.branch1 = "Current index"; + o.branch2 = "Stashed index changes"; + o.ancestor = "Stash base"; @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi + merge = lookup_tree(o.repo, &info->i_tree); + merge_base = lookup_tree(o.repo, &info->b_tree); + -+ merge_incore_nonrecursive(&o, head, merge, merge_base, ++ merge_incore_nonrecursive(&o, merge_base, head, merge, + &result); + ++ oidcpy(&index_tree, &result.tree->object.oid); ++ merge_finalize(&o, &result); ++ + if (!result.clean) return error(_("conflicts in index. " "Try without --index.")); - +- - discard_index(the_repository->index); - repo_read_index(the_repository); - if (write_index_as_tree(&index_tree, the_repository->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); -+ oidcpy(&index_tree, &result.tree->object.oid); -+ merge_finalize(&o, &result); } } base-commit: d38352cd43ab9745686d697872408bc3249a153f -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-26 12:16 Subject: [PATCH v3 1/5] builtin/stash: remove unused header Message-ID: <6a165c4df456b6bd5e5ab46664b023a45e670926.1790425008.git.ben.knoble@gmail.com> In-Reply-To: ``` Clang complains that oid-array.h is unused. Certainly none of the oid_array* functions, types, etc., are used, and the transitively-included hash.h declarations are used but covered by a pre-existing direct #include of hash.h. Signed-off-by: D. Ben Knoble --- builtin/stash.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index 7a9843413b..dfea2d2c4c 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -31,7 +31,6 @@ #include "reflog.h" #include "reflog-walk.h" #include "add-interactive.h" -#include "oid-array.h" #include "commit.h" #define INCLUDE_ALL_FILES 2 -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-26 12:16 Subject: [PATCH v3 2/5] stash: prepare merge options earlier Message-ID: In-Reply-To: ``` In a future commit, we will reuse these options for the index merge of "apply --index", not just for the worktree. Signed-off-by: D. Ben Knoble --- builtin/stash.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index dfea2d2c4c..043a38cc6d 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -664,6 +664,8 @@ static enum stash_apply_result do_apply_stash(const char *prefix, repo_get_index_file(the_repository), 0, NULL)) return error(_("cannot apply a stash in the middle of a merge")); + init_ui_merge_options(&o, the_repository); + if (index) { if (oideq(&info->b_tree, &info->i_tree) || oideq(&c_tree, &info->i_tree)) { @@ -695,8 +697,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix, } } - init_ui_merge_options(&o, the_repository); - o.branch1 = label_ours ? label_ours : "Updated upstream"; o.branch2 = label_theirs ? label_theirs : "Stashed changes"; o.ancestor = label_base ? label_base : "Stash base"; -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-26 12:16 Subject: [PATCH v3 3/5] t3903: test stash --index merges Message-ID: <8b5ea5e6f47ee9a57df3a4d97a457d024b3dec00.1790425008.git.ben.knoble@gmail.com> In-Reply-To: ``` 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. Suggested-by: Phillip Wood Signed-off-by: D. Ben Knoble --- t/t3903-stash.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 721158606f..9bc99fa252 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -374,6 +374,27 @@ setup_stash() { test_cmp expect actual ' +# 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 && -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-26 12:16 Subject: [PATCH v3 4/5] t3903: test failed "stash apply --index" Message-ID: In-Reply-To: ``` The next commit will refactor index handling for applied stashes, so let's make sure we cover conflicted index merging, too. Helped-by: Phillip Wood Signed-off-by: D. Ben Knoble --- t/t3903-stash.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 9bc99fa252..11942d875b 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -395,6 +395,27 @@ setup_stash() { test_cmp expect file ' +test_expect_success 'stash apply --index leaves everything untouched on failure' ' + git reset --hard && + echo test >other-file && + git add other-file && + git stash && + echo unrelated >file && + echo unrelated >another-file && + git add another-file && + echo conflict >other-file && + git add other-file && + git diff-files -p >expect && + git diff-index --cached HEAD >expect-index && + + test_must_fail git stash apply --index 2>err && + test_grep "conflicts in index. Try without --index" err && + git diff-files -p >actual && + test_cmp expect actual && + git diff-index --cached HEAD >actual-index && + test_cmp expect-index actual-index +' + test_expect_success 'stash -k' ' echo bar3 >file && echo bar4 >file2 && -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-26 12:16 Subject: [PATCH v3 5/5] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "git stash apply --index" does a 2-step dance to report index conflicts before carrying out the main unstash: first, attempt to merge the index (and remember the name of the resulting tree). If that succeeds, reset the index and carry on unstashing the working tree, then use the remembered index tree to unstash the index. The "merge the index" step is performed on the actual index by a combination of git-diff-tree(1) and git-apply(1), which incurs an extra cost to git-reset(1) to cleanup. This also introduces an autostash bug when stash.index is true: "git reset" eventually wants to remove_merge_branch_state(), which calls save_autostash() due to a03b55530a (merge: teach --autostash option, 2020-04-07). This can happen from a "git merge --autostash", which itself calls save_autostash(). Operating on the file-system in this way is not re-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH ref [1]. This bug has lurked for a while, but it would have been impossible to trigger without the availability of stash.index to force the autostash apply into index mode. [1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/ 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. 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 prefer to clean up these vestiges of the shell-based git-stash, though. Reported-by: Eli Barzilay Helped-by: Phillip Wood Helped-by: Junio C Hamano Signed-off-by: D. Ben Knoble --- builtin/stash.c | 80 ++++++++++-------------------------------------- t/t7600-merge.sh | 9 ++++++ 2 files changed, 26 insertions(+), 63 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index 043a38cc6d..ac3b3cf84d 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id, return ret; } -static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit) -{ - struct child_process cp = CHILD_PROCESS_INIT; - const char *w_commit_hex = oid_to_hex(w_commit); - - /* - * Diff-tree would not be very hard to replace with a native function, - * however it should be done together with apply_cached. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL); - strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex); - - return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); -} - -static int apply_cached(struct strbuf *out) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Apply currently only reads either from stdin or a file, thus - * apply_all_patches would have to be updated to optionally take a - * buffer. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "apply", "--cached", NULL); - return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0); -} - -static int reset_head(void) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Reset is overall quite simple, however there is no current public - * API for resetting. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL); - - return run_command(&cp); -} - static int is_path_a_directory(const char *path) { /* @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix, oideq(&c_tree, &info->i_tree)) { has_index = 0; } else { - struct strbuf out = STRBUF_INIT; + struct merge_result result = { 0 }; - if (diff_tree_binary(&out, &info->w_commit)) { - strbuf_release(&out); - return error(_("could not generate diff %s^!."), - oid_to_hex(&info->w_commit)); - } + o.branch1 = "Current index"; + o.branch2 = "Stashed index changes"; + o.ancestor = "Stash base"; - 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); + + merge_incore_nonrecursive(&o, merge_base, head, merge, + &result); + + oidcpy(&index_tree, &result.tree->object.oid); + merge_finalize(&o, &result); + + if (!result.clean) return error(_("conflicts in index. " "Try without --index.")); - - discard_index(the_repository->index); - repo_read_index(the_repository); - if (write_index_as_tree(&index_tree, the_repository->index, - repo_get_index_file(the_repository), 0, NULL)) - return error(_("could not save index tree")); - - reset_head(); - discard_index(the_repository->index); - repo_read_index(the_repository); } } diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index 64fe21717d..8f6109fb91 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -801,6 +801,15 @@ verify_no_mergehead () { test_cmp result.1-5 file ' +test_expect_success 'fast-forward merge with --autostash, stash.index' ' + git reset --hard c0 && + git stash clear && + echo staged >>z && git add z && + git -c stash.index=true merge --autostash c1 2>err && + test_grep "Applied autostash." err && + test_stdout_line_count = 0 git stash list +' + test_expect_success 'failed fast-forward merge with --autostash' ' git reset --hard c0 && git merge-file file file.orig file.5 && -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-26 12:20 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` On Sat, Sep 26, 2026 at 8:17 AM D. Ben Knoble wrote: > > 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 and fourth add missing test > coverage (which catch breakages from prior incorrect rounds of this > series), while the last holds the interesting bits. > > Changes in v3: Woops. Contrary to my usual practice of late, I sent this in reply to v2 rather than v1. Oh well. ``` ## Junio C Hamano, 2026-09-27 18:59 Subject: Re: [PATCH v3 5/5] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "D. Ben Knoble" writes: > @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix, > oideq(&c_tree, &info->i_tree)) { > has_index = 0; > } else { > - struct strbuf out = STRBUF_INIT; > + struct merge_result result = { 0 }; > > - if (diff_tree_binary(&out, &info->w_commit)) { > - strbuf_release(&out); > - return error(_("could not generate diff %s^!."), > - oid_to_hex(&info->w_commit)); > - } > + o.branch1 = "Current index"; > + o.branch2 = "Stashed index changes"; > + o.ancestor = "Stash base"; > > - ret = apply_cached(&out); > - strbuf_release(&out); > - if (ret) > + o.verbosity = 0; We realize that 'o' is a struct merge_options defined on the stack for this function, initialized with init_ui_merge_options() fairly early on. It would have initialized '.verbosity' to the default verbosity, the merge.verbosity configuration variable, or the GIT_MERGE_VERBOSITY environment variable. You drop the verbosity here, presumably because you want to match the previous implementation 'diff-tree | apply --cached' (which I guess was fairly quiet, but I do not use 'stash pop --index' myself). > + head = lookup_tree(o.repo, &c_tree); > + merge = lookup_tree(o.repo, &info->i_tree); > + merge_base = lookup_tree(o.repo, &info->b_tree); > + > + merge_incore_nonrecursive(&o, merge_base, head, merge, > + &result); > + > + oidcpy(&index_tree, &result.tree->object.oid); > + merge_finalize(&o, &result); And then the (index) merge is quiet, which is nice. > + > + if (!result.clean) > return error(_("conflicts in index. " > "Try without --index.")); > - > - discard_index(the_repository->index); > - repo_read_index(the_repository); > - if (write_index_as_tree(&index_tree, the_repository->index, > - repo_get_index_file(the_repository), 0, NULL)) > - return error(_("could not save index tree")); > - > - reset_head(); > - discard_index(the_repository->index); > - repo_read_index(the_repository); > } > } But the thing is, this is not the end of the function, or the last call to the merge machinery using 'o'. We then use the same 'o' to drive another three-way merge. Yet nobody restores '.verbosity' that was unconditionally turned off above for that second merge. It is a bit surprising that the existing test suite did not catch this. Perhaps we do not test --quiet and the merge.verbosity configuration in combination? Anyway, I think you'd need something like the following (caveat emptor: written against checked out 'seen' while reading the patch, and not even compile tested). Thanks. builtin/stash.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git c/builtin/stash.c w/builtin/stash.c index 0f10b9c703..a165419d77 100644 --- c/builtin/stash.c +++ w/builtin/stash.c @@ -622,6 +622,9 @@ static enum stash_apply_result do_apply_stash(const char *prefix, init_ui_merge_options(&o, the_repository); + if (quiet) + o.verbosity = 0; + if (index) { if (oideq(&info->b_tree, &info->i_tree) || oideq(&c_tree, &info->i_tree)) { @@ -633,8 +636,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix, o.branch2 = "Stashed index changes"; o.ancestor = "Stash base"; - 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); @@ -658,9 +659,6 @@ 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); ``` ## Junio C Hamano, 2026-09-27 19:21 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` "D. Ben Knoble" writes: > 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 and fourth add missing test > coverage (which catch breakages from prior incorrect rounds of this > series), while the last holds the interesting bits. I may have reported this on the previous round, too, but 'seen' seems to break t5520 when this topic is merged. I'll eject the topic from my tree for now in the meantime. ``` ## Junio C Hamano, 2026-09-28 09:40 Subject: Re: [PATCH v3 5/5] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "D. Ben Knoble" writes: > + merge_incore_nonrecursive(&o, merge_base, head, merge, > + &result); > + > + oidcpy(&index_tree, &result.tree->object.oid); This is risky, isn't it? If there were catastrophic failure (e.g., missing object that were involved in the merge), merge_incore_nonrecursive() may stuff -1 to result.clean and return without populating result.tree, and when that happens, result.tree->object.oid would be dereferencing NULL. ``` ## Phillip Wood, 2026-09-28 09:50 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: <346c4209-9600-4302-817f-e8f6b364ce6a@gmail.com> In-Reply-To: ``` On 27/09/2026 20:21, Junio C Hamano wrote: > "D. Ben Knoble" writes: > >> 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 and fourth add missing test >> coverage (which catch breakages from prior incorrect rounds of this >> series), while the last holds the interesting bits. > > I may have reported this on the previous round, too, but 'seen' > seems to break t5520 when this topic is merged. I'll eject the > topic from my tree for now in the meantime. I'm a bit stumped by that as the failing test (5520.69 '--rebase -f with rebased upstream') does not stash anything. There seems to be something funny going on with pull's fork-point detection. If I add GIT_TRACE=1 to "git pull --rebase" then on 'seen' I see trace: built-in: git rebase --no-autostash --onto ae9857430e281d178a3755aecfc5e29c46a02306 f29aa667ce68e4d514557081ca7f54b12e108922 but with this series I see trace: built-in: git rebase --no-autostash --onto ae9857430e281d178a3755aecfc5e29c46a02306 ae9857430e281d178a3755aecfc5e29c46a02306 so the upstream commit has changed. The previous test also checks the fork-point behavior and the failing test just runs "git reset --hard" at the start rather than re-creating the reflogs which seems a bit iffy to me but I've no idea why this series causes it to fail. I tried a merge of 'master' and 'seen' just in case the failure was caused by the base I'd used for this series but that passes. Thanks Phillip ``` ## D. Ben Knoble, 2026-09-28 12:02 Subject: Re: [PATCH v3 5/5] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` On Sun, Sep 27, 2026 at 2:59 PM Junio C Hamano wrote: > > "D. Ben Knoble" writes: > > > @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix, > > oideq(&c_tree, &info->i_tree)) { > > has_index = 0; > > } else { > > - struct strbuf out = STRBUF_INIT; > > + struct merge_result result = { 0 }; > > > > - if (diff_tree_binary(&out, &info->w_commit)) { > > - strbuf_release(&out); > > - return error(_("could not generate diff %s^!."), > > - oid_to_hex(&info->w_commit)); > > - } > > + o.branch1 = "Current index"; > > + o.branch2 = "Stashed index changes"; > > + o.ancestor = "Stash base"; > > > > - ret = apply_cached(&out); > > - strbuf_release(&out); > > - if (ret) > > + o.verbosity = 0; > > We realize that 'o' is a struct merge_options defined on the stack > for this function, initialized with init_ui_merge_options() fairly > early on. It would have initialized '.verbosity' to the default > verbosity, the merge.verbosity configuration variable, or the > GIT_MERGE_VERBOSITY environment variable. > > You drop the verbosity here, presumably because you want to match > the previous implementation 'diff-tree | apply --cached' (which I > guess was fairly quiet, but I do not use 'stash pop --index' > myself). Yes, the original piped "apply --cached" output to a strbuf and discarded it. > > + > > + if (!result.clean) > > return error(_("conflicts in index. " > > "Try without --index.")); > > - > > - discard_index(the_repository->index); > > - repo_read_index(the_repository); > > - if (write_index_as_tree(&index_tree, the_repository->index, > > - repo_get_index_file(the_repository), 0, NULL)) > > - return error(_("could not save index tree")); > > - > > - reset_head(); > > - discard_index(the_repository->index); > > - repo_read_index(the_repository); > > } > > } > > > But the thing is, this is not the end of the function, or the last > call to the merge machinery using 'o'. We then use the same 'o' to > drive another three-way merge. Yet nobody restores '.verbosity' > that was unconditionally turned off above for that second merge. But you're right, we should restore the verbosity (which is not what the sketch patch does exactly). Will fix. ``` ## D. Ben Knoble, 2026-09-28 12:03 Subject: Re: [PATCH v3 5/5] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` On Mon, Sep 28, 2026 at 5:40 AM Junio C Hamano wrote: > > "D. Ben Knoble" writes: > > > + merge_incore_nonrecursive(&o, merge_base, head, merge, > > + &result); > > + > > + oidcpy(&index_tree, &result.tree->object.oid); > > This is risky, isn't it? > > If there were catastrophic failure (e.g., missing object that were > involved in the merge), merge_incore_nonrecursive() may stuff -1 to > result.clean and return without populating result.tree, and when > that happens, result.tree->object.oid would be dereferencing NULL. Indeed… unfortunate. Thanks for spotting. -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-28 12:05 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: <346c4209-9600-4302-817f-e8f6b364ce6a@gmail.com> ``` On Mon, Sep 28, 2026 at 5:50 AM Phillip Wood wrote: > > On 27/09/2026 20:21, Junio C Hamano wrote: > > "D. Ben Knoble" writes: > > > >> 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 and fourth add missing test > >> coverage (which catch breakages from prior incorrect rounds of this > >> series), while the last holds the interesting bits. > > > > I may have reported this on the previous round, too, but 'seen' > > seems to break t5520 when this topic is merged. I'll eject the > > topic from my tree for now in the meantime. First I'm hearing about it, but I'll try to bisect seen and see what I can find. > I'm a bit stumped by that as the failing test (5520.69 '--rebase -f with > rebased upstream') does not stash anything. I wonder if a prior test is affected "silently" and we only find out by .69? > There seems to be something > funny going on with pull's fork-point detection. If I add GIT_TRACE=1 to > "git pull --rebase" then on 'seen' I see > > trace: built-in: git rebase --no-autostash --onto > ae9857430e281d178a3755aecfc5e29c46a02306 > f29aa667ce68e4d514557081ca7f54b12e108922 > > but with this series I see > > trace: built-in: git rebase --no-autostash --onto > ae9857430e281d178a3755aecfc5e29c46a02306 > ae9857430e281d178a3755aecfc5e29c46a02306 > > so the upstream commit has changed. The previous test also checks the > fork-point behavior and the failing test just runs "git reset --hard" at > the start rather than re-creating the reflogs which seems a bit iffy to > me but I've no idea why this series causes it to fail. I tried a merge > of 'master' and 'seen' just in case the failure was caused by the base > I'd used for this series but that passes. Thanks Phillip! -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-28 12:33 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` Just leaving some breadcrumb notes… On Mon, Sep 28, 2026 at 8:05 AM D. Ben Knoble wrote: > > On Mon, Sep 28, 2026 at 5:50 AM Phillip Wood wrote: > > > > On 27/09/2026 20:21, Junio C Hamano wrote: From my local version of the branch, the following script points at 4f65642eb0 (Merge branch 'tb/rerere-lock-grace' into jch, 2026-09-27): #! /bin/zsh HEAD=$(git rev-parse HEAD) && git merge --no-edit bk/autostash-index-reset && if ! ninja -C build; then exit 125; fi && meson test -C build t5520-pull # no chain! need to keep going no matter what code=$? && git reset --hard $HEAD && exit $code (using "git bisect start --first-parent origin/seen @") [Cc: Thomas Bachem in case you have any immediate ideas] I don't think the bisect log will interest anyone, but I've attached it anyway. -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-28 13:00 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` On Mon, Sep 28, 2026 at 8:33 AM D. Ben Knoble wrote: > > Just leaving some breadcrumb notes… > > On Mon, Sep 28, 2026 at 8:05 AM D. Ben Knoble wrote: > > > > On Mon, Sep 28, 2026 at 5:50 AM Phillip Wood wrote: > > > > > > On 27/09/2026 20:21, Junio C Hamano wrote: > > From my local version of the branch, the following script points at > 4f65642eb0 (Merge branch 'tb/rerere-lock-grace' into jch, 2026-09-27): And within that topic, bisect points to 2d1fa0323f (rebase, cherry-pick, revert: run auto maintenance when done, 2026-09-17) in t5220.69 as Phillip said. expecting success of 5520.69 '--rebase -f with rebased upstream': test_when_finished "test_might_fail git rebase --abort" && git reset --hard to-rebase-orig && git pull --rebase -f me copy && echo "conflicting modification" >expect && test_cmp expect file && echo file >expect && test_cmp expect file2 ++ test_when_finished 'test_might_fail git rebase --abort' ++ test 0 = 0 ++ test_cleanup=$'{ test_might_fail git rebase --abort\n\t\t} || eval_ret=$?; :' ++ git reset --hard to-rebase-orig HEAD is now at cb9bf26 to-rebase ++ git pull --rebase -f me copy From . * branch copy -> FETCH_HEAD Rebasing (1/4) Auto-merging file CONFLICT (content): Merge conflict in file error: could not apply f29aa66... file hint: Resolve all conflicts manually, mark them as resolved with hint: "git add/rm ", then run "git rebase --continue". hint: You can instead skip this commit: run "git rebase --skip". hint: To abort and get back to the state before "git rebase", run "git rebase --abort". hint: Disable this message with "git config set advice.mergeConflict false" Could not apply f29aa66... # file error: last command exited with $?=1 -- D. Ben Knoble ``` ## Phillip Wood, 2026-09-28 13:45 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` Hi Ben On 28/09/2026 14:00, D. Ben Knoble wrote: > On Mon, Sep 28, 2026 at 8:33 AM D. Ben Knoble wrote: >> >> Just leaving some breadcrumb notes… >> >> On Mon, Sep 28, 2026 at 8:05 AM D. Ben Knoble wrote: >>> >>> On Mon, Sep 28, 2026 at 5:50 AM Phillip Wood wrote: >>>> >>>> On 27/09/2026 20:21, Junio C Hamano wrote: >> >> From my local version of the branch, the following script points at >> 4f65642eb0 (Merge branch 'tb/rerere-lock-grace' into jch, 2026-09-27): > > And within that topic, bisect points to 2d1fa0323f (rebase, > cherry-pick, revert: run auto maintenance when done, 2026-09-17) Oh, when I was thinking about this over lunch I did wonder if that might be the culprit. Previously we didn't run "git maintenance --auto" after a rebase with the 'merge' backend but with that topic we do, and because we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire' gets triggered it will expire the reflog entries that 'git pull --rebase' relies on. As you suggested in another mail, I assume this topic has changed something in one of the '--autostash' tests that come before the failing test triggers which the new behavior. What that something is I'm not sure; off the top of my head I'd expect the number of reflog entries in HEAD to be the same but maybe I'm missing something. Adding git config maintenance.reflog-expire.auto 0 to the 'setup' test fixes the test failure, but it would be good to try and understand why this topic triggers the reflog to be expired in case there is something nasty happening that we've not thought of. Thanks Phillip > in > t5220.69 as Phillip said. > > expecting success of 5520.69 '--rebase -f with rebased upstream': > test_when_finished "test_might_fail git rebase --abort" && > git reset --hard to-rebase-orig && > git pull --rebase -f me copy && > echo "conflicting modification" >expect && > test_cmp expect file && > echo file >expect && > test_cmp expect file2 > > ++ test_when_finished 'test_might_fail git rebase --abort' > ++ test 0 = 0 > ++ test_cleanup=$'{ test_might_fail git rebase --abort\n\t\t} || eval_ret=$?; :' > ++ git reset --hard to-rebase-orig > HEAD is now at cb9bf26 to-rebase > ++ git pull --rebase -f me copy > From . > * branch copy -> FETCH_HEAD > Rebasing (1/4) > Auto-merging file > CONFLICT (content): Merge conflict in file > error: could not apply f29aa66... file > hint: Resolve all conflicts manually, mark them as resolved with > hint: "git add/rm ", then run "git rebase --continue". > hint: You can instead skip this commit: run "git rebase --skip". > hint: To abort and get back to the state before "git rebase", run "git > rebase --abort". > hint: Disable this message with "git config set advice.mergeConflict false" > Could not apply f29aa66... # file > error: last command exited with $?=1 > > ``` ## Thomas Bachem, 2026-09-28 14:50 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood wrote: > Oh, when I was thinking about this over lunch I did wonder if that might > be the culprit. Previously we didn't run "git maintenance --auto" after > a rebase with the 'merge' backend but with that topic we do, and because > we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire' > gets triggered it will expire the reflog entries that 'git pull > --rebase' relies on. As you suggested in another mail, I assume this That is it. I ran t5520 on 'seen' with and without Ben's series under GIT_TRACE2_EVENT, and the two runs differ in one place: which command's auto maintenance runs "git reflog expire --all". "git pull --rebase" computes the fork point before it fetches, from the reflog of refs/remotes/me/copy, and test 69 needs the entry that test 68's fetch wrote there, copy-orig (f29aa66) to ae98574. With the reflog empty, "merge-base --fork-point" falls back to the ref itself, ae98574 is no ancestor of to-rebase, and pull hands the merge head to rebase as the upstream. That is your "--onto ae98... ae98...", and the four commits from copy-orig up come back, the first of them conflicting with "conflict". > topic has changed something in one of the '--autostash' tests that come > before the failing test triggers which the new behavior. What that > something is I'm not sure; off the top of my head I'd expect the number > of reflog entries in HEAD to be the same but maybe I'm missing > something. Adding It is eight entries fewer, and they come from the failed merges, not from the autostash tests. "git merge" restores a dirty tree with "stash apply --index --quiet", and until Ben's series that spawned "git reset --quiet --refresh", which writes "reset: moving to HEAD" to the reflog. That happens eight times in t5520 before test 68. Auto maintenance expires reflogs once HEAD's reflog holds a hundred entries that the policy would remove, the default of maintenance.reflog-expire.auto, and after the first test_tick that is every entry. Which run crosses the hundred depends on how many entries and maintenance runs came before it. On 'seen' the expiry lands on "git commit -m conflict" in test 68, before the fetch writes the entry. Eight entries fewer move the crossing past that commit, and the maintenance run my topic adds at the end of the rebase in test 68 is the next one: after the fetch, before test 69 reads the reflog. Either change alone leaves it somewhere harmless, and nothing else is going on. The expiry is the usual 90 days applied to entries dated 2005, and the only new thing is one more maintenance run per rebase, the same one "git commit" and "git fetch" run. > git config maintenance.reflog-expire.auto 0 > > to the 'setup' test fixes the test failure, but it would be good to try > and understand why this topic triggers the reflog to be expired in case > there is something nasty happening that we've not thought of. I'd pin the expiry itself instead, as ea7d894f44 (t34xx: don't expire reflogs where it matters, 2026-02-24) did for the rebase tests: git config set gc.reflogExpire never && git config set gc.reflogExpireUnreachable never && That covers a "git gc" as well, which expires reflogs on its own. With it, 'seen' plus Ben's series passes t5520 here and no expiry runs during the script at all. I sent it as a patch on master: FWIW, any script that reads a reflog after a hundred HEAD updates can fall into the same hole. I have not looked further than t5520. Thomas ``` ## Junio C Hamano, 2026-09-28 15:32 Subject: Re: [PATCH v3 5/5] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "D. Ben Knoble" writes: > On Mon, Sep 28, 2026 at 5:40 AM Junio C Hamano wrote: >> >> "D. Ben Knoble" writes: >> >> > + merge_incore_nonrecursive(&o, merge_base, head, merge, >> > + &result); >> > + >> > + oidcpy(&index_tree, &result.tree->object.oid); >> >> This is risky, isn't it? >> >> If there were catastrophic failure (e.g., missing object that were >> involved in the merge), merge_incore_nonrecursive() may stuff -1 to >> result.clean and return without populating result.tree, and when >> that happens, result.tree->object.oid would be dereferencing NULL. > > Indeed… unfortunate. Thanks for spotting. I did $ git grep merge_incore_nonrecursive \*.c and read all the current callers. They all have code to specifically check for the (result.clean < 0) condition and error out before touching any of the other members of the result structure, so they seem to be safe. ``` ## D. Ben Knoble, 2026-09-28 15:36 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` Let me see if I understand correctly… On Mon, Sep 28, 2026 at 10:50 AM Thomas Bachem wrote: > > On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood wrote: > > Oh, when I was thinking about this over lunch I did wonder if that might > > be the culprit. Previously we didn't run "git maintenance --auto" after > > a rebase with the 'merge' backend but with that topic we do, and because > > we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire' > > gets triggered it will expire the reflog entries that 'git pull > > --rebase' relies on. As you suggested in another mail, I assume this > "git pull --rebase" computes the fork point before it fetches, from > the reflog of refs/remotes/me/copy, This is described by the manual for git-rebase under --fork-point, which is on unless we have an or --keep-base (modulo config). Put a pin in this. > and test 69 needs the entry that > test 68's fetch wrote there, copy-orig (f29aa66) to ae98574. With the > reflog empty, "merge-base --fork-point" falls back to the ref itself, > ae98574 is no ancestor of to-rebase, and pull hands the merge head to > rebase as the upstream. That is your "--onto ae98... ae98...", and the > four commits from copy-orig up come back, the first of them > conflicting with "conflict". > > > topic has changed something in one of the '--autostash' tests that come > > before the failing test triggers which the new behavior. What that > > something is I'm not sure; off the top of my head I'd expect the number > > of reflog entries in HEAD to be the same but maybe I'm missing > > something. Adding > > It is eight entries fewer, and they come from the failed merges, not > from the autostash tests. "git merge" restores a dirty tree with > "stash apply --index --quiet", and until Ben's series that spawned > "git reset --quiet --refresh", which writes "reset: moving to HEAD" > to the reflog. That happens eight times in t5520 before test 68. > > Auto maintenance expires reflogs once HEAD's reflog holds a hundred > entries that the policy would remove, the default of > maintenance.reflog-expire.auto, and after the first test_tick that is > every entry. Which run crosses the hundred depends on how many entries > and maintenance runs came before it. On 'seen' the expiry lands on > "git commit -m conflict" in test 68, before the fetch writes the entry. > Eight entries fewer move the crossing past that commit, and the > maintenance run my topic adds at the end of the rebase in test 68 is > the next one: after the fetch, before test 69 reads the reflog. Either > change alone leaves it somewhere harmless, and nothing else is going > on. The expiry is the usual 90 days applied to entries dated 2005, and > the only new thing is one more maintenance run per rebase, the same > one "git commit" and "git fetch" run. In short, expiry used to happen prior to .68, so the reflog entry created in that test which is used by "pull --rebase" in .69 is picked up. With fewer reflog entries, expiry happens later, and it just so happens to drop the important entry. Darn! But here's what I can't figure out, returning to that pin from earlier: I was a bit surprised to see mention of rebase reading reflogs! When I remembered --fork-point, I was even more curious (but at least it's obvious that rebase will read the reflogs in some scenarios). What confuses me is that builtin/pull.c:run_rebase() sure looks like it provides an to the command invocation, so shouldn't --fork-point and reflog use be disabled???? I'll try tracing that test myself later, I suppose. It's nice to know we have a fix available (thanks for the patch), but it sure feels like a hack :) oh well? -- D. Ben Knoble ``` ## Phillip Wood, 2026-09-28 15:40 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` Hi Thomas On 28/09/2026 15:50, Thomas Bachem wrote: > On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood wrote: >> >> topic has changed something in one of the '--autostash' tests that come >> before the failing test triggers which the new behavior. What that >> something is I'm not sure; off the top of my head I'd expect the number >> of reflog entries in HEAD to be the same but maybe I'm missing >> something. Adding > > It is eight entries fewer, and they come from the failed merges, not > from the autostash tests. "git merge" restores a dirty tree with > "stash apply --index --quiet", Thanks for tracking that down, I couldn't see where we'd be calling "git stash apply" with "--index" but builtin/merge.c:restore_state() calls "git stash apply --index --quiet" rather than calling one of the autostash helper functions which do not use "--index". > and until Ben's series that spawned > "git reset --quiet --refresh", which writes "reset: moving to HEAD" > to the reflog. That happens eight times in t5520 before test 68. That accounts for the difference in the number of reflog entries. It's good to have an explanation for why we're expiring the reflog entries at a slightly different time. Thanks Phillip > Auto maintenance expires reflogs once HEAD's reflog holds a hundred > entries that the policy would remove, the default of > maintenance.reflog-expire.auto, and after the first test_tick that is > every entry. Which run crosses the hundred depends on how many entries > and maintenance runs came before it. On 'seen' the expiry lands on > "git commit -m conflict" in test 68, before the fetch writes the entry. > Eight entries fewer move the crossing past that commit, and the > maintenance run my topic adds at the end of the rebase in test 68 is > the next one: after the fetch, before test 69 reads the reflog. Either > change alone leaves it somewhere harmless, and nothing else is going > on. The expiry is the usual 90 days applied to entries dated 2005, and > the only new thing is one more maintenance run per rebase, the same > one "git commit" and "git fetch" run. > >> git config maintenance.reflog-expire.auto 0 >> >> to the 'setup' test fixes the test failure, but it would be good to try >> and understand why this topic triggers the reflog to be expired in case >> there is something nasty happening that we've not thought of. > > I'd pin the expiry itself instead, as ea7d894f44 (t34xx: don't expire > reflogs where it matters, 2026-02-24) did for the rebase tests: > > git config set gc.reflogExpire never && > git config set gc.reflogExpireUnreachable never && > > That covers a "git gc" as well, which expires reflogs on its own. With > it, 'seen' plus Ben's series passes t5520 here and no expiry runs > during the script at all. I sent it as a patch on master: > > > FWIW, any script that reads a reflog after a hundred HEAD updates can > fall into the same hole. I have not looked further than t5520. > > Thomas ``` ## Phillip Wood, 2026-09-28 15:44 Subject: Re: [PATCH v3 3/5] t3903: test stash --index merges Message-ID: <97f86d82-b5ec-44df-9ccf-8e6cd93e45f4@gmail.com> In-Reply-To: <8b5ea5e6f47ee9a57df3a4d97a457d024b3dec00.1790425008.git.ben.knoble@gmail.com> ``` Hi Ben On 26/09/2026 13:16, D. Ben Knoble wrote: > 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 test looks good, but without the changes in patch 5 it fails and so adding it here breaks running "git bisect" on this series. I'd squash this into the final patch and I think we can probably replace an existing "stash apply --index" tests that are not so strict with this one, rather than adding a new test. Thanks Phillip > Suggested-by: Phillip Wood > Signed-off-by: D. Ben Knoble > --- > t/t3903-stash.sh | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 721158606f..9bc99fa252 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -374,6 +374,27 @@ setup_stash() { > test_cmp expect actual > ' > > +# 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 && ``` ## D. Ben Knoble, 2026-09-28 15:55 Subject: Re: [PATCH v3 3/5] t3903: test stash --index merges Message-ID: In-Reply-To: <97f86d82-b5ec-44df-9ccf-8e6cd93e45f4@gmail.com> ``` On Mon, Sep 28, 2026 at 11:44 AM Phillip Wood wrote: > > Hi Ben > > On 26/09/2026 13:16, D. Ben Knoble wrote: > > 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 test looks good, but without the changes in patch 5 it fails and so > adding it here breaks running "git bisect" on this series. I'd squash > this into the final patch Interesting. I thought I checked that the test passed sans patch 5, but I'll double check. I can't think of a reason it wouldn't offhand, but my thoughts on patch 5's changes have become a bit scattered. > and I think we can probably replace an > existing "stash apply --index" tests that are not so strict with this > one, rather than adding a new test. That's probably a good idea, thanks. ``` ## Phillip Wood, 2026-09-29 09:41 Subject: Re: [PATCH v3 3/5] t3903: test stash --index merges Message-ID: <21a5c1fc-b268-493c-bd61-fa0afdf98bee@gmail.com> In-Reply-To: ``` Hi Ben On 28/09/2026 16:55, D. Ben Knoble wrote: > On Mon, Sep 28, 2026 at 11:44 AM Phillip Wood wrote: >> >> The test looks good, but without the changes in patch 5 it fails and so >> adding it here breaks running "git bisect" on this series. I'd squash >> this into the final patch > > Interesting. I thought I checked that the test passed sans patch 5, > but I'll double check. I can't think of a reason it wouldn't offhand, > but my thoughts on patch 5's changes have become a bit scattered. It fails because it tries to apply a patch that looks like @@ -1,3 +1,3 @@ A B -C +staged to a file that looks like committed B C and so the first context line does not match. Because the changes do not overlap the merge machinery is perfectly happy. As an aside when we clear the worktree changes from "git stash push -p" generate the patch with "-U1" to try and avoid problems like this. Thanks Phillip > >> and I think we can probably replace an >> existing "stash apply --index" tests that are not so strict with this >> one, rather than adding a new test. > > That's probably a good idea, thanks. ``` ## D. Ben Knoble, 2026-09-29 11:38 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` On Mon, Sep 28, 2026 at 11:36 AM D. Ben Knoble wrote: > > Let me see if I understand correctly… > > On Mon, Sep 28, 2026 at 10:50 AM Thomas Bachem wrote: > > > > On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood wrote: > > > Oh, when I was thinking about this over lunch I did wonder if that might > > > be the culprit. Previously we didn't run "git maintenance --auto" after > > > a rebase with the 'merge' backend but with that topic we do, and because > > > we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire' > > > gets triggered it will expire the reflog entries that 'git pull > > > --rebase' relies on. As you suggested in another mail, I assume this > > > "git pull --rebase" computes the fork point before it fetches, from > > the reflog of refs/remotes/me/copy, > > This is described by the manual for git-rebase under --fork-point, > which is on unless we have an or --keep-base (modulo > config). Put a pin in this. > But here's what I can't figure out, returning to that pin from > earlier: I was a bit surprised to see mention of rebase reading > reflogs! When I remembered --fork-point, I was even more curious (but > at least it's obvious that rebase will read the reflogs in some > scenarios). > > What confuses me is that builtin/pull.c:run_rebase() sure looks like > it provides an to the command invocation, so shouldn't > --fork-point and reflog use be disabled???? Indeed, from GIT_TRACE2 output I can see we do run git rebase --no-autostash --onto ae98… f29a… but well before that we run git merge-base --fork-point refs/remotes/me/copy to-rebase which is then presumably fed down to the rebase. Interesting. -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-29 12:18 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 ``` ## D. Ben Knoble, 2026-09-29 12:18 Subject: [PATCH v4 1/5] builtin/stash: remove unused header Message-ID: <6a165c4df456b6bd5e5ab46664b023a45e670926.1790684309.git.ben.knoble@gmail.com> In-Reply-To: ``` Clang complains that oid-array.h is unused. Certainly none of the oid_array* functions, types, etc., are used, and the transitively-included hash.h declarations are used but covered by a pre-existing direct #include of hash.h. Signed-off-by: D. Ben Knoble --- builtin/stash.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index 7a9843413b..dfea2d2c4c 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -31,7 +31,6 @@ #include "reflog.h" #include "reflog-walk.h" #include "add-interactive.h" -#include "oid-array.h" #include "commit.h" #define INCLUDE_ALL_FILES 2 -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-29 12:18 Subject: [PATCH v4 2/5] stash: prepare merge options earlier Message-ID: <35b64ae3217629498ea19c1285edfeb32c5cb53d.1790684309.git.ben.knoble@gmail.com> In-Reply-To: ``` In a future commit, we will reuse these options for the index merge of "apply --index", not just for the worktree. Signed-off-by: D. Ben Knoble --- builtin/stash.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index dfea2d2c4c..d2b736d4e6 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -664,6 +664,11 @@ static enum stash_apply_result do_apply_stash(const char *prefix, repo_get_index_file(the_repository), 0, NULL)) 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) || oideq(&c_tree, &info->i_tree)) { @@ -695,8 +700,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix, } } - init_ui_merge_options(&o, the_repository); - o.branch1 = label_ours ? label_ours : "Updated upstream"; o.branch2 = label_theirs ? label_theirs : "Stashed changes"; o.ancestor = label_base ? label_base : "Stash base"; @@ -704,9 +707,6 @@ 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); -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-29 12:18 Subject: [PATCH v4 3/5] t3903: test failed "stash apply --index" Message-ID: <7b0b317ce061d672ef143b1628a2d0097a878c87.1790684309.git.ben.knoble@gmail.com> In-Reply-To: ``` The next commit will refactor index handling for applied stashes, so let's make sure we cover conflicted index merging, too. Helped-by: Phillip Wood Signed-off-by: D. Ben Knoble --- t/t3903-stash.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 721158606f..70af58e161 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -374,6 +374,27 @@ setup_stash() { test_cmp expect actual ' +test_expect_success 'stash apply --index leaves everything untouched on failure' ' + git reset --hard && + echo test >other-file && + git add other-file && + git stash && + echo unrelated >file && + echo unrelated >another-file && + git add another-file && + echo conflict >other-file && + git add other-file && + git diff-files -p >expect && + git diff-index --cached HEAD >expect-index && + + test_must_fail git stash apply --index 2>err && + test_grep "conflicts in index. Try without --index" err && + git diff-files -p >actual && + test_cmp expect actual && + git diff-index --cached HEAD >actual-index && + test_cmp expect-index actual-index +' + test_expect_success 'stash -k' ' echo bar3 >file && echo bar4 >file2 && -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-29 12:18 Subject: [PATCH v4 4/5] t5520: don't expire reflogs where it matters Message-ID: <2ac371d2dc1425cc47bf369e88b321d3c0c8c605.1790684309.git.ben.knoble@gmail.com> In-Reply-To: ``` From: Thomas Bachem 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. 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. 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 Signed-off-by: D. Ben Knoble --- t/t5520-pull.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 27f38ab3c8..bc818605a5 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -35,6 +35,12 @@ test_pull_autostash_fail () { } 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 && + echo file >file && git add file && git commit -a -m original -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-29 12:18 Subject: [PATCH v4 5/5] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "git stash apply --index" does a 2-step dance to report index conflicts before carrying out the main unstash: first, attempt to merge the index (and remember the name of the resulting tree). If that succeeds, reset the index and carry on unstashing the working tree, then use the remembered index tree to unstash the index. The "merge the index" step is performed on the actual index by a combination of git-diff-tree(1) and git-apply(1), which incurs an extra cost to git-reset(1) to cleanup. This also introduces an autostash bug when stash.index is true: "git reset" eventually wants to remove_merge_branch_state(), which calls save_autostash() due to a03b55530a (merge: teach --autostash option, 2020-04-07). This can happen from a "git merge --autostash", which itself calls save_autostash(). Operating on the file-system in this way is not re-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH ref [1]. This bug has lurked for a while, but it would have been impossible to trigger without the availability of stash.index to force the autostash apply into index mode. [1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/ 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. 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 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. Reported-by: Eli Barzilay Helped-by: Phillip Wood Helped-by: Junio C Hamano Signed-off-by: D. Ben Knoble --- builtin/stash.c | 80 ++++++++++-------------------------------------- t/t3903-stash.sh | 21 +++++++++++++ t/t7600-merge.sh | 9 ++++++ 3 files changed, 47 insertions(+), 63 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index d2b736d4e6..ec07547376 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id, return ret; } -static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit) -{ - struct child_process cp = CHILD_PROCESS_INIT; - const char *w_commit_hex = oid_to_hex(w_commit); - - /* - * Diff-tree would not be very hard to replace with a native function, - * however it should be done together with apply_cached. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL); - strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex); - - return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); -} - -static int apply_cached(struct strbuf *out) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Apply currently only reads either from stdin or a file, thus - * apply_all_patches would have to be updated to optionally take a - * buffer. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "apply", "--cached", NULL); - return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0); -} - -static int reset_head(void) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Reset is overall quite simple, however there is no current public - * API for resetting. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL); - - return run_command(&cp); -} - static int is_path_a_directory(const char *path) { /* @@ -674,29 +630,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix, oideq(&c_tree, &info->i_tree)) { has_index = 0; } else { - struct strbuf out = STRBUF_INIT; + struct merge_result result = { 0 }; - if (diff_tree_binary(&out, &info->w_commit)) { - strbuf_release(&out); - return error(_("could not generate diff %s^!."), - oid_to_hex(&info->w_commit)); - } + o.branch1 = "Current index"; + o.branch2 = "Stashed index changes"; + o.ancestor = "Stash base"; - ret = apply_cached(&out); - strbuf_release(&out); - if (ret) + head = lookup_tree(o.repo, &c_tree); + merge = lookup_tree(o.repo, &info->i_tree); + merge_base = lookup_tree(o.repo, &info->b_tree); + + merge_incore_nonrecursive(&o, merge_base, head, merge, + &result); + + if (!result.clean) { + merge_finalize(&o, &result); return error(_("conflicts in index. " "Try without --index.")); - - discard_index(the_repository->index); - repo_read_index(the_repository); - if (write_index_as_tree(&index_tree, the_repository->index, - repo_get_index_file(the_repository), 0, NULL)) - return error(_("could not save index tree")); - - reset_head(); - discard_index(the_repository->index); - repo_read_index(the_repository); + } else { + oidcpy(&index_tree, &result.tree->object.oid); + merge_finalize(&o, &result); + } } } diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 70af58e161..70c6031958 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -395,6 +395,27 @@ 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 && diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index 64fe21717d..8f6109fb91 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -801,6 +801,15 @@ verify_no_mergehead () { test_cmp result.1-5 file ' +test_expect_success 'fast-forward merge with --autostash, stash.index' ' + git reset --hard c0 && + git stash clear && + echo staged >>z && git add z && + git -c stash.index=true merge --autostash c1 2>err && + test_grep "Applied autostash." err && + test_stdout_line_count = 0 git stash list +' + test_expect_success 'failed fast-forward merge with --autostash' ' git reset --hard c0 && git merge-file file file.orig file.5 && -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## Phillip Wood, 2026-09-29 15:46 Subject: Re: [PATCH v4 4/5] t5520: don't expire reflogs where it matters Message-ID: <3547f4aa-649a-4f46-868c-0e50dfa69466@gmail.com> In-Reply-To: <2ac371d2dc1425cc47bf369e88b321d3c0c8c605.1790684309.git.ben.knoble@gmail.com> ``` On 29/09/2026 13:18, D. Ben Knoble wrote: > From: Thomas Bachem > > 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. > > 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. > > 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. I find this commit message quite hard to understand. From my perspective the important points are - "git merge" uses "git stash" to clear any uncommitted changes from the worktree before it tries each strategy. The stashed changes are popped with "--index". - switching "git stash pop --index" to use merge_incore_nonrecursive() causes "git merge" to stop writing the reflog entries that came from "git stash pop --index" running "git reset" - that combined with "git rebase" starting to run "git maintenance --auto" changed when we expire the reflogs which breaks the fork- point detection. The changes themselves look good Thanks Phillip > > 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 > Signed-off-by: D. Ben Knoble > --- > t/t5520-pull.sh | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh > index 27f38ab3c8..bc818605a5 100755 > --- a/t/t5520-pull.sh > +++ b/t/t5520-pull.sh > @@ -35,6 +35,12 @@ test_pull_autostash_fail () { > } > > 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 && > + > echo file >file && > git add file && > git commit -a -m original ``` ## Phillip Wood, 2026-09-29 15:48 Subject: Re: [PATCH v4 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` Hi Ben On 29/09/2026 13:18, D. Ben Knoble wrote: > > 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 ;) I've left a comment on the new patch 4, but everything else in the range-diff looks ready to me. Thanks Phillip > 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 ``` ## Phillip Wood, 2026-09-29 15:54 Subject: Re: [PATCH v3 0/5] stash: clean up index-mode test merge Message-ID: <54957537-e40d-45b6-886c-5fc433f3d54e@gmail.com> In-Reply-To: ``` Hi Ben On 28/09/2026 16:36, D. Ben Knoble wrote: > Let me see if I understand correctly… > > On Mon, Sep 28, 2026 at 10:50 AM Thomas Bachem wrote: >> >> On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood wrote: >>> Oh, when I was thinking about this over lunch I did wonder if that might >>> be the culprit. Previously we didn't run "git maintenance --auto" after >>> a rebase with the 'merge' backend but with that topic we do, and because >>> we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire' >>> gets triggered it will expire the reflog entries that 'git pull >>> --rebase' relies on. As you suggested in another mail, I assume this > >> "git pull --rebase" computes the fork point before it fetches, from >> the reflog of refs/remotes/me/copy, > > This is described by the manual for git-rebase under --fork-point, > which is on unless we have an or --keep-base (modulo > config). Put a pin in this. > >> and test 69 needs the entry that >> test 68's fetch wrote there, copy-orig (f29aa66) to ae98574. With the >> reflog empty, "merge-base --fork-point" falls back to the ref itself, >> ae98574 is no ancestor of to-rebase, and pull hands the merge head to >> rebase as the upstream. That is your "--onto ae98... ae98...", and the >> four commits from copy-orig up come back, the first of them >> conflicting with "conflict". >> >>> topic has changed something in one of the '--autostash' tests that come >>> before the failing test triggers which the new behavior. What that >>> something is I'm not sure; off the top of my head I'd expect the number >>> of reflog entries in HEAD to be the same but maybe I'm missing >>> something. Adding >> >> It is eight entries fewer, and they come from the failed merges, not >> from the autostash tests. "git merge" restores a dirty tree with >> "stash apply --index --quiet", and until Ben's series that spawned >> "git reset --quiet --refresh", which writes "reset: moving to HEAD" >> to the reflog. That happens eight times in t5520 before test 68. >> >> Auto maintenance expires reflogs once HEAD's reflog holds a hundred >> entries that the policy would remove, the default of >> maintenance.reflog-expire.auto, and after the first test_tick that is >> every entry. Which run crosses the hundred depends on how many entries >> and maintenance runs came before it. On 'seen' the expiry lands on >> "git commit -m conflict" in test 68, before the fetch writes the entry. >> Eight entries fewer move the crossing past that commit, and the >> maintenance run my topic adds at the end of the rebase in test 68 is >> the next one: after the fetch, before test 69 reads the reflog. Either >> change alone leaves it somewhere harmless, and nothing else is going >> on. The expiry is the usual 90 days applied to entries dated 2005, and >> the only new thing is one more maintenance run per rebase, the same >> one "git commit" and "git fetch" run. > > In short, expiry used to happen prior to .68, so the reflog entry > created in that test which is used by "pull --rebase" in .69 is picked > up. With fewer reflog entries, expiry happens later, and it just so > happens to drop the important entry. Darn! Yes, it is incredibly bad luck that the test broke, though I guess it is also fortunate as it means we can fix the latent bug in the test. > > But here's what I can't figure out, returning to that pin from > earlier: I was a bit surprised to see mention of rebase reading > reflogs! When I remembered --fork-point, I was even more curious (but > at least it's obvious that rebase will read the reflogs in some > scenarios). > > What confuses me is that builtin/pull.c:run_rebase() sure looks like > it provides an to the command invocation, so shouldn't > --fork-point and reflog use be disabled???? > > I'll try tracing that test myself later, I suppose. It's nice to know > we have a fix available (thanks for the patch), but it sure feels like > a hack :) oh well? It is a bit confusing that "git pull --rebase" does not use "git rebase --fork-point", instead it calls "git merge-base --fork-point" (which is where we read the reflog of the remote branch) itself and then passes that as the upstream revision to "git rebase". I think this is because fork-point handling was added to "git pull" before "--fork-point" existed in "git rebase". "git rebase" only looks for a fork-point if its upstream argument is a ref, so as "git pull" passes an object id, the fork-point detection in rebase is bypassed. Thanks Phillip ``` ## Ben Knoble, 2026-09-29 17:31 Subject: Re: [PATCH v4 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` > Le 29 sept. 2026 à 11:48, Phillip Wood a écrit : > > Hi Ben > >> On 29/09/2026 13:18, D. Ben Knoble wrote: >> 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 ;) > > I've left a comment on the new patch 4, but everything else in the range-diff looks ready to me. > > Thanks > > Phillip Thanks Phillip. Pending other positive acks, I’m not sure if I should reroll with Thomas’s new patch, reroll dropping it now there’s a seen topic for it, or just wait ;) I’ll probably wait a bit and see how the dust settles, but: Junio if you want to see a reroll hit the list using the new synthetic base to make things nicer for you, I can do so. In particular, I think the last check I made when I saw your mail about the synthetic base had the prior round. ``` ## Junio C Hamano, 2026-09-29 20:07 Subject: Re: [PATCH v4 5/5] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "D. Ben Knoble" writes: > + merge_incore_nonrecursive(&o, merge_base, head, merge, > + &result); In a hard error from merge_incore_nonrecursive(), result->clean is set to -1, which means that ... > + if (!result.clean) { ... "result.clean is false" is not true here, so we will ... > + merge_finalize(&o, &result); > return error(_("conflicts in index. " > "Try without --index.")); > + } else { ... come here to access result.tree member, no? > + oidcpy(&index_tree, &result.tree->object.oid); > + merge_finalize(&o, &result); > + } IOW, shouldn't it be more like three-way check, if (result.clean < 0) { merge_finalize(&o, &result); return error(_("index merge failed.")); } else if (!result.clean) { merge_finalize(&o, &result); return error(_("conflict in index merge.")); } else { ... happy path ... } or something like that? ``` ## D. Ben Knoble, 2026-09-30 01:29 Subject: Re: [PATCH v4 5/5] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` On Tue, Sep 29, 2026 at 4:07 PM Junio C Hamano wrote: > > "D. Ben Knoble" writes: > > > + merge_incore_nonrecursive(&o, merge_base, head, merge, > > + &result); > > In a hard error from merge_incore_nonrecursive(), result->clean is > set to -1, which means that ... > > > + if (!result.clean) { > > ... "result.clean is false" is not true here, so we will ... > > > + merge_finalize(&o, &result); > > return error(_("conflicts in index. " > > "Try without --index.")); > > + } else { > > ... come here to access result.tree member, no? > > > + oidcpy(&index_tree, &result.tree->object.oid); > > + merge_finalize(&o, &result); > > + } > > IOW, shouldn't it be more like three-way check, Yep. Missed that when looking at the result struct. Will fix. > > if (result.clean < 0) { > merge_finalize(&o, &result); > return error(_("index merge failed.")); > } else if (!result.clean) { > merge_finalize(&o, &result); > return error(_("conflict in index merge.")); > } else { > ... happy path ... > } > > or something like that? -- D. Ben Knoble ``` ## D. Ben Knoble, 2026-09-30 21:24 Subject: [PATCH v5 1/4] builtin/stash: remove unused header Message-ID: In-Reply-To: ``` Clang complains that oid-array.h is unused. Certainly none of the oid_array* functions, types, etc., are used, and the transitively-included hash.h declarations are used but covered by a pre-existing direct #include of hash.h. Signed-off-by: D. Ben Knoble --- builtin/stash.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index 7a9843413b..dfea2d2c4c 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -31,7 +31,6 @@ #include "reflog.h" #include "reflog-walk.h" #include "add-interactive.h" -#include "oid-array.h" #include "commit.h" #define INCLUDE_ALL_FILES 2 -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-30 21:24 Subject: [PATCH v5 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 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 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 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 -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-30 21:24 Subject: [PATCH v5 3/4] t3903: test failed "stash apply --index" Message-ID: In-Reply-To: ``` The next commit will refactor index handling for applied stashes, so let's make sure we cover conflicted index merging, too. Helped-by: Phillip Wood Signed-off-by: D. Ben Knoble --- t/t3903-stash.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 721158606f..70af58e161 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -374,6 +374,27 @@ setup_stash() { test_cmp expect actual ' +test_expect_success 'stash apply --index leaves everything untouched on failure' ' + git reset --hard && + echo test >other-file && + git add other-file && + git stash && + echo unrelated >file && + echo unrelated >another-file && + git add another-file && + echo conflict >other-file && + git add other-file && + git diff-files -p >expect && + git diff-index --cached HEAD >expect-index && + + test_must_fail git stash apply --index 2>err && + test_grep "conflicts in index. Try without --index" err && + git diff-files -p >actual && + test_cmp expect actual && + git diff-index --cached HEAD >actual-index && + test_cmp expect-index actual-index +' + test_expect_success 'stash -k' ' echo bar3 >file && echo bar4 >file2 && -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-30 21:24 Subject: [PATCH v5 2/4] stash: prepare merge options earlier Message-ID: <8e99033ef025003c35bd82069ea89b1509e8bd9b.1790803471.git.ben.knoble@gmail.com> In-Reply-To: ``` In a future commit, we will reuse these options for the index merge of "apply --index", not just for the worktree. Signed-off-by: D. Ben Knoble --- builtin/stash.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index dfea2d2c4c..d2b736d4e6 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -664,6 +664,11 @@ static enum stash_apply_result do_apply_stash(const char *prefix, repo_get_index_file(the_repository), 0, NULL)) 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) || oideq(&c_tree, &info->i_tree)) { @@ -695,8 +700,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix, } } - init_ui_merge_options(&o, the_repository); - o.branch1 = label_ours ? label_ours : "Updated upstream"; o.branch2 = label_theirs ? label_theirs : "Stashed changes"; o.ancestor = label_base ? label_base : "Stash base"; @@ -704,9 +707,6 @@ 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); -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-30 21:24 Subject: [PATCH v5 4/4] builtin/stash: merge index in-core Message-ID: In-Reply-To: ``` "git stash apply --index" does a 2-step dance to report index conflicts before carrying out the main unstash: first, attempt to merge the index (and remember the name of the resulting tree). If that succeeds, reset the index and carry on unstashing the working tree, then use the remembered index tree to unstash the index. The "merge the index" step is performed on the actual index by a combination of git-diff-tree(1) and git-apply(1), which incurs an extra cost to git-reset(1) to cleanup. This also introduces an autostash bug when stash.index is true: "git reset" eventually wants to remove_merge_branch_state(), which calls save_autostash() due to a03b55530a (merge: teach --autostash option, 2020-04-07). This can happen from a "git merge --autostash", which itself calls save_autostash(). Operating on the file-system in this way is not re-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH ref [1]. This bug has lurked for a while, but it would have been impossible to trigger without the availability of stash.index to force the autostash apply into index mode. [1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/ 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. 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 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. Reported-by: Eli Barzilay Helped-by: Phillip Wood Helped-by: Junio C Hamano Signed-off-by: D. Ben Knoble --- builtin/stash.c | 83 ++++++++++++------------------------------------ t/t3903-stash.sh | 21 ++++++++++++ t/t7600-merge.sh | 9 ++++++ 3 files changed, 50 insertions(+), 63 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index d2b736d4e6..fa3deeecbe 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id, return ret; } -static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit) -{ - struct child_process cp = CHILD_PROCESS_INIT; - const char *w_commit_hex = oid_to_hex(w_commit); - - /* - * Diff-tree would not be very hard to replace with a native function, - * however it should be done together with apply_cached. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL); - strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex); - - return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); -} - -static int apply_cached(struct strbuf *out) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Apply currently only reads either from stdin or a file, thus - * apply_all_patches would have to be updated to optionally take a - * buffer. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "apply", "--cached", NULL); - return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0); -} - -static int reset_head(void) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Reset is overall quite simple, however there is no current public - * API for resetting. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL); - - return run_command(&cp); -} - static int is_path_a_directory(const char *path) { /* @@ -674,29 +630,30 @@ static enum stash_apply_result do_apply_stash(const char *prefix, oideq(&c_tree, &info->i_tree)) { has_index = 0; } else { - struct strbuf out = STRBUF_INIT; + struct merge_result result = { 0 }; - if (diff_tree_binary(&out, &info->w_commit)) { - strbuf_release(&out); - return error(_("could not generate diff %s^!."), - oid_to_hex(&info->w_commit)); - } + o.branch1 = "Current index"; + o.branch2 = "Stashed index changes"; + o.ancestor = "Stash base"; - ret = apply_cached(&out); - strbuf_release(&out); - if (ret) + head = lookup_tree(o.repo, &c_tree); + merge = lookup_tree(o.repo, &info->i_tree); + merge_base = lookup_tree(o.repo, &info->b_tree); + + merge_incore_nonrecursive(&o, merge_base, head, merge, + &result); + + 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.")); - - discard_index(the_repository->index); - repo_read_index(the_repository); - if (write_index_as_tree(&index_tree, the_repository->index, - repo_get_index_file(the_repository), 0, NULL)) - return error(_("could not save index tree")); - - reset_head(); - discard_index(the_repository->index); - repo_read_index(the_repository); + } else { + oidcpy(&index_tree, &result.tree->object.oid); + merge_finalize(&o, &result); + } } } diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 70af58e161..70c6031958 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -395,6 +395,27 @@ 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 && diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index 64fe21717d..8f6109fb91 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -801,6 +801,15 @@ verify_no_mergehead () { test_cmp result.1-5 file ' +test_expect_success 'fast-forward merge with --autostash, stash.index' ' + git reset --hard c0 && + git stash clear && + echo staged >>z && git add z && + git -c stash.index=true merge --autostash c1 2>err && + test_grep "Applied autostash." err && + test_stdout_line_count = 0 git stash list +' + test_expect_success 'failed fast-forward merge with --autostash' ' git reset --hard c0 && git merge-file file file.orig file.5 && -- 2.56.0.rc1.315.gc6ed9934b7.dirty ``` ## D. Ben Knoble, 2026-09-30 21:26 Subject: Re: [PATCH v4 0/5] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` On Tue, Sep 29, 2026 at 1:32 PM Ben Knoble wrote: > > > > Le 29 sept. 2026 à 11:48, Phillip Wood a écrit : > > > > Hi Ben > > > >> On 29/09/2026 13:18, D. Ben Knoble wrote: > >> 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 ;) > > > > I've left a comment on the new patch 4, but everything else in the range-diff looks ready to me. > > > > Thanks > > > > Phillip > > Thanks Phillip. Pending other positive acks, I’m not sure if I should reroll with Thomas’s new patch, reroll dropping it now there’s a seen topic for it, or just wait ;) > > I’ll probably wait a bit and see how the dust settles, but: > > Junio if you want to see a reroll hit the list using the new synthetic base to make things nicer for you, I can do so. In particular, I think the last check I made when I saw your mail about the synthetic base had the prior round. I realized Junio wasn't CC'd on the prior mail, but since I re-rolled and the merge base changed, I think I've got it right for v5, which just went out. -- D. Ben Knoble ``` ## Phillip Wood, 2026-10-01 15:52 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 ``` ## Junio C Hamano, 2026-10-01 17:47 Subject: Re: [PATCH v5 0/4] stash: clean up index-mode test merge Message-ID: In-Reply-To: ``` Phillip Wood writes: > 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 for writing and reviewing. These now look very good to me too. Let me mark them for 'next'. ```