Re: [PATCH v2 3/6] sequencer: factor out part of pick_commits()

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 21, 2023, 19:31 UTC
Message-ID: <xmqqpm7xjam2.fsf@gitster.g>
In-Reply-To: <31bb644e769a085bd2db97cdb5aae78729efc52d.1682089075.git.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> 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.

Show 152 quoted lines

> 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;