{"thread":{"id":"66281","subject":"[BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","startedAt":"2026-09-07T04:45:08Z","lastAt":"2026-09-19T21:32:22Z","messageCount":12,"participants":["Eli Barzilay","D. Ben Knoble","Phillip Wood","Ben Knoble"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"552086","messageId":"CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com","threadId":"66281","inReplyTo":null,"subject":"[BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"Eli Barzilay","fromEmail":"eli@barzilay.org","sentAt":"2026-09-07T04:44:23Z","receivedAt":"2026-09-07T04:45:08Z","isPatch":false,"body":"Disclaimer, the following is written by an agent, but the bug is a\nreal problem that I have.\n\n\nWith stash.index=true, an autostash that is applied successfully is\nnevertheless stored as a stash entry, and deleting MERGE_AUTOSTASH\nfails. A staged change at the time of the merge is required to\ntrigger it.\n\nReproduction (independent of the reporter's configuration):\n\n #!/bin/sh\n set -e\n export GIT_AUTHOR_NAME=A GIT_AUTHOR_EMAIL=a@b \\\n GIT_COMMITTER_NAME=A GIT_COMMITTER_EMAIL=a@b\n rm -rf /tmp/gitbug && mkdir /tmp/gitbug && cd /tmp/gitbug\n git init -q -b main up\n cd up && echo a >u && echo z >z && git add . &&\n git commit -qm base && cd ..\n git clone -q up dn\n cd up && echo more >>u && git commit -qam up2 && cd ../dn\n echo staged >>z && git add z # a STAGED change is required\n git fetch -q origin\n git -c stash.index=true merge --ff-only --autostash origin/main\n echo \"--- git stash list:\"; git stash list\n\nActual output:\n\n Updating 34a5e40..84ccd9d\n Created autostash: 71d4617\n Fast-forward\n u | 1 +\n 1 file changed, 1 insertion(+)\n Applied autostash.\n error: cannot lock ref 'MERGE_AUTOSTASH': unable to resolve\nreference 'MERGE_AUTOSTASH'\n --- git stash list:\n stash@{0}: autostash\n\nExpected: the same without the error and with an empty stash list, as\nhappens with stash.index=false (the only change to the script).\n\nThe autostash is applied correctly, so nothing is lost: the leftover\nentry duplicates what is already in the working tree and index. The\ncommand also exits 0, so the error is easy to miss, and the entries\naccumulate one per merge.\n\nReported via `git merge` above for brevity, but the common way to meet\nthis is `git pull` with rebase.autoStash and pull.rebase set: when the\npull can fast-forward, builtin/pull.c:1164 hands off to\n`git merge --ff-only --autostash` rather than to rebase. Every such\npull with something staged leaves an entry behind.\n\nAnalysis\n--------\n\nMerge keeps its autostash in the MERGE_AUTOSTASH ref\n(builtin/merge.c:1675) and applies it from finish()\n(builtin/merge.c:540). apply_save_autostash_ref() resolves the ref,\napplies it, and then deletes it (sequencer.c:4821-4848).\n\nThe apply is a child process, `git stash apply `\n(sequencer.c:4737-4751). stash.index turns that into an --index\napply, which takes the index-restoring branch of do_apply_stash() and\ncalls reset_head() (builtin/stash.c:684-691), i.e. a\n`git reset --quiet --refresh` child (builtin/stash.c:455-467).\n\nThat reset has no pathspec, so it calls remove_branch_state()\n(builtin/reset.c:543) -> remove_merge_branch_state()\n(branch.c:829-838), whose last statement is\n\n save_autostash_ref(r, \"MERGE_AUTOSTASH\");\n\nwhich stores the autostash into refs/stash and deletes the ref -- in\nthe middle of the very apply that was about to consume it. Control\nreturns to apply_save_autostash_ref(), the apply reports success\n(\"Applied autostash.\"), and its refs_delete_ref() then fails on a ref\nthat is already gone, producing the error line.\n\nThe child's own explanation, \"Autostash exists; creating a new stash\nentry.\" (sequencer.c:4775-4779), never reaches the user because the\nparent mutes the apply child's output (sequencer.c:4736-4738).\n\nA staged change is required because with a clean index the stash's\nbase and index trees are equal, has_index is cleared\n(builtin/stash.c:665-668), and reset_head() is never reached.\n\nThe rebase backend is unaffected: it keeps its autostash in the file\n.git/rebase-merge/autostash, which remove_merge_branch_state() does\nnot touch. Only the merge (ref-based) autostash is exposed.\n\nPossible directions, in case they are useful: remove_merge_branch_state()\nis about ending a merge, and `git stash apply --index` is not ending\none -- having stash's reset_head() avoid the branch-state cleanup, or\nteaching an in-flight autostash apply to shield MERGE_AUTOSTASH, would\nboth close it. Making apply_save_autostash_ref() tolerate a missing\nref would silence the error but leave the duplicate entry.\n\nVersions\n--------\n\nSeen with git 2.55.0 on Linux (WSL2). The code paths above are\nunchanged on master as of 2026-09-07.\n\n-- \n ((x=>x(x))(x=>x(x))) Eli Barzilay:\n http://barzilay.org/ Maze is Life!\n"},{"id":"552770","messageId":"CALnO6CCkq7mjBUKxOYcwKX8=SrH441FuWopoGZutPk99JRTGUA@mail.gmail.com","threadId":"66281","inReplyTo":"CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-15T21:16:30Z","receivedAt":"2026-09-15T21:16:44Z","isPatch":false,"body":"Hi Eli,\n\nSince I added stash.index configuration, thought I'd take a look. I'm\nout of my depth, but let's persevere anyway!\n\nOn Mon, Sep 7, 2026 at 12:47 AM Eli Barzilay wrote:\n>\n> Disclaimer, the following is written by an agent, but the bug is a\n> real problem that I have.\n>\n>\n> With stash.index=true, an autostash that is applied successfully is\n> nevertheless stored as a stash entry, and deleting MERGE_AUTOSTASH\n> fails. A staged change at the time of the merge is required to\n> trigger it.\n>\n> Reproduction (independent of the reporter's configuration):\n>\n> #!/bin/sh\n> set -e\n> export GIT_AUTHOR_NAME=A GIT_AUTHOR_EMAIL=a@b \\\n> GIT_COMMITTER_NAME=A GIT_COMMITTER_EMAIL=a@b\n> rm -rf /tmp/gitbug && mkdir /tmp/gitbug && cd /tmp/gitbug\n> git init -q -b main up\n> cd up && echo a >u && echo z >z && git add . &&\n> git commit -qm base && cd ..\n> git clone -q up dn\n> cd up && echo more >>u && git commit -qam up2 && cd ../dn\n> echo staged >>z && git add z # a STAGED change is required\n> git fetch -q origin\n> git -c stash.index=true merge --ff-only --autostash origin/main\n> echo \"--- git stash list:\"; git stash list\n>\n> Actual output:\n>\n> Updating 34a5e40..84ccd9d\n> Created autostash: 71d4617\n> Fast-forward\n> u | 1 +\n> 1 file changed, 1 insertion(+)\n> Applied autostash.\n> error: cannot lock ref 'MERGE_AUTOSTASH': unable to resolve\n> reference 'MERGE_AUTOSTASH'\n> --- git stash list:\n> stash@{0}: autostash\n>\n> Expected: the same without the error and with an empty stash list, as\n> happens with stash.index=false (the only change to the script).\n\nI can reproduce this locally. Thanks for the helpful script. For some\nextra tweaking, I've put a \"PATH=…:$PATH\" assignment at the top that\nprepends my local Git build's bin-wrappers, then put \"GIT_DEBUGGER=$1\nGIT_TRACE2=$2\" in front of the merge command; that way I can debug a\nfew things.\n\n> Analysis\n> --------\n>\n> Merge keeps its autostash in the MERGE_AUTOSTASH ref\n> (builtin/merge.c:1675) and applies it from finish()\n> (builtin/merge.c:540). apply_save_autostash_ref() resolves the ref,\n> applies it, and then deletes it (sequencer.c:4821-4848).\n>\n> The apply is a child process, `git stash apply `\n> (sequencer.c:4737-4751). stash.index turns that into an --index\n> apply, which takes the index-restoring branch of do_apply_stash() and\n> calls reset_head() (builtin/stash.c:684-691), i.e. a\n> `git reset --quiet --refresh` child (builtin/stash.c:455-467).\n>\n> That reset has no pathspec, so it calls remove_branch_state()\n> (builtin/reset.c:543) -> remove_merge_branch_state()\n> (branch.c:829-838), whose last statement is\n>\n> save_autostash_ref(r, \"MERGE_AUTOSTASH\");\n>\n> which stores the autostash into refs/stash and deletes the ref -- in\n> the middle of the very apply that was about to consume it. Control\n> returns to apply_save_autostash_ref(), the apply reports success\n> (\"Applied autostash.\"), and its refs_delete_ref() then fails on a ref\n> that is already gone, producing the error line.\n\nAnd this lines up with the code, I think. I find it a bit odd that\n\"git stash apply --index\" ends up getting to a reset mode that tries\nto throw away a bunch of branch state!\n\nIdeally that would be simpler, I think, but I don't see an easy way to\ndo it with the existing \"git reset\" subprocess.\n\nI'm experimenting with something that swaps that out for a call to\nreset_working_tree(), but I don't think I've gotten it quite right for\nthis bug yet (let alone run other test cases that might be affected by\nthis change).\n\nBTW, it's really weird to me that the reset manual doesn't mention all\nthese \"extra\" cleanups reset does via remove_merge_branch_state()!\n\n> Possible directions, in case they are useful: remove_merge_branch_state()\n> is about ending a merge, and `git stash apply --index` is not ending\n> one -- having stash's reset_head() avoid the branch-state cleanup, or\n> teaching an in-flight autostash apply to shield MERGE_AUTOSTASH, would\n> both close it. Making apply_save_autostash_ref() tolerate a missing\n> ref would silence the error but leave the duplicate entry.\n\nI also thought briefly about disabling stash.index for a merge\nautostash, but that's really papering over things, I think.\n\nI'll keep noodling on this (hopefully tomorrow morning), but in the\nmeantime input from others welcome :)\n\n-- \nD. Ben Knoble\n"},{"id":"552785","messageId":"56991232-5d16-41d1-9c7d-ca7ebdd9fce7@gmail.com","threadId":"66281","inReplyTo":"CALnO6CCkq7mjBUKxOYcwKX8=SrH441FuWopoGZutPk99JRTGUA@mail.gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-16T13:35:26Z","receivedAt":"2026-09-16T13:35:35Z","isPatch":false,"body":"Hi Ben\n\nOn 15/09/2026 22:16, D. Ben Knoble wrote:\n> \n> I'm experimenting with something that swaps that out for a call to\n> reset_working_tree(), but I don't think I've gotten it quite right for\n> this bug yet (let alone run other test cases that might be affected by\n> this change).\n\nIt looks like stash has its own unpack_trees() wrapper, so I think the \nsimplest fix is to replace reset_head() with\n\n\treset_tree(&c_tree, 0, 1);\n\nTaking a step back, this code applies the stashed index changes into the \ncurrent index, writes the result to a tree and then resets the index to \nHEAD. We could avoid touching the index at all if we used \nmerge_incore_nonrecursive() to cherry pick the index changes instead. \nThat way we'd get a proper three-way merge and avoid spawning \nsubprocesses for \"git diff-tree\", \"git apply --cached\", and \"git reset\". \nWe're already using merge_ort_nonrecursive() to merge the working tree \nchanges in that function so we have nearly everything we need already \nset up to merge the index changes as well. Essentially, when merging the \nindex, we just need to call merge_incore_nonrecursive() instead of \nmerge_ort_nonrecursive() and use info->i_tree instead of info->w_tree.\n\n> BTW, it's really weird to me that the reset manual doesn't mention all\n> these \"extra\" cleanups reset does via remove_merge_branch_state()!\n\nAgreed, I think it comes from \"git foo --abort\" calling \"git reset \n(--merge|--hard)\" though that doesn't really explain why a mixed reset \nalso removes the branch state.\n\nThanks\n\nPhillip\n>> Possible directions, in case they are useful: remove_merge_branch_state()\n>> is about ending a merge, and `git stash apply --index` is not ending\n>> one -- having stash's reset_head() avoid the branch-state cleanup, or\n>> teaching an in-flight autostash apply to shield MERGE_AUTOSTASH, would\n>> both close it. Making apply_save_autostash_ref() tolerate a missing\n>> ref would silence the error but leave the duplicate entry.\n> \n> I also thought briefly about disabling stash.index for a merge\n> autostash, but that's really papering over things, I think.\n> \n> I'll keep noodling on this (hopefully tomorrow morning), but in the\n> meantime input from others welcome :)\n> \n\n"},{"id":"552788","messageId":"0BCA251B-9536-46E3-A6C5-7F917366F92D@gmail.com","threadId":"66281","inReplyTo":"56991232-5d16-41d1-9c7d-ca7ebdd9fce7@gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-16T14:30:30Z","receivedAt":"2026-09-16T14:30:44Z","isPatch":false,"body":"Hi Phillip,\n\n> Le 16 sept. 2026 à 09:35, Phillip Wood a écrit :\n> \n> Hi Ben\n> \n>> On 15/09/2026 22:16, D. Ben Knoble wrote:\n>> I'm experimenting with something that swaps that out for a call to\n>> reset_working_tree(), but I don't think I've gotten it quite right for\n>> this bug yet (let alone run other test cases that might be affected by\n>> this change).\n> \n> It looks like stash has its own unpack_trees() wrapper, so I think the simplest fix is to replace reset_head() with\n> \n> reset_tree(&c_tree, 0, 1);\n> \n> 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. 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.\n\nWow, I wish I’d had this info this morning! I spent a couple hours trying to understand this flow and still don’t have it in my head :) Thanks for the pointers.\n\nWith the way I batch my side project time, it’ll be tomorrow before I get to trying to make and test patches for this, but I’ excited now.\n\nI may try to summarize my own notes (= questions about the existing code) and send those out later today, though, since I’d love to make my understanding line up with yours!\n\n>> BTW, it's really weird to me that the reset manual doesn't mention all\n>> these \"extra\" cleanups reset does via remove_merge_branch_state()!\n> \n> Agreed, I think it comes from \"git foo --abort\" calling \"git reset (--merge|--hard)\" though that doesn't really explain why a mixed reset also removes the branch state.\n\nYeah, that abort bit makes sense. I wonder if we should have had a better side-channel for communicating that, but I’m a bit too afraid to touch that for now ;)\n\n> Thanks\n> \n> Phillip\n\nThank *you*!"},{"id":"552798","messageId":"CALO-guua8fcRq5n_M8=r9GMZ-aW4LaddXxS9rz0YhpAQ9TL3mA@mail.gmail.com","threadId":"66281","inReplyTo":"0BCA251B-9536-46E3-A6C5-7F917366F92D@gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"Eli Barzilay","fromEmail":"eli@barzilay.org","sentAt":"2026-09-16T19:57:37Z","receivedAt":"2026-09-16T19:57:51Z","isPatch":false,"body":"[Note from the peanut gallery since I can't spend time diving into the\ncode -- that's result of collecting dead autostashes is the only thorn\nin my joy of discovering `stash.index` + `rebase.autoStash`. So while\nI can't spend time in the code, I'll be happy to try patches or\nwhatever if it helps...]\n\nOn Wed, Sep 16, 2026 at 10:30 AM Ben Knoble wrote:\n>\n> Hi Phillip,\n>\n> > Le 16 sept. 2026 à 09:35, Phillip Wood a écrit :\n> >\n> > Hi Ben\n> >\n> >> On 15/09/2026 22:16, D. Ben Knoble wrote:\n> >> I'm experimenting with something that swaps that out for a call to\n> >> reset_working_tree(), but I don't think I've gotten it quite right for\n> >> this bug yet (let alone run other test cases that might be affected by\n> >> this change).\n> >\n> > It looks like stash has its own unpack_trees() wrapper, so I think the simplest fix is to replace reset_head() with\n> >\n> > reset_tree(&c_tree, 0, 1);\n> >\n> > 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. 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.\n>\n> Wow, I wish I’d had this info this morning! I spent a couple hours trying to understand this flow and still don’t have it in my head :) Thanks for the pointers.\n>\n> With the way I batch my side project time, it’ll be tomorrow before I get to trying to make and test patches for this, but I’ excited now.\n>\n> I may try to summarize my own notes (= questions about the existing code) and send those out later today, though, since I’d love to make my understanding line up with yours!\n>\n> >> BTW, it's really weird to me that the reset manual doesn't mention all\n> >> these \"extra\" cleanups reset does via remove_merge_branch_state()!\n> >\n> > Agreed, I think it comes from \"git foo --abort\" calling \"git reset (--merge|--hard)\" though that doesn't really explain why a mixed reset also removes the branch state.\n>\n> Yeah, that abort bit makes sense. I wonder if we should have had a better side-channel for communicating that, but I’m a bit too afraid to touch that for now ;)\n>\n> > Thanks\n> >\n> > Phillip\n>\n> Thank *you*!\n\n\n\n-- \n ((x=>x(x))(x=>x(x))) Eli Barzilay:\n http://barzilay.org/ Maze is Life!\n"},{"id":"552808","messageId":"fd4c2cc3-d457-49b0-bf3c-96063e40700d@gmail.com","threadId":"66281","inReplyTo":"0BCA251B-9536-46E3-A6C5-7F917366F92D@gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-17T09:24:07Z","receivedAt":"2026-09-17T09:24:13Z","isPatch":false,"body":"Hi Ben\n\nOn 16/09/2026 15:30, Ben Knoble wrote:\n>> Le 16 sept. 2026 à 09:35, Phillip Wood a écrit :\n>> Taking a step back, this code applies the stashed index changes\n>> into the current index, writes the result to a tree and then resets\n>> the index to HEAD. We could avoid touching the index at all if we\n>> used merge_incore_nonrecursive() to cherry pick the index changes\n>> instead. That way we'd get a proper three-way merge and avoid\n>> spawning subprocesses for \"git diff-tree\", \"git apply --cached\",\n>> and \"git reset\". We're already using merge_ort_nonrecursive() to\n>> merge the working tree changes in that function so we have nearly\n>> everything we need already set up to merge the index changes as\n>> well. Essentially, when merging the index, we just need to call\n>> merge_incore_nonrecursive() instead of merge_ort_nonrecursive()\n>> and use info->i_tree instead of info->w_tree.\n\n> \n> Wow, I wish I’d had this info this morning! I spent a couple hours\n > trying to understand this flow and still don’t have it in my head :)\n > Thanks for the pointers.\nIt is a bit confusing the way it updates the index, then resets it only \nto update it again at the end. I don't think we can avoid that though if \nwe want to error out when there are conflicts merging the index.\n\n> I may try to summarize my own notes (= questions about the existing\n > code) and send those out later today, though, since I’d love to make\n > my understanding line up with yours!\nI'm happy to try and answer any questions, on or off the list - it would \nbe really nice if we can merge the index changes and avoid a bunch a \nsubprocesses.\n\nThanks\n\nPhillip\n\n"},{"id":"552810","messageId":"CALnO6CCtACSLmkCm77=rY40+90f_3OTuGKg30AVORtQk2+=gMg@mail.gmail.com","threadId":"66281","inReplyTo":"CALO-guua8fcRq5n_M8=r9GMZ-aW4LaddXxS9rz0YhpAQ9TL3mA@mail.gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-17T12:04:39Z","receivedAt":"2026-09-17T12:04:55Z","isPatch":false,"body":"On Wed, Sep 16, 2026 at 3:57 PM Eli Barzilay wrote:\n>\n> [Note from the peanut gallery since I can't spend time diving into the\n> code -- that's result of collecting dead autostashes is the only thorn\n> in my joy of discovering `stash.index` + `rebase.autoStash`. So while\n> I can't spend time in the code, I'll be happy to try patches or\n> whatever if it helps...]\n\nThanks, I'll keep you on CC for patches as well :)\n"},{"id":"552812","messageId":"CALnO6CDfwscMWZktBu3FtXOQVbcBRo76nqK07kMnrzC5cPyZiQ@mail.gmail.com","threadId":"66281","inReplyTo":"fd4c2cc3-d457-49b0-bf3c-96063e40700d@gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-17T13:15:09Z","receivedAt":"2026-09-17T13:15:25Z","isPatch":false,"body":"Ok, here we go.\n\nOn Thu, Sep 17, 2026 at 5:24 AM Phillip Wood wrote:\n>\n> Hi Ben\n>\n> On 16/09/2026 15:30, Ben Knoble wrote:\n> >> Le 16 sept. 2026 à 09:35, Phillip Wood a écrit :\n> >> Taking a step back, this code applies the stashed index changes\n> >> into the current index, writes the result to a tree and then resets\n> >> the index to HEAD.\n\nI noted that we save the current index (?) into c_tree with\nwrite_index_as_tree(), then feed diff_tree_binary() into git apply\n--cache, aka applying the stashed index changes.\n\n[from my notes]\n - that is, diff H..I in the diagram from the manual:\n\n A stash entry is represented as a commit whose tree records the state\n of the working directory, and its first parent is the commit at HEAD\n when the entry was created. The tree of the second parent records the\n state of the index when the entry is made, and it is made a child of\n the HEAD commit. The ancestry graph looks like this:\n\n .----W\n / /\n -----H----I\n\n where H is the HEAD commit, I is a commit that records the state of the\n index, and W is a commit that records the state of the working tree.\n\nWe then have a discard_index()/repo_read_index() pair, which I assume\nis refreshing the in-memory index? Followed by\nwrite_index_as_tree(&index_tree)… so that must be the \"save the\nresults of applying the stashed changes\" part.\n\nWhat I totally misunderstood was \"reset the index to HEAD\"---of course\nthat's what \"git reset\" does! But I didn't understand why until…\n\n> It is a bit confusing the way it updates the index, then resets it only\n> to update it again at the end. I don't think we can avoid that though if\n> we want to error out when there are conflicts merging the index.\n\n…which now makes (some) sense. That also explains why my attempts to\nuse reset_working_tree() in various forms could never work :) I had\nthe wrong idea entirely. But it seemed in my debugging like something\nwas touching the working tree, so I wish I had kept better notes.\n\nJust finishing up the flow:\n\n- we then refresh the in-memory index again (we just reset the on-disk\nindex to HEAD)\n- we continue on with a \"normal\" stash apply merge of c_tree (old\nindex), w_tree (W), and b_tree (which I assume is H?); this applies\nthe stashed working tree changes on the current state?\n- [skipping ahead] in the index case, we reset_tree(&index_tree, 0,\n0), restoring the stashed index changes. Sans doc comments for\nunpack_trees() beyond \"N-way merge len trees […] resulting index […]\",\nI haven't puzzled out what's going on, but it _looks_ like we merge a\nsingle tree, possibly with the original index (opts.src_index) and\nwrite to a destination which is the repository's index.\n\nIt's extra unclear what the reset bit is applied to in\n\n> It looks like stash has its own unpack_trees() wrapper, so I think the\n> simplest fix is to replace reset_head() with\n>\n> reset_tree(&c_tree, 0, 1);\n\nI'm not sure if that is a pre-merge reset, post-merge reset, or\nsomething in between.\nUnfortunately, a whole bunch of tests fail (6 files) with this suggestion :/\n\nSummary of Failures:\n\n 339/1062 git:t3904-stash-patch ERROR\n 0.45s exit status 1\n 503/1062 git:t3903-stash ERROR\n 4.40s exit status 1\n 763/1062 git:t6424-merge-unrelated-index-changes ERROR\n 0.90s exit status 1\n 778/1062 git:t6402-merge-rename ERROR\n 2.06s exit status 1\n 864/1062 git:t1092-sparse-checkout-compatibility ERROR\n 26.19s exit status 1\n 868/1062 git:t7611-merge-abort ERROR\n 0.45s exit status 1\n\nIt _also_ doesn't make the bug go away, hm.\n\n> >> We could avoid touching the index at all if we\n> >> used merge_incore_nonrecursive() to cherry pick the index changes\n> >> instead. That way we'd get a proper three-way merge and avoid\n> >> spawning subprocesses for \"git diff-tree\", \"git apply --cached\",\n> >> and \"git reset\". We're already using merge_ort_nonrecursive() to\n> >> merge the working tree changes in that function so we have nearly\n> >> everything we need already set up to merge the index changes as\n> >> well. Essentially, when merging the index, we just need to call\n> >> merge_incore_nonrecursive() instead of merge_ort_nonrecursive()\n> >> and use info->i_tree instead of info->w_tree.\n\nIf I'm following this, the suggestion is to replace (parts of) the\nearly \"if (index)\" block with a merge_incore_nonrecursive() to merge\nindex changes, reporting conflicts as we do today, and saving the tree\nfor later… and this would not touch the real index, so we wouldn't\nhave to reset at all? Interesting!\n\nThe attached patch [Gmail headaches, sorry], which needs some\npolishing [*], passes tests and fixes the bug! Yahoo. I'll send a\nseries later, tomorrow probably.\n(It won't apply directly, because it's on top of the experimental\nreset_tree() version, but resolving conflicts should be easy.)\n\n[*] namely, the log message, some tiny first cleanups, and removing\nnow-unused functions\n\n-- \nD. Ben Knoble\n\n\nFrom d53a04217dd2c8b967e71717a020cff1c8668fa1 Mon Sep 17 00:00:00 2001\nMessage-ID: \nFrom: \"D. Ben Knoble\" \nDate: Thu, 17 Sep 2026 09:12:50 -0400\nSubject: [PATCH] wip: fix stash bug\n\nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 43 +++++++++++++++++++++++++------------------\n 1 file changed, 25 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex b0895687d9..87d764ee32 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -655,29 +655,36 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t oideq(&c_tree, &info->i_tree)) {\n \t\t\thas_index = 0;\n \t\t} else {\n-\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\tstruct merge_result result = { 0 };\n \n-\t\t\tif (diff_tree_binary(&out, &info->w_commit)) {\n-\t\t\t\tstrbuf_release(&out);\n-\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n-\t\t\t\t\t oid_to_hex(&info->w_commit));\n-\t\t\t}\n+\t\t\tinit_ui_merge_options(&o, the_repository);\n \n-\t\t\tret = apply_cached(&out);\n-\t\t\tstrbuf_release(&out);\n-\t\t\tif (ret)\n+\t\t\to.branch1 = label_ours ? label_ours : \"Updated upstream\";\n+\t\t\to.branch2 = \"Stashed index changes\";\n+\t\t\to.ancestor = label_base ? label_base : \"Stash base\";\n+\n+\t\t\tif (oideq(&info->b_tree, &c_tree))\n+\t\t\t\to.branch1 = \"Version stash was based on\";\n+\n+\t\t\tif (quiet)\n+\t\t\t\to.verbosity = 0;\n+\n+\t\t\tif (o.verbosity >= 3)\n+\t\t\t\tprintf_ln(_(\"Merging %s with %s\"),\n+\t\t\t\t\t\to.branch1, o.branch2);\n+\n+\t\t\thead = lookup_tree(o.repo, &c_tree);\n+\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n+\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n+\n+\t\t\tmerge_incore_nonrecursive(&o, head, merge, merge_base,\n+\t\t\t\t\t\t &result);\n+\n+\t\t\tif (!result.clean)\n \t\t\t\treturn error(_(\"conflicts in index. \"\n \t\t\t\t\t \"Try without --index.\"));\n \n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n-\t\t\tif (write_index_as_tree(&index_tree, the_repository->index,\n-\t\t\t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n-\t\t\t\treturn error(_(\"could not save index tree\"));\n-\n-\t\t\treset_tree(&c_tree, 0, 1);\n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n+\t\t\tindex_tree = result.tree->object.oid;\n \t\t}\n \t}\n \n\nbase-commit: 339ab2a8f14c0c304ae2f28df1a859f3d2cf610c\nprerequisite-patch-id: 74b6847e4daaa8814f5975ce3d077bede199504b\nprerequisite-patch-id: a2465f4d108ada71b84c8a87e3506dc3e3b52683\n-- \n2.55.0.1003.g10538fe699.dirty\n\n"},{"id":"552817","messageId":"44881557-f15e-4ec5-b1c5-4112752f757c@gmail.com","threadId":"66281","inReplyTo":"CALnO6CDfwscMWZktBu3FtXOQVbcBRo76nqK07kMnrzC5cPyZiQ@mail.gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-17T15:55:24Z","receivedAt":"2026-09-17T15:55:28Z","isPatch":false,"body":"On 17/09/2026 14:15, D. Ben Knoble wrote:\n> Ok, here we go.\n\nExcellent!\n\n> On Thu, Sep 17, 2026 at 5:24 AM Phillip Wood wrote:\n>> On 16/09/2026 15:30, Ben Knoble wrote:\n>>>> Le 16 sept. 2026 à 09:35, Phillip Wood a écrit :\n>>>> Taking a step back, this code applies the stashed index changes\n>>>> into the current index, writes the result to a tree and then resets\n>>>> the index to HEAD.\n> \n> I noted that we save the current index (?) into c_tree with\n> write_index_as_tree(), then feed diff_tree_binary() into git apply\n> --cache, aka applying the stashed index changes.\n> \n> [from my notes]\n> - that is, diff H..I in the diagram from the manual:\n> \n> A stash entry is represented as a commit whose tree records the state\n> of the working directory, and its first parent is the commit at HEAD\n> when the entry was created. The tree of the second parent records the\n> state of the index when the entry is made, and it is made a child of\n> the HEAD commit. The ancestry graph looks like this:\n> \n> .----W\n> / /\n> -----H----I\n> \n> where H is the HEAD commit, I is a commit that records the state of the\n> index, and W is a commit that records the state of the working tree.\n> \n> We then have a discard_index()/repo_read_index() pair, which I assume\n> is refreshing the in-memory index? \n\nYep - because we're forking another git process that updates the index \nwe need to read the index file again when that process exits.\n\n> Followed by\n> write_index_as_tree(&index_tree)… so that must be the \"save the\n> results of applying the stashed changes\" part.\n\nYep\n\n> What I totally misunderstood was \"reset the index to HEAD\"---of course\n> that's what \"git reset\" does! But I didn't understand why until…\n> \n>> It is a bit confusing the way it updates the index, then resets it only\n>> to update it again at the end. I don't think we can avoid that though if\n>> we want to error out when there are conflicts merging the index.\n> \n> …which now makes (some) sense. That also explains why my attempts to\n> use reset_working_tree() in various forms could never work :) I had\n> the wrong idea entirely. But it seemed in my debugging like something\n> was touching the working tree, so I wish I had kept better notes.\n> \n> Just finishing up the flow:\n> \n> - we then refresh the in-memory index again (we just reset the on-disk\n> index to HEAD)\n> - we continue on with a \"normal\" stash apply merge of c_tree (old\n> index), w_tree (W), and b_tree (which I assume is H?); this applies\n> the stashed working tree changes on the current state?\n\nYes, b_tree is (H) and it cherry-picks the stashed working tree changes \non to the current worktree. c_tree is the current index at that point.\n > - [skipping ahead] in the index case, we reset_tree(&index_tree, 0,\n> 0), restoring the stashed index changes.\n\nYes\n\n> Sans doc comments for\n> unpack_trees() beyond \"N-way merge len trees […] resulting index […]\",\n> I haven't puzzled out what's going on, but it _looks_ like we merge a\n> single tree, possibly with the original index (opts.src_index) and\n> write to a destination which is the repository's index.\n\nApart from the \"git read-tree\" man page, the documentation for \nunpack_trees() is basically non-existent which is a real shame for such \na fundamental function. As I understand it, the one-way merge copies \nacross the stat data from the old index to the new index for entries \nthat are unchanged between the two. Without the \"reset\" bit it also \nchecks that there are no unmerged index entries and unstaged changes for \npaths that are removed by the merge.\n> It's extra unclear what the reset bit is applied to in\n> \n>> It looks like stash has its own unpack_trees() wrapper, so I think the\n>> simplest fix is to replace reset_head() with\n>>\n>> reset_tree(&c_tree, 0, 1);\n> \n> I'm not sure if that is a pre-merge reset, post-merge reset, or\n> something in between.\n> Unfortunately, a whole bunch of tests fail (6 files) with this suggestion :/\n> \n> Summary of Failures:\n> \n> 339/1062 git:t3904-stash-patch ERROR\n> 0.45s exit status 1\n> 503/1062 git:t3903-stash ERROR\n> 4.40s exit status 1\n> 763/1062 git:t6424-merge-unrelated-index-changes ERROR\n> 0.90s exit status 1\n> 778/1062 git:t6402-merge-rename ERROR\n> 2.06s exit status 1\n> 864/1062 git:t1092-sparse-checkout-compatibility ERROR\n> 26.19s exit status 1\n> 868/1062 git:t7611-merge-abort ERROR\n> 0.45s exit status 1\n> \n> It _also_ doesn't make the bug go away, hm.\n\nOh, I wonder what's happening there.\n\n>>>> We could avoid touching the index at all if we\n>>>> used merge_incore_nonrecursive() to cherry pick the index changes\n>>>> instead. That way we'd get a proper three-way merge and avoid\n>>>> spawning subprocesses for \"git diff-tree\", \"git apply --cached\",\n>>>> and \"git reset\". We're already using merge_ort_nonrecursive() to\n>>>> merge the working tree changes in that function so we have nearly\n>>>> everything we need already set up to merge the index changes as\n>>>> well. Essentially, when merging the index, we just need to call\n>>>> merge_incore_nonrecursive() instead of merge_ort_nonrecursive()\n>>>> and use info->i_tree instead of info->w_tree.\n> \n> If I'm following this, the suggestion is to replace (parts of) the\n> early \"if (index)\" block with a merge_incore_nonrecursive() to merge\n> index changes, reporting conflicts as we do today, and saving the tree\n> for later… and this would not touch the real index, so we wouldn't\n> have to reset at all? Interesting!\n> \n> The attached patch [Gmail headaches, sorry], which needs some\n> polishing [*], passes tests and fixes the bug! Yahoo. I'll send a\n> series later, tomorrow probably.\n> (It won't apply directly, because it's on top of the experimental\n> reset_tree() version, but resolving conflicts should be easy.)\n\nI had a quick look at the patch, it looks good, but I think we can \nsimplify it a bit. As we abort if there are conflicts I don't think we \nneed to spend any effort setting the conflict labels (I'm not sure if we \ncan pass NULL, but \"\" would certainly suffice). Does the current code \nprint any errors from apply when the patch does not apply? If not we \nshould silence the merge by setting verbosity=0. Also I think we should \nuse oidcpy to copy the merged tree (it probably does not matter in this \ncase, but it I think it does some extra checks on the hash function \nwhich a simple assignment does not)\n\nThanks for working on this, it will be a nice improvement\n\nPhillip\n\n> [*] namely, the log message, some tiny first cleanups, and removing\n> now-unused functions\n> \n\n"},{"id":"552884","messageId":"9ae2cb8a-7f2d-4c79-937b-170b8937bd8c@gmail.com","threadId":"66281","inReplyTo":"44881557-f15e-4ec5-b1c5-4112752f757c@gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-19T15:22:08Z","receivedAt":"2026-09-19T15:22:12Z","isPatch":false,"body":"On 17/09/2026 16:55, Phillip Wood wrote:\n> On 17/09/2026 14:15, D. Ben Knoble wrote:\n>>> It looks like stash has its own unpack_trees() wrapper, so I think the\n>>> simplest fix is to replace reset_head() with\n>>>\n>>>          reset_tree(&c_tree, 0, 1);\n>>\n>> I'm not sure if that is a pre-merge reset, post-merge reset, or\n>> something in between.\n>> Unfortunately, a whole bunch of tests fail (6 files) with this \n>> suggestion :/\n>>\n>> Summary of Failures:\n>>\n>>   339/1062 git:t3904-stash-patch                              ERROR\n>>        0.45s   exit status 1\n>>   503/1062 git:t3903-stash                                    ERROR\n>>        4.40s   exit status 1\n>>   763/1062 git:t6424-merge-unrelated-index-changes            ERROR\n>>        0.90s   exit status 1\n>>   778/1062 git:t6402-merge-rename                             ERROR\n>>        2.06s   exit status 1\n>>   864/1062 git:t1092-sparse-checkout-compatibility            ERROR\n>>       26.19s   exit status 1\n>>   868/1062 git:t7611-merge-abort                              ERROR\n>>        0.45s   exit status 1\n>>\n>> It _also_ doesn't make the bug go away, hm.\n> \n> Oh, I wonder what's happening there.\n\nSo the tests fail because when git tries to merge the stashed worktree \nchanges it thinks there are unstaged changes in the worktree. That's \nbecause when we merge the stashed index changes the stat data and \nCE_UPTODATE flag are cleared for paths that are updated by the merge. \nWhen we remove those merged changes from the index we need to refresh \nthe index to restore the stat data. Adding a call to refresh_index() \nafter reset_tree() makes the tests pass.\n\nAnyway that's all a bit irrelevant if we're going to start using \nmerge_incore_nonrecursive() but the test failures were bugging me so I \nthought I'd have a look at what was causing them.\n\nThanks\n\nPhillip\n\n\n>>>>> We could avoid touching the index at all if we\n>>>>> used merge_incore_nonrecursive() to cherry pick the index changes\n>>>>> instead. That way we'd get a proper three-way merge and avoid\n>>>>> spawning subprocesses for \"git diff-tree\", \"git apply --cached\",\n>>>>> and \"git reset\". We're already using merge_ort_nonrecursive() to\n>>>>> merge the working tree changes in that function so we have nearly\n>>>>> everything we need already set up to merge the index changes as\n>>>>> well. Essentially, when merging the index, we just need to call\n>>>>> merge_incore_nonrecursive() instead of merge_ort_nonrecursive()\n>>>>> and use info->i_tree instead of info->w_tree.\n>>\n>> If I'm following this, the suggestion is to replace (parts of) the\n>> early \"if (index)\" block with a merge_incore_nonrecursive() to merge\n>> index changes, reporting conflicts as we do today, and saving the tree\n>> for later… and this would not touch the real index, so we wouldn't\n>> have to reset at all? Interesting!\n>>\n>> The attached patch [Gmail headaches, sorry], which needs some\n>> polishing [*], passes tests and fixes the bug! Yahoo. I'll send a\n>> series later, tomorrow probably.\n>> (It won't apply directly, because it's on top of the experimental\n>> reset_tree() version, but resolving conflicts should be easy.)\n> \n> I had a quick look at the patch, it looks good, but I think we can \n> simplify it a bit. As we abort if there are conflicts I don't think we \n> need to spend any effort setting the conflict labels (I'm not sure if we \n> can pass NULL, but \"\" would certainly suffice). Does the current code \n> print any errors from apply when the patch does not apply? If not we \n> should silence the merge by setting verbosity=0. Also I think we should \n> use oidcpy to copy the merged tree (it probably does not matter in this \n> case, but it I think it does some extra checks on the hash function \n> which a simple assignment does not)\n> \n> Thanks for working on this, it will be a nice improvement\n> \n> Phillip\n> \n>> [*] namely, the log message, some tiny first cleanups, and removing\n>> now-unused functions\n>>\n> \n\n"},{"id":"552893","messageId":"CALnO6CBJoiu8Xzx_p9wYYUrR6ZxB93B8-su1Pxhr8WJ+68va1g@mail.gmail.com","threadId":"66281","inReplyTo":"44881557-f15e-4ec5-b1c5-4112752f757c@gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-19T20:02:02Z","receivedAt":"2026-09-19T20:02:17Z","isPatch":false,"body":"On Thu, Sep 17, 2026 at 11:55 AM Phillip Wood wrote:\n>\n> On 17/09/2026 14:15, D. Ben Knoble wrote:\n\n[snip]\n\n> > Sans doc comments for\n> > unpack_trees() beyond \"N-way merge len trees […] resulting index […]\",\n> > I haven't puzzled out what's going on, but it _looks_ like we merge a\n> > single tree, possibly with the original index (opts.src_index) and\n> > write to a destination which is the repository's index.\n>\n> Apart from the \"git read-tree\" man page, the documentation for\n> unpack_trees() is basically non-existent which is a real shame for such\n> a fundamental function. As I understand it, the one-way merge copies\n> across the stat data from the old index to the new index for entries\n> that are unchanged between the two.\n\nUp to here, I'm with you.\n\n> Without the \"reset\" bit it also\n> checks that there are no unmerged index entries and unstaged changes for\n> paths that are removed by the merge.\n\nThat is such a precise check to be covered by only \"reset\", heh. I\nsuppose the idea is that if \"reset\" is on, we don't need to check\nbecause we'll reset those entries/paths? More documentation from\nexperts in this area welcome…\n\n> > It's extra unclear what the reset bit is applied to in\n> >\n> >> It looks like stash has its own unpack_trees() wrapper, so I think the\n> >> simplest fix is to replace reset_head() with\n> >>\n> >> reset_tree(&c_tree, 0, 1);\n> >\n> > I'm not sure if that is a pre-merge reset, post-merge reset, or\n> > something in between.\n> > Unfortunately, a whole bunch of tests fail (6 files) with this suggestion :/\n> >\n[snip]\n> >\n> > It _also_ doesn't make the bug go away, hm.\n>\n> Oh, I wonder what's happening there.\n\nLooks like you figured it out :) When I was thinking about a series, I\nthought it might be nice to do the \"trivial\" fix first, then the\nrefactor we prefer, but… I'm not too sure. Since the other version\nworks, I might just go with it and omit the intermediate reset_tree\nstate.\n\n> I had a quick look at the patch, it looks good, but I think we can\n> simplify it a bit. As we abort if there are conflicts I don't think we\n> need to spend any effort setting the conflict labels (I'm not sure if we\n> can pass NULL, but \"\" would certainly suffice). Does the current code\n> print any errors from apply when the patch does not apply? If not we\n> should silence the merge by setting verbosity=0.\n\nActually, this was my reason to set the labels, too! I thought they\nmight show up in any logged messages. I'll take another look at what\nhappens here.\n\n> Also I think we should\n> use oidcpy to copy the merged tree (it probably does not matter in this\n> case, but it I think it does some extra checks on the hash function\n> which a simple assignment does not)\n\nGreat idea, thanks.\n\n-- \nD. Ben Knoble\n"},{"id":"552903","messageId":"CALnO6CCNEzvit8m4qXr3_oOzGWy5JwU77rp7Jr-7Hbcr1Vu5vw@mail.gmail.com","threadId":"66281","inReplyTo":"CALnO6CDfwscMWZktBu3FtXOQVbcBRo76nqK07kMnrzC5cPyZiQ@mail.gmail.com","subject":"Re: [BUG] stash.index=true leaves a redundant stash entry after an autostash fast-forward","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-19T21:32:07Z","receivedAt":"2026-09-19T21:32:22Z","isPatch":false,"body":"On Thu, Sep 17, 2026 at 9:15 AM D. Ben Knoble wrote:\n> The attached patch [Gmail headaches, sorry], which needs some\n> polishing [*], passes tests and fixes the bug! Yahoo. I'll send a\n> series later, tomorrow probably.\n> (It won't apply directly, because it's on top of the experimental\n> reset_tree() version, but resolving conflicts should be easy.)\n>\n> [*] namely, the log message, some tiny first cleanups, and removing\n> now-unused functions\n\nI've sent out a series with message ID\n\n\n-- \nD. Ben Knoble\n"}]}