From: Phillip Wood <phillip.wood123@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
Johannes Schindelin <Johannes.Schindelin@gmx.de>,
Stefan Haller <lists@haller-berlin.de>,
Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v2 3/6] sequencer: factor out part of pick_commits()
Date: Fri, 21 Apr 2023 21:00:47 +0100 [thread overview]
Message-ID: <049a792d-e015-3583-452d-923b9aee4a72@gmail.com> (raw)
In-Reply-To: <xmqqpm7xjam2.fsf@gitster.g>
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;
next prev parent reply other threads:[~2023-04-21 20:01 UTC|newest]
Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-19 14:48 [PATCH] rebase -i: do not update "done" when rescheduling command Phillip Wood via GitGitGadget
2023-03-20 7:29 ` Stefan Haller
2023-03-20 17:46 ` Junio C Hamano
2023-03-24 10:50 ` Phillip Wood
2023-03-24 15:49 ` Junio C Hamano
2023-03-24 16:22 ` Phillip Wood
2023-03-27 7:04 ` Johannes Schindelin
2023-08-03 12:56 ` Phillip Wood
2023-08-23 8:54 ` Johannes Schindelin
2023-04-21 14:57 ` [PATCH v2 0/6] rebase -i: impove handling of failed commands Phillip Wood via GitGitGadget
2023-04-21 14:57 ` [PATCH v2 1/6] rebase -i: move unlink() calls Phillip Wood via GitGitGadget
2023-04-21 17:22 ` Junio C Hamano
2023-04-27 10:15 ` Phillip Wood
2023-04-21 14:57 ` [PATCH v2 2/6] rebase -i: remove patch file after conflict resolution Phillip Wood via GitGitGadget
2023-04-21 19:01 ` Junio C Hamano
2023-04-27 10:17 ` Phillip Wood
2023-06-21 20:14 ` Glen Choo
2023-07-14 10:08 ` Phillip Wood
2023-07-14 16:51 ` Junio C Hamano
2023-07-17 15:39 ` Phillip Wood
2023-04-21 14:57 ` [PATCH v2 3/6] sequencer: factor out part of pick_commits() Phillip Wood via GitGitGadget
2023-04-21 19:12 ` Eric Sunshine
2023-04-21 19:31 ` Junio C Hamano
2023-04-21 20:00 ` Phillip Wood [this message]
2023-04-21 21:21 ` Junio C Hamano
2023-04-21 14:57 ` [PATCH v2 4/6] rebase --continue: refuse to commit after failed command Phillip Wood via GitGitGadget
2023-04-21 19:14 ` Eric Sunshine
2023-04-21 21:05 ` Junio C Hamano
2023-06-21 20:35 ` Glen Choo
2023-04-21 14:57 ` [PATCH v2 5/6] rebase: fix rewritten list for failed pick Phillip Wood via GitGitGadget
2023-06-21 20:49 ` Glen Choo
2023-07-25 15:42 ` Phillip Wood
2023-07-25 16:46 ` Glen Choo
2023-07-26 13:08 ` Phillip Wood
2023-07-26 17:48 ` Glen Choo
2023-07-28 13:19 ` Phillip Wood
2023-04-21 14:57 ` [PATCH v2 6/6] rebase -i: fix adding failed command to the todo list Phillip Wood via GitGitGadget
2023-06-21 20:59 ` Glen Choo
2023-04-21 16:56 ` [PATCH v2 0/6] rebase -i: impove handling of failed commands Junio C Hamano
2023-06-21 20:07 ` Glen Choo
2023-08-01 15:23 ` [PATCH v3 0/7] " Phillip Wood via GitGitGadget
2023-08-01 15:23 ` [PATCH v3 1/7] rebase -i: move unlink() calls Phillip Wood via GitGitGadget
2023-08-01 17:22 ` Junio C Hamano
2023-08-01 18:42 ` Phillip Wood
2023-08-01 19:31 ` Junio C Hamano
2023-08-01 15:23 ` [PATCH v3 2/7] rebase -i: remove patch file after conflict resolution Phillip Wood via GitGitGadget
2023-08-01 17:23 ` Junio C Hamano
2023-08-01 18:47 ` Phillip Wood
2023-08-01 15:23 ` [PATCH v3 3/7] sequencer: use rebase_path_message() Phillip Wood via GitGitGadget
2023-08-01 17:23 ` Junio C Hamano
2023-08-01 18:49 ` Phillip Wood
2023-08-02 22:02 ` Junio C Hamano
2023-08-01 15:23 ` [PATCH v3 4/7] sequencer: factor out part of pick_commits() Phillip Wood via GitGitGadget
2023-08-23 8:55 ` Johannes Schindelin
2023-08-01 15:23 ` [PATCH v3 5/7] rebase: fix rewritten list for failed pick Phillip Wood via GitGitGadget
2023-08-23 8:55 ` Johannes Schindelin
2023-09-04 14:31 ` Phillip Wood
2023-08-01 15:23 ` [PATCH v3 6/7] rebase --continue: refuse to commit after failed command Phillip Wood via GitGitGadget
2023-08-23 9:01 ` Johannes Schindelin
2023-09-04 14:37 ` Phillip Wood
2023-09-05 11:17 ` Johannes Schindelin
2023-09-05 14:57 ` Junio C Hamano
2023-09-05 15:25 ` Phillip Wood
2023-08-01 15:23 ` [PATCH v3 7/7] rebase -i: fix adding failed command to the todo list Phillip Wood via GitGitGadget
2023-08-02 22:10 ` [PATCH v3 0/7] rebase -i: impove handling of failed commands Junio C Hamano
2023-08-03 13:06 ` Phillip Wood
2023-08-09 13:08 ` Phillip Wood
2023-08-07 20:16 ` Glen Choo
2023-08-09 10:06 ` Phillip Wood
2023-09-06 15:22 ` [PATCH v4 " Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 1/7] rebase -i: move unlink() calls Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 2/7] rebase -i: remove patch file after conflict resolution Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 3/7] sequencer: use rebase_path_message() Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 4/7] sequencer: factor out part of pick_commits() Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 5/7] rebase: fix rewritten list for failed pick Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 6/7] rebase --continue: refuse to commit after failed command Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 7/7] rebase -i: fix adding failed command to the todo list Phillip Wood via GitGitGadget
2023-09-06 21:01 ` [PATCH v4 0/7] rebase -i: impove handling of failed commands Junio C Hamano
2023-09-07 9:56 ` Johannes Schindelin
2023-09-07 20:33 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=049a792d-e015-3583-452d-923b9aee4a72@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=lists@haller-berlin.de \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).