[v3,4/7] sequencer: factor out part of pick_commits()

Commit Message

Phillip Wood Aug. 1, 2023, 3:23 p.m. UTC

From: Phillip Wood <[e-mail]>

This simplifies the next commit. If a pick fails we now return the error
at the end of the loop body rather than returning early, a successful
"edit" command continues to return early. There are three things to
check to ensure that removing the early return for an error does not
change the behavior of the code:

(1) We could enter the block guarded by "if (reschedule)". This block
    is not entered because "reschedlue" is always zero when picking a
    commit.

(2) We could enter the block guarded by
    "else if (is_rebase_i(opts) &&  check_todo && !res)". This block is
    not entered when returning an error because "res" is non-zero in
    that case.

(3) todo_list->current could be incremented before returning. That is
    avoided by moving the increment which is of course a potential
    change in behavior itself. The move is safe because none of the
    callers look at todo_list after this function returns. Moving the
    increment makes it clear we only want to advance the current item
    if the command was successful.

Signed-off-by: Phillip Wood <[e-mail]>
---
 sequencer.c | 132 ++++++++++++++++++++++++++++------------------------
 1 file changed, 71 insertions(+), 61 deletions(-)

Comments

Johannes Schindelin Aug. 23, 2023, 8:55 a.m. UTC | #1

Hi Phillip,

On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <[e-mail]>
>
> This simplifies the next commit. If a pick fails we now return the error
> at the end of the loop body rather than returning early, a successful
> "edit" command continues to return early. There are three things to
> check to ensure that removing the early return for an error does not
> change the behavior of the code:
>
> (1) We could enter the block guarded by "if (reschedule)". This block
>     is not entered because "reschedlue" is always zero when picking a
>     commit.
>
> (2) We could enter the block guarded by
>     "else if (is_rebase_i(opts) &&  check_todo && !res)". This block is
>     not entered when returning an error because "res" is non-zero in
>     that case.
>
> (3) todo_list->current could be incremented before returning. That is
>     avoided by moving the increment which is of course a potential
>     change in behavior itself. The move is safe because none of the
>     callers look at todo_list after this function returns. Moving the
>     increment makes it clear we only want to advance the current item
>     if the command was successful.

Well argued, and it matches exactly what the patch says.

FWIW I had to look at the patch locally, using

$ git show --color-moved --color-moved-ws=allow-indentation-change \
	a1fad70f4b920252d84b5b5578b1b9b0a7bba7ca

to convince myself that the code was extracted into `pick_one_commit()` as
verbatim as possible. There are a couple of (benign) differences: a `&`
was removed since `check_todo` is now a pointer, and some wrapping
differences that leave the logic unchanged.

So here is my ACK.

Thank you,
Johannes

Patch

diff --git a/sequencer.c b/sequencer.c
index dbddd19b2c2..62277e7bcc1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4642,6 +4642,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)
@@ -4697,66 +4763,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;
@@ -4817,9 +4826,10 @@  static int pick_commits(struct repository *r,
 			return -1;
 		}
 
-		todo_list->current++;
 		if (res)
 			return res;
+
+		todo_list->current++;
 	}
 
 	if (is_rebase_i(opts)) {