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

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