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

From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Sep 19, 2026, 20:02 UTC
Message-ID: <CALnO6CBJoiu8Xzx_p9wYYUrR6ZxB93B8-su1Pxhr8WJ+68va1g@mail.gmail.com>
In-Reply-To: <44881557-f15e-4ec5-b1c5-4112752f757c@gmail.com>

On Thu, Sep 17, 2026 at 11:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

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

[snip]

Show 11 quoted lines

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

Up to here, I'm with you.

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

That is such a precise check to be covered by only "reset", heh. I suppose the idea is that if "reset" is on, we don't need to check because we'll reset those entries/paths? More documentation from experts in this area welcome…

Show 11 quoted lines

> > 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 :/
> >

[snip]

> >
> > It _also_ doesn't make the bug go away, hm.
>
> Oh, I wonder what's happening there.

Looks like you figured it out :) When I was thinking about a series, I thought it might be nice to do the "trivial" fix first, then the refactor we prefer, but… I'm not too sure. Since the other version works, I might just go with it and omit the intermediate reset_tree state.

Show 6 quoted lines

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

Actually, this was my reason to set the labels, too! I thought they might show up in any logged messages. I'll take another look at what happens here.

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

Great idea, thanks.

-- 
D. Ben Knoble