Re: [PATCH v3 4/7] sequencer: factor out part of pick_commits()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Phillip,

On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This simplifies the next commit. If a pick fails we now return the error
> at the end of the loop body rather than returning early, a successful
> "edit" command continues to return early. There are three things to
> check to ensure that removing the early return for an error does not
> change the behavior of the code:
>
> (1) We could enter the block guarded by "if (reschedule)". This block
>     is not entered because "reschedlue" is always zero when picking a
>     commit.
>
> (2) We could enter the block guarded by
>     "else if (is_rebase_i(opts) &&  check_todo && !res)". This block is
>     not entered when returning an error because "res" is non-zero in
>     that case.
>
> (3) todo_list->current could be incremented before returning. That is
>     avoided by moving the increment which is of course a potential
>     change in behavior itself. The move is safe because none of the
>     callers look at todo_list after this function returns. Moving the
>     increment makes it clear we only want to advance the current item
>     if the command was successful.

Well argued, and it matches exactly what the patch says.

FWIW I had to look at the patch locally, using

$ git show --color-moved --color-moved-ws=allow-indentation-change \
	a1fad70f4b920252d84b5b5578b1b9b0a7bba7ca

to convince myself that the code was extracted into `pick_one_commit()` as
verbatim as possible. There are a couple of (benign) differences: a `&`
was removed since `check_todo` is now a pointer, and some wrapping
differences that leave the logic unchanged.

So here is my ACK.

Thank you,
Johannes