| Message ID | 31bb644e769a085bd2db97cdb5aae78729efc52d.1682089075.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase -i: impove handling of failed commands | expand |
On Fri, Apr 21, 2023 at 11:11 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote: > This is simplifies a change in a later commit. If a pick fails we now s/This is/This/ > return the error at then end of the loop body rather than returning s/then end/the end/ > early but there is no change in behavior. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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. * 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 <phillip.wood@dunelm.org.uk> > --- > 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;
Hi Junio On 21/04/2023 20:31, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> 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 <phillip.wood@dunelm.org.uk> >> --- >> 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;
Phillip Wood <phillip.wood123@gmail.com> writes: > 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. Perhaps the proposed log message can be updated to mention these to save time from "git log" readers? >> * the todo_list->current counter is incremented The todo_list->current counter gets incremented before leaving the function now, but all three callers of pick_commits() immediately call todo_list_release(), the value, whether it is incremented or not, is discarded without getting looked at. So it is not a noop, but the difference in behaviour does not become externally observable? Thanks.
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;