Re: [PATCH v3 0/7] rebase -i: impove handling of failed commands

[Phillip Wood <phillip.wood123@gmail.com>]

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