{"thread":{"id":"52911","subject":"rebase --abort Unespected behavior","startedAt":"2020-02-28T17:36:27Z","lastAt":"2020-03-04T20:55:11Z","messageCount":4,"participants":["Blaise Garant","Phillip Wood","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"392676","messageId":"CANXsDork=bL=SUodXDzkcnjpPALm53e++UkVkJFWxaZPMBK-SQ@mail.gmail.com","threadId":"52911","inReplyTo":null,"subject":"rebase --abort Unespected behavior","fromName":"Blaise Garant","fromEmail":"blaise@garantcoutu.com","sentAt":"2020-02-28T17:36:14Z","receivedAt":"2020-02-28T17:36:27Z","isPatch":false,"sender":{"key":"blaise@garantcoutu.com","avatar":null},"body":"Hello,\n\nI don't know if this is a bug but it was unexpected for us. I\naccidentally added untracked files through a `git add .` while doing\nan interactive rebase and aborting the rebase deleted those files. Is\nthis to be expected?\n\nTo reproduce:\nmkdir test_folder\ncd test_folder\ngit init\ntouch first\ngit add .\ngit commit -m 'First'\necho 1 >> first\ngit add .\ngit commit -m 'Second'\necho 2 >> first\ngit add .\ntouch second\ngit commit -m 'Third'\ngit rebase -i HEAD~2 #set second to be edited\ngit add .\ngit status        #second should have staged\ngit rebase --abort\nls        #second has been deleted\n\nNot sure this is an expected behavior.\nThanks\nBlaise Garant\n"},{"id":"392709","messageId":"13cdf33b-7ca2-c391-fddd-53bdbae7f0d3@gmail.com","threadId":"52911","inReplyTo":"CANXsDork=bL=SUodXDzkcnjpPALm53e++UkVkJFWxaZPMBK-SQ@mail.gmail.com","subject":"Re: rebase --abort Unespected behavior","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-02-29T14:28:03Z","receivedAt":"2020-02-29T14:28:09Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Blaise\n\nOn 28/02/2020 17:36, Blaise Garant wrote:\n> Hello,\n> \n> I don't know if this is a bug but it was unexpected for us. I\n> accidentally added untracked files through a `git add .` while doing\n> an interactive rebase and aborting the rebase deleted those files. Is\n> this to be expected?\n\nI agree that this is surprising and undesirable but it's not unexpected \ngiven the way --abort is implemented. 'rebase --abort' calls 'reset \n--hard <branch we're rebasing>' so it will discard all the uncommitted \nchanges in the worktree and reset the worktree and index to the branch tip.\n\nThe tricky thing with your situation is that the files are tracked at \nthe point we call 'reset --hard' as they've been added to the index so \ngit feels free to discard them. Perhaps rather than calling 'reset \n--hard' it would be better to use a custom callback with unpack_trees() \nthat errors out if there are any paths in the index that are not in \nHEAD, the commit we just picked or the branch tip we're resetting to. If \nwe do that we should consider using the same thing for \n'cherry-pick/merge/reset --abort' as well. --autostash potentially \ncomplicates things as the file might be in the stash but not in the \nother commits but lets not worry about that at the moment.\n\nIf your untracked files were ignored then I think 'git add .' would have \ncomplained or just not added them, but 'git checkout' and 'git merge' \nwill happily overwrite ignored files so ignoring them is not always an \nideal solution.\n\nBest Wishes\n\nPhillip\n\n\n> To reproduce:\n> mkdir test_folder\n> cd test_folder\n> git init\n> touch first\n> git add .\n> git commit -m 'First'\n> echo 1 >> first\n> git add .\n> git commit -m 'Second'\n> echo 2 >> first\n> git add .\n> touch second\n> git commit -m 'Third'\n> git rebase -i HEAD~2 #set second to be edited\n> git add .\n> git status        #second should have staged\n> git rebase --abort\n> ls        #second has been deleted\n> \n> Not sure this is an expected behavior.\n> Thanks\n> Blaise Garant\n> \n"},{"id":"392711","messageId":"CABPp-BFDJL5sbfY_CK_VuO0+hR3ZNsZqQKoRWNFMjwkqt52Riw@mail.gmail.com","threadId":"52911","inReplyTo":"13cdf33b-7ca2-c391-fddd-53bdbae7f0d3@gmail.com","subject":"Re: rebase --abort Unespected behavior","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-02-29T15:51:15Z","receivedAt":"2020-02-29T15:51:29Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Phillip and Blaise,\n\nOn Sat, Feb 29, 2020 at 6:30 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>\n> Hi Blaise\n>\n> On 28/02/2020 17:36, Blaise Garant wrote:\n> > Hello,\n> >\n> > I don't know if this is a bug but it was unexpected for us. I\n> > accidentally added untracked files through a `git add .` while doing\n> > an interactive rebase and aborting the rebase deleted those files. Is\n> > this to be expected?\n>\n> I agree that this is surprising and undesirable but it's not unexpected\n> given the way --abort is implemented. 'rebase --abort' calls 'reset\n> --hard <branch we're rebasing>' so it will discard all the uncommitted\n> changes in the worktree and reset the worktree and index to the branch tip.\n\nAnd it's worth noting that if they had done something similar outside\nof rebase/merge/cherry-pick/etc.:\n\ngit add $UNTRACKED\ngit reset --hard\n\nthen the $UNTRACKED file would be deleted, so this isn't new or\nunusual but matches git behavior elsewhere.\n\n> The tricky thing with your situation is that the files are tracked at\n> the point we call 'reset --hard' as they've been added to the index so\n> git feels free to discard them. Perhaps rather than calling 'reset\n> --hard' it would be better to use a custom callback with unpack_trees()\n> that errors out if there are any paths in the index that are not in\n> HEAD, the commit we just picked or the branch tip we're resetting to. If\n\nShould such a special callback also be used for reset --hard?\n\nAlso, this special callback, as stated here, wouldn't work: paths can\nexist in a merge that didn't exist in any of the three commits being\nthreeway merged.  All of the following situations are cases where that\ncan happen (and there may be more that I'm just not thinking of off\nthe top of my head):\n\n1) merge in a not-fully clean state.  rebase may disallow this, at\nleast right now, but merge traditionally hasn't.  I'm not sure\ncherry-pick --no-commit disallows this.\n2) directory/file or submodule/file or submodule/subdirectory or\nregular-file/symlink conflicts. the merge machinery should be free to\nrename paths to something that didn't exist on either side so that the\npaths from both side can coexist.\n3) directory renames.  If one side renamed z/->y/, and the other side\nadded a new file z/new, the merge should be allowed to move that file\nto y/new (depending on the setting of merge.directoryRenames...).\nNote that y/new didn't exist on either side of history nor in the\nmerge base.\n\n> we do that we should consider using the same thing for\n> 'cherry-pick/merge/reset --abort' as well. --autostash potentially\n> complicates things as the file might be in the stash but not in the\n> other commits but lets not worry about that at the moment.\n\nreset --abort?  Not sure what you're referring to here.\n"},{"id":"392873","messageId":"46d88e20-3832-ece3-16b6-7088c11bf586@gmail.com","threadId":"52911","inReplyTo":"CABPp-BFDJL5sbfY_CK_VuO0+hR3ZNsZqQKoRWNFMjwkqt52Riw@mail.gmail.com","subject":"Re: rebase --abort Unespected behavior","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-03-04T20:55:06Z","receivedAt":"2020-03-04T20:55:11Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 29/02/2020 15:51, Elijah Newren wrote:\n> Hi Phillip and Blaise,\n> \n> On Sat, Feb 29, 2020 at 6:30 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>>\n>> Hi Blaise\n>>\n>> On 28/02/2020 17:36, Blaise Garant wrote:\n>>> Hello,\n>>>\n>>> I don't know if this is a bug but it was unexpected for us. I\n>>> accidentally added untracked files through a `git add .` while doing\n>>> an interactive rebase and aborting the rebase deleted those files. Is\n>>> this to be expected?\n>>\n>> I agree that this is surprising and undesirable but it's not unexpected\n>> given the way --abort is implemented. 'rebase --abort' calls 'reset\n>> --hard <branch we're rebasing>' so it will discard all the uncommitted\n>> changes in the worktree and reset the worktree and index to the branch tip.\n> \n> And it's worth noting that if they had done something similar outside\n> of rebase/merge/cherry-pick/etc.:\n> \n> git add $UNTRACKED\n> git reset --hard\n> \n> then the $UNTRACKED file would be deleted, so this isn't new or\n> unusual but matches git behavior elsewhere.\n\nIt's not new but it can be confusing. rm warns if you remove a newly \nadded file but reset --hard will happily blow it away\n\n$ touch untracked\n$ git add untracked\n$ git rm untracked\nerror: the following file has changes staged in the index:\n     untracked\n(use --cached to keep the file, or -f to force removal)\n$ git reset --hard\nHEAD is now at <oid> <subject>\n\n>> The tricky thing with your situation is that the files are tracked at\n>> the point we call 'reset --hard' as they've been added to the index so\n>> git feels free to discard them. Perhaps rather than calling 'reset\n>> --hard' it would be better to use a custom callback with unpack_trees()\n>> that errors out if there are any paths in the index that are not in\n>> HEAD, the commit we just picked or the branch tip we're resetting to. If\n> \n> Should such a special callback also be used for reset --hard?\n >\n> Also, this special callback, as stated here, wouldn't work: paths can\n> exist in a merge that didn't exist in any of the three commits being\n> threeway merged.  All of the following situations are cases where that\n> can happen (and there may be more that I'm just not thinking of off\n> the top of my head):\n> \n> 1) merge in a not-fully clean state.  rebase may disallow this, at\n> least right now, but merge traditionally hasn't.  I'm not sure\n> cherry-pick --no-commit disallows this.\n\nI'm pretty sure cherry-pick --no-commit will operate on a dirty index, \nI'd forgotten about that\n\n> 2) directory/file or submodule/file or submodule/subdirectory or\n> regular-file/symlink conflicts. the merge machinery should be free to\n> rename paths to something that didn't exist on either side so that the\n> paths from both side can coexist.\n> 3) directory renames.  If one side renamed z/->y/, and the other side\n> added a new file z/new, the merge should be allowed to move that file\n> to y/new (depending on the setting of merge.directoryRenames...).\n> Note that y/new didn't exist on either side of history nor in the\n> merge base.\n\nThanks for pointing that out. I'd assumed we'd only check index entries \nin stage 0 so if the user aborts while these entries are unmerged then \nit wouldn't be a problem but if they've partially resolved the merge \nthen --abort could fail.\n\n>> we do that we should consider using the same thing for\n>> 'cherry-pick/merge/reset --abort' as well. --autostash potentially\n>> complicates things as the file might be in the stash but not in the\n>> other commits but lets not worry about that at the moment.\n> \n> reset --abort?  Not sure what you're referring to here.\n\nI meant revert\n\nI'm not sure what a good way forward is, blindly wiping out newly added \nfiles is not great from the users point of view but avoiding false \npositives is tricky\n\nBest Wishes\n\nPhillip\n"}]}