Re: rebase --abort Unespected behavior

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 4, 2020, 20:55 UTC
Message-ID: <46d88e20-3832-ece3-16b6-7088c11bf586@gmail.com>
In-Reply-To: <CABPp-BFDJL5sbfY_CK_VuO0+hR3ZNsZqQKoRWNFMjwkqt52Riw@mail.gmail.com>

Hi Elijah

On 29/02/2020 15:51, Elijah Newren wrote:

Show 27 quoted lines

> Hi Phillip and Blaise,
> 
> On Sat, Feb 29, 2020 at 6:30 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Blaise
>>
>> On 28/02/2020 17:36, Blaise Garant wrote:
>>> Hello,
>>>
>>> I don't know if this is a bug but it was unexpected for us. I
>>> accidentally added untracked files through a `git add .` while doing
>>> an interactive rebase and aborting the rebase deleted those files. Is
>>> this to be expected?
>>
>> I agree that this is surprising and undesirable but it's not unexpected
>> given the way --abort is implemented. 'rebase --abort' calls 'reset
>> --hard <branch we're rebasing>' so it will discard all the uncommitted
>> changes in the worktree and reset the worktree and index to the branch tip.
> 
> And it's worth noting that if they had done something similar outside
> of rebase/merge/cherry-pick/etc.:
> 
> git add $UNTRACKED
> git reset --hard
> 
> then the $UNTRACKED file would be deleted, so this isn't new or
> unusual but matches git behavior elsewhere.

It's not new but it can be confusing. rm warns if you remove a newly added file but reset --hard will happily blow it away

$ touch untracked
$ git add untracked
$ git rm untracked
error: the following file has changes staged in the index:
     untracked
(use --cached to keep the file, or -f to force removal)
$ git reset --hard
HEAD is now at <oid> <subject>

Show 8 quoted lines

>> The tricky thing with your situation is that the files are tracked at
>> the point we call 'reset --hard' as they've been added to the index so
>> git feels free to discard them. Perhaps rather than calling 'reset
>> --hard' it would be better to use a custom callback with unpack_trees()
>> that errors out if there are any paths in the index that are not in
>> HEAD, the commit we just picked or the branch tip we're resetting to. If
> 
> Should such a special callback also be used for reset --hard?

Show 9 quoted lines

> Also, this special callback, as stated here, wouldn't work: paths can
> exist in a merge that didn't exist in any of the three commits being
> threeway merged.  All of the following situations are cases where that
> can happen (and there may be more that I'm just not thinking of off
> the top of my head):
> 
> 1) merge in a not-fully clean state.  rebase may disallow this, at
> least right now, but merge traditionally hasn't.  I'm not sure
> cherry-pick --no-commit disallows this.

I'm pretty sure cherry-pick --no-commit will operate on a dirty index, I'd forgotten about that

Show 9 quoted lines

> 2) directory/file or submodule/file or submodule/subdirectory or
> regular-file/symlink conflicts. the merge machinery should be free to
> rename paths to something that didn't exist on either side so that the
> paths from both side can coexist.
> 3) directory renames.  If one side renamed z/->y/, and the other side
> added a new file z/new, the merge should be allowed to move that file
> to y/new (depending on the setting of merge.directoryRenames...).
> Note that y/new didn't exist on either side of history nor in the
> merge base.

Thanks for pointing that out. I'd assumed we'd only check index entries in stage 0 so if the user aborts while these entries are unmerged then it wouldn't be a problem but if they've partially resolved the merge then --abort could fail.

Show 6 quoted lines

>> we do that we should consider using the same thing for
>> 'cherry-pick/merge/reset --abort' as well. --autostash potentially
>> complicates things as the file might be in the stash but not in the
>> other commits but lets not worry about that at the moment.
> 
> reset --abort?  Not sure what you're referring to here.

I meant revert

I'm not sure what a good way forward is, blindly wiping out newly added files is not great from the users point of view but avoiding false positives is tricky

Best Wishes

Phillip