Re: [PATCH v3 0/7] rebase -i: impove handling of failed commands
- From
Glen Choo <chooglen@google.com>
- Date
- Aug 7, 2023, 20:16 UTC
- Message-ID
- <kl6l4jla38wc.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <pull.1492.v3.git.1690903412.gitgitgadget@gmail.com>
Hi Phillip!
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 14 quoted lines
> 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.
Show 20 quoted lines
> 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.