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@xxxxxxxxx> 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.
