From: Phillip Wood Date: Fri, 21 Apr 2023 20:00:47 GMT Subject: Re: [PATCH v2 3/6] sequencer: factor out part of pick_commits() Message-ID: <049a792d-e015-3583-452d-923b9aee4a72@gmail.com> In-Reply-To: Hi Junio On 21/04/2023 20:31, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" writes: > >> From: Phillip Wood >> >> This is simplifies a change in a later commit. If a pick fails we now >> return the error at then end of the loop body rather than returning >> early but there is no change in behavior. > > The new pick_one_commit() function is pretty much verbatim copy/move > from inside the "any command below SQUASH" block, and in the > original code, the block returned with an error whenever res is not > 0, with one exception that TODO_EDIT would return with 0 if there is > no error (but still with a patch). > > The new code that calls pick_one_commit() helper lets this exception > case to return from the function in the "any command below SQUASH" > block, but everything else falls through *and* eventually at the end > of the outer block there is > > if (res) > return res; > > that makes us return from the function. > > But there are now a few other things done after the if/else if/else > cascade, namely > > * there is an extra "if (reschedule)" and "else if (rebase-i) etc" > logic. There are two blocks that might be entered. One guarded by "if (reschedule)" - this is not entered because reschedlue is always zero when picking a commit. The other is guarded by "else if (is_rebase_i(opts) && check_todo && !res)" and so will not be entered when we want to return an error because "res" is non-zero in that case. Best Wishes Phillip > * the todo_list->current counter is incremented > > are done BEFORE that "if res is not zero return". I am not sure we > can safely claim "there is no change in behaviour". > > Am I missing something? > > Thanks. > >> Signed-off-by: Phillip Wood >> --- >> sequencer.c | 129 ++++++++++++++++++++++++++++------------------------ >> 1 file changed, 69 insertions(+), 60 deletions(-) >> >> diff --git a/sequencer.c b/sequencer.c >> index c4a548f2c98..2d463818dd1 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -4628,6 +4628,72 @@ N_("Could not execute the todo command\n" >> " git rebase --edit-todo\n" >> " git rebase --continue\n"); >> >> +static int pick_one_commit(struct repository *r, >> + struct todo_list *todo_list, >> + struct replay_opts *opts, >> + int *check_todo) >> +{ >> + int res; >> + struct todo_item *item = todo_list->items + todo_list->current; >> + const char *arg = todo_item_get_arg(todo_list, item); >> + if (is_rebase_i(opts)) >> + opts->reflog_message = reflog_message( >> + opts, command_to_string(item->command), NULL); >> + >> + res = do_pick_commit(r, item, opts, is_final_fixup(todo_list), >> + check_todo); >> + if (is_rebase_i(opts) && res < 0) { >> + /* Reschedule */ >> + advise(_(rescheduled_advice), >> + get_item_line_length(todo_list, todo_list->current), >> + get_item_line(todo_list, todo_list->current)); >> + todo_list->current--; >> + if (save_todo(todo_list, opts)) >> + return -1; >> + } >> + if (item->command == TODO_EDIT) { >> + struct commit *commit = item->commit; >> + if (!res) { >> + if (!opts->verbose) >> + term_clear_line(); >> + fprintf(stderr, _("Stopped at %s... %.*s\n"), >> + short_commit_name(commit), item->arg_len, arg); >> + } >> + return error_with_patch(r, commit, >> + arg, item->arg_len, opts, res, !res); >> + } >> + if (is_rebase_i(opts) && !res) >> + record_in_rewritten(&item->commit->object.oid, >> + peek_command(todo_list, 1)); >> + if (res && is_fixup(item->command)) { >> + if (res == 1) >> + intend_to_amend(); >> + return error_failed_squash(r, item->commit, opts, >> + item->arg_len, arg); >> + } else if (res && is_rebase_i(opts) && item->commit) { >> + int to_amend = 0; >> + struct object_id oid; >> + >> + /* >> + * If we are rewording and have either >> + * fast-forwarded already, or are about to >> + * create a new root commit, we want to amend, >> + * otherwise we do not. >> + */ >> + if (item->command == TODO_REWORD && >> + !repo_get_oid(r, "HEAD", &oid) && >> + (oideq(&item->commit->object.oid, &oid) || >> + (opts->have_squash_onto && >> + oideq(&opts->squash_onto, &oid)))) >> + to_amend = 1; >> + >> + return res | error_with_patch(r, item->commit, >> + arg, item->arg_len, opts, >> + res, to_amend); >> + } >> + return res; >> +} >> + >> static int pick_commits(struct repository *r, >> struct todo_list *todo_list, >> struct replay_opts *opts) >> @@ -4683,66 +4749,9 @@ static int pick_commits(struct repository *r, >> } >> } >> if (item->command <= TODO_SQUASH) { >> - if (is_rebase_i(opts)) >> - opts->reflog_message = reflog_message(opts, >> - command_to_string(item->command), NULL); >> - >> - res = do_pick_commit(r, item, opts, >> - is_final_fixup(todo_list), >> - &check_todo); >> - if (is_rebase_i(opts) && res < 0) { >> - /* Reschedule */ >> - advise(_(rescheduled_advice), >> - get_item_line_length(todo_list, >> - todo_list->current), >> - get_item_line(todo_list, >> - todo_list->current)); >> - todo_list->current--; >> - if (save_todo(todo_list, opts)) >> - return -1; >> - } >> - if (item->command == TODO_EDIT) { >> - struct commit *commit = item->commit; >> - if (!res) { >> - if (!opts->verbose) >> - term_clear_line(); >> - fprintf(stderr, >> - _("Stopped at %s... %.*s\n"), >> - short_commit_name(commit), >> - item->arg_len, arg); >> - } >> - return error_with_patch(r, commit, >> - arg, item->arg_len, opts, res, !res); >> - } >> - if (is_rebase_i(opts) && !res) >> - record_in_rewritten(&item->commit->object.oid, >> - peek_command(todo_list, 1)); >> - if (res && is_fixup(item->command)) { >> - if (res == 1) >> - intend_to_amend(); >> - return error_failed_squash(r, item->commit, opts, >> - item->arg_len, arg); >> - } else if (res && is_rebase_i(opts) && item->commit) { >> - int to_amend = 0; >> - struct object_id oid; >> - >> - /* >> - * If we are rewording and have either >> - * fast-forwarded already, or are about to >> - * create a new root commit, we want to amend, >> - * otherwise we do not. >> - */ >> - if (item->command == TODO_REWORD && >> - !repo_get_oid(r, "HEAD", &oid) && >> - (oideq(&item->commit->object.oid, &oid) || >> - (opts->have_squash_onto && >> - oideq(&opts->squash_onto, &oid)))) >> - to_amend = 1; >> - >> - return res | error_with_patch(r, item->commit, >> - arg, item->arg_len, opts, >> - res, to_amend); >> - } >> + res = pick_one_commit(r, todo_list, opts, &check_todo); >> + if (!res && item->command == TODO_EDIT) >> + return 0; >> } else if (item->command == TODO_EXEC) { >> char *end_of_arg = (char *)(arg + item->arg_len); >> int saved = *end_of_arg;