Message ID | pull.1492.v3.git.1690903412.gitgitgadget@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | rebase -i: impove handling of failed commands | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > sequencer: factor out part of pick_commits() > rebase: fix rewritten list for failed pick > rebase --continue: refuse to commit after failed command > rebase -i: fix adding failed command to the todo list I'd really prefer to see the latter half of the series reviewed (both for the design and its implementation) by those who have more stake in the sequencer code than myself. I just noticed that we have a question on the last step left unanswered since the very initial iteration. cf. https://lore.kernel.org/git/f05deb00-1bcd-9e05-739f-6a30d6d8cf3b@gmx.de/ Thanks.
On 02/08/2023 23:10, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> sequencer: factor out part of pick_commits() >> rebase: fix rewritten list for failed pick >> rebase --continue: refuse to commit after failed command >> rebase -i: fix adding failed command to the todo list > > I'd really prefer to see the latter half of the series reviewed > (both for the design and its implementation) by those who have more > stake in the sequencer code than myself. That would certainly be nice. > I just noticed that we have a question on the last step left > unanswered since the very initial iteration. > > cf. https://lore.kernel.org/git/f05deb00-1bcd-9e05-739f-6a30d6d8cf3b@gmx.de/ Thanks, I'd forgotten about that. Best Wishes Phillip
Hi Phillip! "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. > > * the commit of the failed pick is recorded as rewritten even though no > commit was picked. > > * the "done" file used by "git status" to show the recently executed > commands contains an incorrect entry. > > Thanks to Eric, Glen and Junio for their comments on v2. Here are the > changes since v2: Thanks for sending this version, and apologies for not getting to it sooner (I tried a few times, but it was hard to reconstruct the context around something as complicated as sequencer.c..). Unfortunately, I don't think I will be able to chime in on subsequent rounds. > Patch 1 - Reworded the commit message. > > Patch 2 - Reworded the commit message, added a test and fixed error message > pointed out by Glen. > > Patch 3 - New cleanup. > > Patch 4 - Reworded the commit message, now only increments > todo_list->current if there is no error. > > Patch 5 - Swapped with next patch. Reworded the commit message, stopped > testing implementation (suggested by Glen). Expanded post-rewrite hook test. > > Patch 6 - Reworded the commit message, now uses the message file rather than > the author script to check if "rebase --continue" should commit staged > changes. Junio suggested using a separate file for this but I think that > would end up being more involved as we'd need to be careful about creating > and removing it. > > Patch 7 - Reworded the commit message. I found the updated commit messages much easier to understand, and the change to no longer test implementation is also very welcome, so overall, I think this is a marked improvement over the previous version. Like Junio, I'm not familiar enough with sequencer or its 'expected behavior' to feel comfortable LGTM-ing the later patches.
On 07/08/2023 21:16, Glen Choo wrote: > Hi Phillip! > > "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. >> >> * the commit of the failed pick is recorded as rewritten even though no >> commit was picked. >> >> * the "done" file used by "git status" to show the recently executed >> commands contains an incorrect entry. >> >> Thanks to Eric, Glen and Junio for their comments on v2. Here are the >> changes since v2: > > Thanks for sending this version, and apologies for not getting to it > sooner (I tried a few times, but it was hard to reconstruct the context > around something as complicated as sequencer.c..). Unfortunately, I > don't think I will be able to chime in on subsequent rounds. Thanks again for you comments on the last round, they were really helpful in improving the commit messages. >> Patch 1 - Reworded the commit message. >> >> Patch 2 - Reworded the commit message, added a test and fixed error message >> pointed out by Glen. >> >> Patch 3 - New cleanup. >> >> Patch 4 - Reworded the commit message, now only increments >> todo_list->current if there is no error. >> >> Patch 5 - Swapped with next patch. Reworded the commit message, stopped >> testing implementation (suggested by Glen). Expanded post-rewrite hook test. >> >> Patch 6 - Reworded the commit message, now uses the message file rather than >> the author script to check if "rebase --continue" should commit staged >> changes. Junio suggested using a separate file for this but I think that >> would end up being more involved as we'd need to be careful about creating >> and removing it. >> >> Patch 7 - Reworded the commit message. > > I found the updated commit messages much easier to understand, and the > change to no longer test implementation is also very welcome, so > overall, I think this is a marked improvement over the previous version. Thanks, I'm glad the messages are easier to understand now > Like Junio, I'm not familiar enough with sequencer or its 'expected > behavior' to feel comfortable LGTM-ing the later patches. Yes, I'm hoping Dscho will have time to take a look at them once 2.42.0 is out. Best Wishes Phillip
On 02/08/2023 23:10, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> sequencer: factor out part of pick_commits() >> rebase: fix rewritten list for failed pick >> rebase --continue: refuse to commit after failed command >> rebase -i: fix adding failed command to the todo list > > I'd really prefer to see the latter half of the series reviewed > (both for the design and its implementation) by those who have more > stake in the sequencer code than myself. Dscho do you think you will have time to look at the last four patches after 2.42.0 is released? Many Thanks Phillip > I just noticed that we have a question on the last step left > unanswered since the very initial iteration. > > cf. https://lore.kernel.org/git/f05deb00-1bcd-9e05-739f-6a30d6d8cf3b@gmx.de/ > > Thanks.