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

On 17/09/2026 14:15, D. Ben Knoble wrote:

Ok, here we go.

Excellent!

On Thu, Sep 17, 2026 at 5:24 AM Phillip Wood [off-list ref] wrote:

quoted

On 16/09/2026 15:30, Ben Knoble wrote:

quoted

quoted

Le 16 sept. 2026 à 09:35, Phillip Wood [off-list ref] 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?

Yep - because we're forking another git process that updates the index we need to read the index file again when that process exits.

Followed by
write_index_as_tree(&index_tree)… so that must be the "save the
results of applying the stashed changes" part.

Yep

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…

quoted

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?

Yes, b_tree is (H) and it cherry-picks the stashed working tree changes on to the current worktree. c_tree is the current index at that point.
 > - [skipping ahead] in the index case, we reset_tree(&index_tree, 0,

0), restoring the stashed index changes.

Yes

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.

Apart from the "git read-tree" man page, the documentation for unpack_trees() is basically non-existent which is a real shame for such a fundamental function. As I understand it, the one-way merge copies across the stat data from the old index to the new index for entries that are unchanged between the two. Without the "reset" bit it also checks that there are no unmerged index entries and unstaged changes for paths that are removed by the merge.

It's extra unclear what the reset bit is applied to in

quoted

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.

Oh, I wonder what's happening there.

quoted

quoted

quoted

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

I had a quick look at the patch, it looks good, but I think we can simplify it a bit. As we abort if there are conflicts I don't think we need to spend any effort setting the conflict labels (I'm not sure if we can pass NULL, but "" would certainly suffice). Does the current code print any errors from apply when the patch does not apply? If not we should silence the merge by setting verbosity=0. Also I think we should use oidcpy to copy the merged tree (it probably does not matter in this case, but it I think it does some extra checks on the hash function which a simple assignment does not)

Thanks for working on this, it will be a nice improvement

Phillip

[*] namely, the log message, some tiny first cleanups, and removing
now-unused functions