Re: [PATCH v2 5/6] rebase: fix rewritten list for failed pick

[Glen Choo <chooglen@google.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

>>> When rebasing commands are moved from the todo list in "git-rebase-todo"
>>> to the "done" file just before they are executed. This means that if a
>>> command fails because it would overwrite an untracked file it has to be
>>> added back into the todo list before the rebase stops for the user to
>>> fix the problem. Unfortunately the way this is done results in the
>>> failed pick being recorded as rewritten.
>> 
>> I could not make the connection from the described problem to the
>> proposed solution. In particular, I couldn't tell what about "the way
>> this is done" that causes the incorrect behavior (e.g. are we failing to
>> clean up something? are we writing the wrong set of metadata?).
>
> Yes, on reflection that first paragraph is not very helpful. I've 
> updated it to
>
> git rebase keeps a list that maps the OID of each commit before
> it was rebased to the OID of the equivalent commit after the rebase.
> This list is used to drive the "post-rewrite" hook that is called at the
> end of a successful rebase. When a rebase stops for the user to resolve
> merge conflicts the OID of the commit being picked is written to
> ".git/rebase-merge/stopped-sha1" and when the rebase is continued that
> OID is added to the list of rewritten commits. Unfortunately when a
> commit cannot be picked because it would overwrite an untracked file we
> still write the "stopped-sha1" file and so when the rebase is continued
> the commit is added into the list of rewritten commits even though it
> has not been picked yet.
>
> Hopefully that is more helpful

Ah, yes that is much easier to visualise and understand. Thanks so much.

>>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>>> index c1fe55dc2c1..a657167befd 100755
>>> --- a/t/t3404-rebase-interactive.sh
>>> +++ b/t/t3404-rebase-interactive.sh
>>> @@ -1289,6 +1289,10 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
>>>   	test_cmp_rev HEAD F &&
>>>   	rm file6 &&
>>>   	test_path_is_missing .git/rebase-merge/author-script &&
>>> +	test_path_is_missing .git/rebase-merge/patch &&
>>> +	test_path_is_missing .git/MERGE_MSG &&
>>> +	test_path_is_missing .git/rebase-merge/message &&
>>> +	test_path_is_missing .git/rebase-merge/stopped-sha &&
>> 
>> This also seems to be testing implementation details, and if so, it
>> would be worth removing them.
>
> With the exception of the "patch" file which exists solely for the 
> benefit of the user this is testing an invariant of the implementation 
> which isn't ideal. I'm worried that removing these checks will mask some 
> subtle regression in the future. I think it is unlikely that the names 
> of these files will change in the future as we try to avoid changes that 
> would cause a rebase to fail if git is upgraded while it has stopped for 
> the user to resolve conflicts. I did think about whether we could add 
> some BUG() statements to sequencer.c instead. Unfortunately I don't 
> think it is that easy for the sequencer to know when these files should 
> be missing without relying on the logic that we are tying to test.

Unfortunately, it's been a while since I reviewed this patch, so forgive
me if I'm rusty. So you're saying that this test is about checking
invariants that we want to preserve between Git versions. I think that's
a reasonable goal - I am slightly skeptical of whether we should be
doing that ad-hoc like this, but I don't feel strongly about it.

IIRC, there was an earlier patch would be different from an where we
tested that author-script is missing, but what we really want is for the
pick to stop. Is the same thing happening here? E.g. is 'testing for
missing stopped-sha' a stand-in for 'testing that the rewritten list is
correct'? If so, it would be nice to test that specifically, but if
that's infeasible, a clarifying comment will probably suffice.