Re: [PATCH v2 0/6] rebase -i: impove handling of failed commands

[Glen Choo <chooglen@google.com>]

Hi Phillip!

We picked up this series during Review Club. You can browse the notes at

  https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1

but we'll post any substantial feedback back to the mailing list anyway.

Firstly, I have to acknowledge that this series appears to be geared
towards to reviewers who are already familiar with the rebase machinery
and don't require a lot of context on the changes. None of the Review
Club attendees were familiar with it, so we had trouble following along
certain patches, but we might not have been the intended audience
anyway.

I can leave comments from that perspective, i.e. what would have been
useful from _if_ this were written for folks who weren't already
familiar with the rebase code, which could be useful if we were trying
to train people to reviewer sequencer.c. However, I don't think this
warrants substantial changes if the series is already clear to the
intended audience.

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This series fixes several bugs in the way we handle a commit cannot be
> picked because it would overwrite an untracked file.
>
>  * after a failed pick "git rebase --continue" will happily commit any
>    staged changes even though no commit was picked.

This sounds like a safer default for most users, but I worry that there
are seasoned users relying on the old behavior.

>  * the commit of the failed pick is recorded as rewritten even though no
>    commit was picked.

Sounds like a reasonable fix.

>  * the "done" file used by "git status" to show the recently executed
>    commands contains an incorrect entry.

This also sounds reasonable, but from Johannes' upthread response, this
sounds like this isn't universally agreed upon.

Perhaps the underlying issue is that the behavior of "git rebase
--continue", and the todo/done lists is underspecified (in public and
internal documentation), so we end up having to reestablish what the
'correct' behavior is (which will probably end up being just a matter of
personal taste). This isn't strictly necessary for the series, but it
would be nice for us to establish what the correct behavior _should be_
(even if we aren't there yet) and use that to guide future fixes.