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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Junio

On 21/04/2023 20:31, Junio C Hamano wrote:
"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:

From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux