Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward

> On Thu, Sep 17, 2026 at 5:24 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 16/09/2026 15:30, Ben Knoble wrote:
>>>> Le 16 sept. 2026 à 09:35, Phillip Wood <phillip.wood123@gmail.com> 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? 

> 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?

> 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.)