From: D. Ben Knoble Date: Thu, 17 Sep 2026 13:15:09 GMT Subject: Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward Message-ID: In-Reply-To: Ok, here we go. On Thu, Sep 17, 2026 at 5:24 AM Phillip Wood wrote: > > Hi Ben > > On 16/09/2026 15:30, Ben Knoble wrote: > >> Le 16 sept. 2026 à 09:35, Phillip Wood a écrit : > >> Taking a step back, this code applies the stashed index changes > >> into the current index, writes the result to a tree and then resets > >> the index to HEAD. I noted that we save the current index (?) into c_tree with write_index_as_tree(), then feed diff_tree_binary() into git apply --cache, aka applying the stashed index changes. [from my notes] - that is, diff H..I in the diagram from the manual: A stash entry is represented as a commit whose tree records the state of the working directory, and its first parent is the commit at HEAD when the entry was created. The tree of the second parent records the state of the index when the entry is made, and it is made a child of the HEAD commit. The ancestry graph looks like this: .----W / / -----H----I where H is the HEAD commit, I is a commit that records the state of the index, and W is a commit that records the state of the working tree. We then have a discard_index()/repo_read_index() pair, which I assume is refreshing the in-memory index? Followed by write_index_as_tree(&index_tree)… so that must be the "save the results of applying the stashed changes" part. What I totally misunderstood was "reset the index to HEAD"---of course that's what "git reset" does! But I didn't understand why until… > It is a bit confusing the way it updates the index, then resets it only > to update it again at the end. I don't think we can avoid that though if > we want to error out when there are conflicts merging the index. …which now makes (some) sense. That also explains why my attempts to use reset_working_tree() in various forms could never work :) I had the wrong idea entirely. But it seemed in my debugging like something was touching the working tree, so I wish I had kept better notes. Just finishing up the flow: - we then refresh the in-memory index again (we just reset the on-disk index to HEAD) - we continue on with a "normal" stash apply merge of c_tree (old index), w_tree (W), and b_tree (which I assume is H?); this applies the stashed working tree changes on the current state? - [skipping ahead] in the index case, we reset_tree(&index_tree, 0, 0), restoring the stashed index changes. Sans doc comments for unpack_trees() beyond "N-way merge len trees […] resulting index […]", I haven't puzzled out what's going on, but it _looks_ like we merge a single tree, possibly with the original index (opts.src_index) and write to a destination which is the repository's index. It's extra unclear what the reset bit is applied to in > It looks like stash has its own unpack_trees() wrapper, so I think the > simplest fix is to replace reset_head() with > > reset_tree(&c_tree, 0, 1); I'm not sure if that is a pre-merge reset, post-merge reset, or something in between. Unfortunately, a whole bunch of tests fail (6 files) with this suggestion :/ Summary of Failures: 339/1062 git:t3904-stash-patch ERROR 0.45s exit status 1 503/1062 git:t3903-stash ERROR 4.40s exit status 1 763/1062 git:t6424-merge-unrelated-index-changes ERROR 0.90s exit status 1 778/1062 git:t6402-merge-rename ERROR 2.06s exit status 1 864/1062 git:t1092-sparse-checkout-compatibility ERROR 26.19s exit status 1 868/1062 git:t7611-merge-abort ERROR 0.45s exit status 1 It _also_ doesn't make the bug go away, hm. > >> We could avoid touching the index at all if we > >> used merge_incore_nonrecursive() to cherry pick the index changes > >> instead. That way we'd get a proper three-way merge and avoid > >> spawning subprocesses for "git diff-tree", "git apply --cached", > >> and "git reset". We're already using merge_ort_nonrecursive() to > >> merge the working tree changes in that function so we have nearly > >> everything we need already set up to merge the index changes as > >> well. Essentially, when merging the index, we just need to call > >> merge_incore_nonrecursive() instead of merge_ort_nonrecursive() > >> and use info->i_tree instead of info->w_tree. If I'm following this, the suggestion is to replace (parts of) the early "if (index)" block with a merge_incore_nonrecursive() to merge index changes, reporting conflicts as we do today, and saving the tree for later… and this would not touch the real index, so we wouldn't have to reset at all? Interesting! The attached patch [Gmail headaches, sorry], which needs some polishing [*], passes tests and fixes the bug! Yahoo. I'll send a series later, tomorrow probably. (It won't apply directly, because it's on top of the experimental reset_tree() version, but resolving conflicts should be easy.) [*] namely, the log message, some tiny first cleanups, and removing now-unused functions -- D. Ben Knoble From d53a04217dd2c8b967e71717a020cff1c8668fa1 Mon Sep 17 00:00:00 2001 Message-ID: From: "D. Ben Knoble" Date: Thu, 17 Sep 2026 09:12:50 -0400 Subject: [PATCH] wip: fix stash bug Signed-off-by: D. Ben Knoble --- builtin/stash.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index b0895687d9..87d764ee32 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -655,29 +655,36 @@ 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_ui_merge_options(&o, the_repository); - ret = apply_cached(&out); - strbuf_release(&out); - if (ret) + o.branch1 = label_ours ? label_ours : "Updated upstream"; + o.branch2 = "Stashed index changes"; + o.ancestor = label_base ? label_base : "Stash base"; + + 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); + + 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_tree(&c_tree, 0, 1); - discard_index(the_repository->index); - repo_read_index(the_repository); + index_tree = result.tree->object.oid; } } base-commit: 339ab2a8f14c0c304ae2f28df1a859f3d2cf610c prerequisite-patch-id: 74b6847e4daaa8814f5975ce3d077bede199504b prerequisite-patch-id: a2465f4d108ada71b84c8a87e3506dc3e3b52683 -- 2.55.0.1003.g10538fe699.dirty