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

[Phillip Wood <phillip.wood123@gmail.com>]

On 17/09/2026 16:55, Phillip Wood wrote:
> On 17/09/2026 14:15, D. Ben Knoble wrote:
>>> 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.

So the tests fail because when git tries to merge the stashed worktree 
changes it thinks there are unstaged changes in the worktree. That's 
because when we merge the stashed index changes the stat data and 
CE_UPTODATE flag are cleared for paths that are updated by the merge. 
When we remove those merged changes from the index we need to refresh 
the index to restore the stat data. Adding a call to refresh_index() 
after reset_tree() makes the tests pass.

Anyway that's all a bit irrelevant if we're going to start using 
merge_incore_nonrecursive() but the test failures were bugging me so I 
thought I'd have a look at what was causing them.

Thanks

Phillip


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