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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 9, 2023, 10:06 UTC
Message-ID: <a3b920a8-f446-2670-f1cd-de07b28f2c7d@gmail.com>
In-Reply-To: <kl6l4jla38wc.fsf@chooglen-macbookpro.roam.corp.google.com>

On 07/08/2023 21:16, Glen Choo wrote:

Show 23 quoted lines

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

Show 24 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.

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