Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Sep 19, 2026, 15:22 UTC
- Message-ID
- <9ae2cb8a-7f2d-4c79-937b-170b8937bd8c@gmail.com>
- In-Reply-To
- <44881557-f15e-4ec5-b1c5-4112752f757c@gmail.com>
On 17/09/2026 16:55, Phillip Wood wrote:
Show 29 quoted lines
> 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
Show 41 quoted lines
>>>>> 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 >> >