Hi Phillip, On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> > > 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
