Skip to contentSkip to conversation

rebase: offer to reschedule failed exec commands automatically

PATCH In review Johannes Schindelin via GitGitGadget 8 participants 29 messages Dec 10, 2018 – Apr 15, 2021 Documentation

lore
Share & tools
lore
#0 lore raw

cover rebase: offer to reschedule failed exec commands automatically

The idea was brought up by Paul Morelle.

To be honest, this idea of rescheduling a failed exec makes so much sense that I wish we had done this from the get-go.

So let's do the next best thing: implement a command-line option and a config setting to make it so.

The obvious intent behind that config setting is to not only give users a way to opt into that behavior already, but also to make it possible to flip the default at a later date, after the feature has been battle-tested and after deprecating the non-rescheduling behavior for a couple of Git versions.

If the team agrees with my reasoning, then the 3rd patch (introducing -y <cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make much sense, as it would introduce a short option that would become obsolete soon.

Johannes Schindelin (3):
rebase: introduce --reschedule-failed-exec
rebase: add a config option to default to --reschedule-failed-exec
rebase: introduce a shortcut for --reschedule-failed-exec

 Documentation/config/rebase.txt |  5 ++++
 Documentation/git-rebase.txt    | 11 +++++++++
 builtin/rebase--interactive.c   |  2 ++
 builtin/rebase.c                | 42 ++++++++++++++++++++++++++++++++-
 git-legacy-rebase.sh            | 24 ++++++++++++++++++-
 git-rebase--common.sh           |  1 +
 sequencer.c                     | 13 +++++++---
 sequencer.h                     |  1 +
 t/t3418-rebase-continue.sh      | 14 +++++++++++
 9 files changed, 108 insertions(+), 5 deletions(-)

base-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-90%2Fdscho%2Freschedule-failed-exec-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-90/dscho/reschedule-failed-exec-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/90

Signature · 1 line
gitgitgadget
#1 lore raw

PATCH 1/3 rebase: introduce --reschedule-failed-exec

From: Johannes Schindelin <[email protected]>

A common use case for the --exec option is to verify that each commit in a topic branch compiles cleanly, via git rebase -x make <base>.

However, when an exec in such a rebase fails, it is not re-scheduled, which in this instance is not particularly helpful.

Let's offer a flag to reschedule failed exec commands.

Based on an idea by Paul Morelle.

---
 Documentation/git-rebase.txt  |  5 +++++
 builtin/rebase--interactive.c |  2 ++
 builtin/rebase.c              | 16 +++++++++++++++-
 git-legacy-rebase.sh          | 16 +++++++++++++++-
 git-rebase--common.sh         |  1 +
 sequencer.c                   | 13 ++++++++++---
 sequencer.h                   |  1 +
 t/t3418-rebase-continue.sh    |  6 ++++++
 8 files changed, 55 insertions(+), 5 deletions(-)
Documentation/git-rebase.txt +5 -0
index 80793bad8d..9dd68f77f6 100644
@@ -501,6 +501,11 @@ See also INCOMPATIBLE OPTIONS below.
 	with care: the final stash application after a successful
 	rebase might result in non-trivial conflicts.
 
+--reschedule-failed-exec::
+--no-reschedule-failed-exec::
+	Automatically reschedule `exec` commands that failed. This only makes
+	sense in interactive mode (or when an `--exec` option was provided).
+
 INCOMPATIBLE OPTIONS
 --------------------
 
builtin/rebase--interactive.c +2 -0
index a2ab68ed06..a9ab009fdb 100644
@@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
 		OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
 		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
+		OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
+			 N_("automatically re-schedule any `exec` that fails")),
 		OPT_END()
 	};
 
builtin/rebase.c +15 -1
index 5b3e5baec8..6d556fc6c8 100644
@@ -104,6 +104,7 @@ struct rebase_options {
 	int rebase_merges, rebase_cousins;
 	char *strategy, *strategy_opts;
 	struct strbuf git_format_patch_opt;
+	int reschedule_failed_exec;
 };
 
 static int is_interactive(struct rebase_options *opts)
@@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)
 			argv_array_push(&child.args, opts->gpg_sign_opt);
 		if (opts->signoff)
 			argv_array_push(&child.args, "--signoff");
+		if (opts->reschedule_failed_exec)
+			argv_array_push(&child.args, "--reschedule-failed-exec");
 
 		status = run_command(&child);
 		goto finished_rebase;
@@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				   "strategy")),
 		OPT_BOOL(0, "root", &options.root,
 			 N_("rebase all reachable commits up to the root(s)")),
+		OPT_BOOL(0, "reschedule-failed-exec",
+			 &options.reschedule_failed_exec,
+			 N_("automatically re-schedule any `exec` that fails")),
 		OPT_END(),
 	};
 	int i;
@@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
+	if (options.reschedule_failed_exec && !is_interactive(&options))
+		die(_("--reschedule-failed-exec requires an interactive rebase"));
+
 	if (options.git_am_opts.argc) {
 		/* all am options except -q are compatible only with --am */
 		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
@@ -1220,7 +1229,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.flags |= REBASE_FORCE;
 	}
 
-	if (options.type == REBASE_PRESERVE_MERGES)
+	if (options.type == REBASE_PRESERVE_MERGES) {
 		/*
 		 * Note: incompatibility with --signoff handled in signoff block above
 		 * Note: incompatibility with --interactive is just a strong warning;
@@ -1230,6 +1239,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("error: cannot combine '--preserve-merges' with "
 			      "'--rebase-merges'"));
 
+		if (options.reschedule_failed_exec)
+			die(_("error: cannot combine '--preserve-merges' with "
+			      "'--reschedule-failed-exec'"));
+	}
+
 	if (options.rebase_merges) {
 		if (strategy_options.nr)
 			die(_("error: cannot combine '--rebase-merges' with "
git-legacy-rebase.sh +15 -1
index b97ffdc9dd..5f0f0c5ab8 100755
@@ -48,6 +48,7 @@ skip!              skip current patch and continue
 edit-todo!         edit the todo list during an interactive rebase
 quit!              abort but keep HEAD where it is
 show-current-patch! show the patch file being applied or merged
+reschedule-failed-exec automatically reschedule failed exec commands
 "
 . git-sh-setup
 set_reflog_action rebase
@@ -92,6 +93,7 @@ autosquash=
 keep_empty=
 allow_empty_message=--allow-empty-message
 signoff=
+reschedule_failed_exec=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
@@ -126,6 +128,8 @@ read_basic_state () {
 		signoff="$(cat "$state_dir"/signoff)"
 		force_rebase=t
 	}
+	test -f "$state_dir"/reschedule-failed-exec &&
+		reschedule_failed_exec=t
 }
 
 finish_rebase () {
@@ -163,7 +167,8 @@ run_interactive () {
 		"$allow_empty_message" "$autosquash" "$verbose" \
 		"$force_rebase" "$onto_name" "$head_name" "$strategy" \
 		"$strategy_opts" "$cmd" "$switch_to" \
-		"$allow_rerere_autoupdate" "$gpg_sign_opt" "$signoff"
+		"$allow_rerere_autoupdate" "$gpg_sign_opt" "$signoff" \
+		"$reschedule_failed_exec"
 }
 
 run_specific_rebase () {
@@ -378,6 +383,12 @@ do
 	--gpg-sign=*)
 		gpg_sign_opt="-S${1#--gpg-sign=}"
 		;;
+	--reschedule-failed-exec)
+		reschedule_failed_exec=--reschedule-failed-exec
+		;;
+	--no-reschedule-failed-exec)
+		reschedule_failed_exec=
+		;;
 	--)
 		shift
 		break
@@ -534,6 +545,9 @@ then
 	#       git-rebase.txt caveats with "unless you know what you are doing"
 	test -n "$rebase_merges" &&
 		die "$(gettext "error: cannot combine '--preserve-merges' with '--rebase-merges'")"
+
+	test -n "$reschedule_failed_exec" &&
+		die "$(gettext "error: cannot combine '--preserve-merges' with '--reschedule-failed-exec'")"
 fi
 
 if test -n "$rebase_merges"
git-rebase--common.sh +1 -0
index 7e39d22871..a8a44608e0 100644
@@ -19,6 +19,7 @@ write_basic_state () {
 		"$state_dir"/allow_rerere_autoupdate
 	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
 	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
+	test -n "$reschedule_failed_exec" && : > "$state_dir"/reschedule-failed-exec
 }
 
 apply_autostash () {
sequencer.c +10 -3
index e1a4dd15f1..69bee63e11 100644
@@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
 static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
+static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
 
 static int git_sequencer_config(const char *k, const char *v, void *cb)
 {
@@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)
 			opts->signoff = 1;
 		}
 
+		if (file_exists(rebase_path_reschedule_failed_exec()))
+			opts->reschedule_failed_exec = 1;
+
 		read_strategy_opts(opts, &buf);
 		strbuf_release(&buf);
 
@@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 		write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
 	if (opts->signoff)
 		write_file(rebase_path_signoff(), "--signoff\n");
+	if (opts->reschedule_failed_exec)
+		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
 
 	return 0;
 }
@@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 			*end_of_arg = saved;
 
 			/* Reread the todo file if it has changed. */
-			if (res)
-				; /* fall through */
-			else if (stat(get_todo_path(opts), &st))
+			if (res) {
+				if (opts->reschedule_failed_exec)
+					reschedule = 1;
+			} else if (stat(get_todo_path(opts), &st))
 				res = error_errno(_("could not stat '%s'"),
 						  get_todo_path(opts));
 			else if (match_stat_data(&todo_list->stat, &st)) {
sequencer.h +1 -0
index 5071a73563..1f865dae26 100644
@@ -39,6 +39,7 @@ struct replay_opts {
 	int allow_empty_message;
 	int keep_redundant_commits;
 	int verbose;
+	int reschedule_failed_exec;
 
 	int mainline;
 
t/t3418-rebase-continue.sh +6 -0
index 0210b2ac6f..54b26a9284 100755
@@ -254,4 +254,10 @@ test_expect_success 'the todo command "break" works' '
 	test_path_is_file execed
 '
 
+test_expect_success '--reschedule-failed-exec' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
+	grep "^exec false" .git/rebase-merge/git-rebase-todo
+'
+
 test_done
Signature · 1 line
gitgitgadget
#5 lore raw

PATCH 2/3 rebase: add a config option to default to --reschedule-failed-exec

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

It would be cumbersome to type out that option all the time, so let's offer the convenience of a config setting: rebase.rescheduleFailedExec.

Besides, this opens the door to changing the default in a future version of Git: it does make some sense to reschedule failed exec commands by default (and if we could go back in time when the exec command was invented, we probably would change that default right from the start).

---
 Documentation/config/rebase.txt | 5 +++++
 builtin/rebase.c                | 5 +++++
 git-legacy-rebase.sh            | 2 ++
 t/t3418-rebase-continue.sh      | 7 ++++++-
 4 files changed, 18 insertions(+), 1 deletion(-)
Documentation/config/rebase.txt +5 -0
index f079bf6b7e..331d250e04 100644
@@ -64,3 +64,8 @@ instead of:
 -------------------------------------------
 +
 Defaults to false.
+
+rebase.rescheduleFailedExec::
+	Automatically reschedule `exec` commands that failed. This only makes
+	sense in interactive mode (or when an `--exec` option was provided).
+	This is the same as specifying the `--reschedule-failed-exec` option.
builtin/rebase.c +5 -0
index 6d556fc6c8..06e450b537 100644
@@ -677,6 +677,11 @@ static int rebase_config(const char *var, const char *value, void *data)
 		return 0;
 	}
 
+	if (!strcmp(var, "rebase.reschedulefailedexec")) {
+		opts->reschedule_failed_exec = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, data);
 }
 
git-legacy-rebase.sh +2 -0
index 5f0f0c5ab8..1b268a5fcc 100755
@@ -99,6 +99,8 @@ case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
 *)	gpg_sign_opt= ;;
 esac
+test "$(git config --bool rebase.reschedulefailedexec)" = "true" &&
+reschedule_failed_exec=--reschedule-failed-exec
 . git-rebase--common
 
 read_basic_state () {
t/t3418-rebase-continue.sh +6 -1
index 54b26a9284..bdaa511bb0 100755
@@ -257,7 +257,12 @@ test_expect_success 'the todo command "break" works' '
 test_expect_success '--reschedule-failed-exec' '
 	test_when_finished "git rebase --abort" &&
 	test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
-	grep "^exec false" .git/rebase-merge/git-rebase-todo
+	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
+	git rebase --abort &&
+	test_must_fail git -c rebase.rescheduleFailedExec=true \
+		rebase -x false HEAD^ 2>err &&
+	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
+	test_i18ngrep "has been rescheduled" err
 '
 
 test_done
Signature · 1 line
gitgitgadget
#20 lore raw

PATCH 3/3 rebase: introduce a shortcut for --reschedule-failed-exec

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

It is a bit cumbersome to write out the --reschedule-failed-exec option before -x <cmd> all the time; let's introduce a convenient option to do both at the same time: -y <cmd>.

---
 Documentation/git-rebase.txt |  6 ++++++
 builtin/rebase.c             | 21 +++++++++++++++++++++
 git-legacy-rebase.sh         |  6 ++++++
 t/t3418-rebase-continue.sh   |  3 +++
 4 files changed, 36 insertions(+)
Documentation/git-rebase.txt +6 -0
index 9dd68f77f6..99ca589ffc 100644
@@ -462,6 +462,12 @@ without an explicit `--interactive`.
 +
 See also INCOMPATIBLE OPTIONS below.
 
+-y <cmd>::
+	This is the same as passing `--reschedule-failed-exec` before
+	`-x <cmd>`, i.e. it appends the specified `exec` command and
+	turns on the mode where failed `exec` commands are automatically
+	rescheduled.
+
 --root::
 	Rebase all commits reachable from <branch>, instead of
 	limiting them with an <upstream>.  This allows you to rebase
builtin/rebase.c +21 -0
index 06e450b537..b707ccf00f 100644
@@ -754,6 +754,23 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
 	return 0;
 }
 
+struct opt_y {
+	struct string_list *list;
+	struct rebase_options *options;
+};
+
+static int parse_opt_y(const struct option *opt, const char *arg, int unset)
+{
+	struct opt_y *o = opt->value;
+
+	if (unset || !arg)
+		return -1;
+
+	o->options->reschedule_failed_exec = 1;
+	string_list_append(o->list, arg);
+	return 0;
+}
+
 static void NORETURN error_on_missing_default_upstream(void)
 {
 	struct branch *current_branch = branch_get(NULL);
@@ -817,6 +834,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
 	struct object_id squash_onto;
 	char *squash_onto_name = NULL;
+	struct opt_y opt_y = { .list = &exec, .options = &options };
 	struct option builtin_rebase_options[] = {
 		OPT_STRING(0, "onto", &options.onto_name,
 			   N_("revision"),
@@ -894,6 +912,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
 				N_("add exec lines after each commit of the "
 				   "editable list")),
+		{ OPTION_CALLBACK, 'y', NULL, &opt_y, N_("<cmd>"),
+			N_("same as --reschedule-failed-exec -x <cmd>"),
+			PARSE_OPT_NONEG, parse_opt_y },
 		OPT_BOOL(0, "allow-empty-message",
 			 &options.allow_empty_message,
 			 N_("allow rebasing commits with empty messages")),
git-legacy-rebase.sh +6 -0
index 1b268a5fcc..8048891fed 100755
@@ -26,6 +26,7 @@ f,force-rebase!    cherry-pick all commits, even if unchanged
 m,merge!           use merging strategies to rebase
 i,interactive!     let the user edit the list of commits to rebase
 x,exec=!           add exec lines after each commit of the editable list
+y=!                same as --reschedule-failed-exec -x
 k,keep-empty	   preserve empty commits during rebase
 allow-empty-message allow rebasing commits with empty messages
 stat!              display a diffstat of what changed upstream
@@ -262,6 +263,11 @@ do
 		cmd="${cmd}exec ${1#--exec=}${LF}"
 		test -z "$interactive_rebase" && interactive_rebase=implied
 		;;
+	-y*)
+		reschedule_failed_exec=--reschedule-failed-exec
+		cmd="${cmd}exec ${1#-y}${LF}"
+		test -z "$interactive_rebase" && interactive_rebase=implied
+		;;
 	--interactive)
 		interactive_rebase=explicit
 		;;
t/t3418-rebase-continue.sh +3 -0
index bdaa511bb0..25aaacacfc 100755
@@ -262,6 +262,9 @@ test_expect_success '--reschedule-failed-exec' '
 	test_must_fail git -c rebase.rescheduleFailedExec=true \
 		rebase -x false HEAD^ 2>err &&
 	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
+	test_i18ngrep "has been rescheduled" err &&
+	git rebase --abort &&
+	test_must_fail git rebase -y false HEAD^ 2>err &&
 	test_i18ngrep "has been rescheduled" err
 '
 
Signature · 1 line
gitgitgadget
#21 lore raw
In reply to Johannes Schindelin via GitGitGadget

Am 10.12.18 um 20:04 schrieb Johannes Schindelin via GitGitGadget:

Quoted 4 lines

The idea was brought up by Paul Morelle.

To be honest, this idea of rescheduling a failed exec makes so much sense that I wish we had done this from the get-go.

The status quo was actually not that bad a decision, because it made 'x false' as a substitute for 'break' very convenient.

But now that we have a real 'break', I'm very much in favor of flipping the behavior over to rescheduling. (I'm actually not a user of the feature, but the proposed behavior is so compellingly logical.)

-- Hannes

#22 lore raw
Quoted 7 lines · Johannes Sixt

Am 10.12.18 um 20:04 schrieb Johannes Schindelin via GitGitGadget:

Quoted 4 lines

The idea was brought up by Paul Morelle.

To be honest, this idea of rescheduling a failed exec makes so much sense that I wish we had done this from the get-go.

Quoted 6 lines

The status quo was actually not that bad a decision, because it made 'x false' as a substitute for 'break' very convenient.

But now that we have a real 'break', I'm very much in favor of flipping the behavior over to rescheduling. (I'm actually not a user of the feature, but the proposed behavior is so compellingly logical.)

In rare cases I had commands that may be dangerous if rerun, but I'd just not run them with -y but with -x.

That brings me to some confusion I had in the last patch:
To catch a flaky test I surely would be tempted to
git rebase -x make -y "make test"
but that might reschedule a compile failure as well, as a single -y has implications on all other -x's.

I wonder if it might be better to push this mechanism one layer down: Instead of having a flag that changes the behavior of the "exec" instructions and having a handy '-y' short cut for the new mode, we'd rather have a new type of command that executes&retries a command ("exnrt", 'n'), which can still get the '-y' command line flag, but more importantly by having 2 separate sets of commands we'd have one set that is a one-shot, and the other that is retried. Then we can teach the user which is safe and which isn't for rescheduling.

By having two classes, I would anticipate fewer compatibility issues ('"Exec" behaves differently, and I forgot I had turned on the rescheduling').

Talking about rescheduling: In the above example the flaky test can flake more than once, so I'd be stuck with keeping 'git rebase --continue'ing after I see the test flaked once again.

My workflow with interactive rebase and fixing up things as I go always involves a manual final "make test" to check "for real", which I could lose now, which is nice.

Thanks,
Stefan

#26 lore raw
In reply to Johannes Schindelin via GitGitGadget
Quoted 7 lines · Johannes Schindelin via GitGitGadget

The idea was brought up by Paul Morelle.

To be honest, this idea of rescheduling a failed exec makes so much sense that I wish we had done this from the get-go.

So let's do the next best thing: implement a command-line option and a config setting to make it so.

I was a bit worried that the optics weren't good enough for recovering from a typoed exec command (or one that otherwise wouldn't be in a state the user could make pass but they wanted the rebase to continue). However, after trying it out, I found:

$ git rebase --exec /bin/false --reschedule-failed-exec HEAD~1
Executing: /bin/false
warning: execution failed: /bin/false
You can fix the problem, and then run
  git rebase --continue

hint: Could not execute the todo command
hint:
hint: exec /bin/false
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:

hint:     git rebase --edit-todo
hint:     git rebase --continue

and if the user just runs "git rebase --continue" without looking at that big hint, they'll get the hint again. When they run "git rebase --edit-todo", they see the failed command listed first and can remove that line.

So, I think my initial worry was unfounded.

Quoted 10 lines

The obvious intent behind that config setting is to not only give users a way to opt into that behavior already, but also to make it possible to flip the default at a later date, after the feature has been battle-tested and after deprecating the non-rescheduling behavior for a couple of Git versions.

If the team agrees with my reasoning, then the 3rd patch (introducing -y <cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make much sense, as it would introduce a short option that would become obsolete soon.

This series is awesome; thanks much to Paul for suggesting this idea. And yeah, I agree and hope the third patch won't be necessary. :-)

Quoted 23 lines

Johannes Schindelin (3):
rebase: introduce --reschedule-failed-exec
rebase: add a config option to default to --reschedule-failed-exec
rebase: introduce a shortcut for --reschedule-failed-exec

 Documentation/config/rebase.txt |  5 ++++
 Documentation/git-rebase.txt    | 11 +++++++++
 builtin/rebase--interactive.c   |  2 ++
 builtin/rebase.c                | 42 ++++++++++++++++++++++++++++++++-
 git-legacy-rebase.sh            | 24 ++++++++++++++++++-
 git-rebase--common.sh           |  1 +
 sequencer.c                     | 13 +++++++---
 sequencer.h                     |  1 +
 t/t3418-rebase-continue.sh      | 14 +++++++++++
 9 files changed, 108 insertions(+), 5 deletions(-)

base-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-90%2Fdscho%2Freschedule-failed-exec-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-90/dscho/reschedule-failed-exec-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/90

Signature · 1 line
gitgitgadget
#2 lore raw

Re: [PATCH 1/3] rebase: introduce --reschedule-failed-exec

In reply to Johannes Schindelin via GitGitGadget
Quoted 86 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

A common use case for the --exec option is to verify that each commit in a topic branch compiles cleanly, via git rebase -x make <base>.

However, when an exec in such a rebase fails, it is not re-scheduled, which in this instance is not particularly helpful.

Let's offer a flag to reschedule failed exec commands.

Based on an idea by Paul Morelle.

---
 Documentation/git-rebase.txt  |  5 +++++
 builtin/rebase--interactive.c |  2 ++
 builtin/rebase.c              | 16 +++++++++++++++-
 git-legacy-rebase.sh          | 16 +++++++++++++++-
 git-rebase--common.sh         |  1 +
 sequencer.c                   | 13 ++++++++++---
 sequencer.h                   |  1 +
 t/t3418-rebase-continue.sh    |  6 ++++++
 8 files changed, 55 insertions(+), 5 deletions(-)
Documentation/git-rebase.txt +5 -0
index 80793bad8d..9dd68f77f6 100644
@@ -501,6 +501,11 @@ See also INCOMPATIBLE OPTIONS below.
        with care: the final stash application after a successful
        rebase might result in non-trivial conflicts.
 
+--reschedule-failed-exec::
+--no-reschedule-failed-exec::
+       Automatically reschedule `exec` commands that failed. This only makes
+       sense in interactive mode (or when an `--exec` option was provided).
+
 INCOMPATIBLE OPTIONS
 --------------------
 
builtin/rebase--interactive.c +2 -0
index a2ab68ed06..a9ab009fdb 100644
@@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
                OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
                OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
                OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
+               OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
+                        N_("automatically re-schedule any `exec` that fails")),
                OPT_END()
        };
 
builtin/rebase.c +6 -0
index 5b3e5baec8..6d556fc6c8 100644
@@ -104,6 +104,7 @@ struct rebase_options {
        int rebase_merges, rebase_cousins;
        char *strategy, *strategy_opts;
        struct strbuf git_format_patch_opt;
+       int reschedule_failed_exec;
 };
 
 static int is_interactive(struct rebase_options *opts)
@@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)
                        argv_array_push(&child.args, opts->gpg_sign_opt);
                if (opts->signoff)
                        argv_array_push(&child.args, "--signoff");
+               if (opts->reschedule_failed_exec)
+                       argv_array_push(&child.args, "--reschedule-failed-exec");
 
                status = run_command(&child);
                goto finished_rebase;
@@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                                   "strategy")),
                OPT_BOOL(0, "root", &options.root,
                         N_("rebase all reachable commits up to the root(s)")),
+               OPT_BOOL(0, "reschedule-failed-exec",
+                        &options.reschedule_failed_exec,
+                        N_("automatically re-schedule any `exec` that fails")),
                OPT_END(),
        };
        int i;

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1206

Quoted 6 lines
builtin/rebase.c +3 -0
@@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                break;
        }
 
+       if (options.reschedule_failed_exec && !is_interactive(&options))
+               die(_("--reschedule-failed-exec requires an interactive rebase"));
+

I was surprised at first that you checked is_interactive() rather than checking for --exec being specified. But I guess this is because users can manually specify 'exec' lines.

What if the user specifies an implicitly interactive rebase (i.e. no editing of the todo list, such as with --rebase-merges or --keep-empty, or soon --strategy or --strategy-option) and also doesn't specify --exec?

Quoted 77 lines
builtin/rebase.c +6 -1
        if (options.git_am_opts.argc) {
                /* all am options except -q are compatible only with --am */
                for (i = options.git_am_opts.argc - 1; i >= 0; i--)
@@ -1220,7 +1229,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                options.flags |= REBASE_FORCE;
        }
 
-       if (options.type == REBASE_PRESERVE_MERGES)
+       if (options.type == REBASE_PRESERVE_MERGES) {
                /*
                 * Note: incompatibility with --signoff handled in signoff block above
                 * Note: incompatibility with --interactive is just a strong warning;
@@ -1230,6 +1239,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                        die(_("error: cannot combine '--preserve-merges' with "
                              "'--rebase-merges'"));
 
+               if (options.reschedule_failed_exec)
+                       die(_("error: cannot combine '--preserve-merges' with "
+                             "'--reschedule-failed-exec'"));
+       }
+
        if (options.rebase_merges) {
                if (strategy_options.nr)
                        die(_("error: cannot combine '--rebase-merges' with "
git-legacy-rebase.sh +12 -1
index b97ffdc9dd..5f0f0c5ab8 100755
@@ -48,6 +48,7 @@ skip!              skip current patch and continue
 edit-todo!         edit the todo list during an interactive rebase
 quit!              abort but keep HEAD where it is
 show-current-patch! show the patch file being applied or merged
+reschedule-failed-exec automatically reschedule failed exec commands
 "
 . git-sh-setup
 set_reflog_action rebase
@@ -92,6 +93,7 @@ autosquash=
 keep_empty=
 allow_empty_message=--allow-empty-message
 signoff=
+reschedule_failed_exec=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 case "$(git config --bool commit.gpgsign)" in
 true)  gpg_sign_opt=-S ;;
@@ -126,6 +128,8 @@ read_basic_state () {
                signoff="$(cat "$state_dir"/signoff)"
                force_rebase=t
        }
+       test -f "$state_dir"/reschedule-failed-exec &&
+               reschedule_failed_exec=t
 }
 
 finish_rebase () {
@@ -163,7 +167,8 @@ run_interactive () {
                "$allow_empty_message" "$autosquash" "$verbose" \
                "$force_rebase" "$onto_name" "$head_name" "$strategy" \
                "$strategy_opts" "$cmd" "$switch_to" \
-               "$allow_rerere_autoupdate" "$gpg_sign_opt" "$signoff"
+               "$allow_rerere_autoupdate" "$gpg_sign_opt" "$signoff" \
+               "$reschedule_failed_exec"
 }
 
 run_specific_rebase () {
@@ -378,6 +383,12 @@ do
        --gpg-sign=*)
                gpg_sign_opt="-S${1#--gpg-sign=}"
                ;;
+       --reschedule-failed-exec)
+               reschedule_failed_exec=--reschedule-failed-exec
+               ;;
+       --no-reschedule-failed-exec)
+               reschedule_failed_exec=
+               ;;
        --)
                shift
                break

git-legacy-rebase.sh then · line 553

Quoted 9 lines
git-legacy-rebase.sh +3 -0
@@ -534,6 +545,9 @@ then
        #       git-rebase.txt caveats with "unless you know what you are doing"
        test -n "$rebase_merges" &&
                die "$(gettext "error: cannot combine '--preserve-merges' with '--rebase-merges'")"
+
+       test -n "$reschedule_failed_exec" &&
+               die "$(gettext "error: cannot combine '--preserve-merges' with '--reschedule-failed-exec'")"
 fi
 
 if test -n "$rebase_merges"

In the builtin rebase, you checked that --reschedule-failed-exec had to be used with an interactive rebase. Here in the legacy rebase you have no such check at all.

Not sure if that's an oversight, or if we're at the point where we just start intentionally allowing legacy rebase to lag and soon throw it out. (When do we get to that point?)

Quoted 74 lines
git-rebase--common.sh +1 -0
index 7e39d22871..a8a44608e0 100644
@@ -19,6 +19,7 @@ write_basic_state () {
                "$state_dir"/allow_rerere_autoupdate
        test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
        test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
+       test -n "$reschedule_failed_exec" && : > "$state_dir"/reschedule-failed-exec
 }
 
 apply_autostash () {
sequencer.c +10 -3
index e1a4dd15f1..69bee63e11 100644
@@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
 static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
+static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
 
 static int git_sequencer_config(const char *k, const char *v, void *cb)
 {
@@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)
                        opts->signoff = 1;
                }
 
+               if (file_exists(rebase_path_reschedule_failed_exec()))
+                       opts->reschedule_failed_exec = 1;
+
                read_strategy_opts(opts, &buf);
                strbuf_release(&buf);
 
@@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
                write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
        if (opts->signoff)
                write_file(rebase_path_signoff(), "--signoff\n");
+       if (opts->reschedule_failed_exec)
+               write_file(rebase_path_reschedule_failed_exec(), "%s", "");
 
        return 0;
 }
@@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
                        *end_of_arg = saved;
 
                        /* Reread the todo file if it has changed. */
-                       if (res)
-                               ; /* fall through */
-                       else if (stat(get_todo_path(opts), &st))
+                       if (res) {
+                               if (opts->reschedule_failed_exec)
+                                       reschedule = 1;
+                       } else if (stat(get_todo_path(opts), &st))
                                res = error_errno(_("could not stat '%s'"),
                                                  get_todo_path(opts));
                        else if (match_stat_data(&todo_list->stat, &st)) {
sequencer.h +1 -0
index 5071a73563..1f865dae26 100644
@@ -39,6 +39,7 @@ struct replay_opts {
        int allow_empty_message;
        int keep_redundant_commits;
        int verbose;
+       int reschedule_failed_exec;
 
        int mainline;
 
t/t3418-rebase-continue.sh +0 -0
index 0210b2ac6f..54b26a9284 100755

t/t3418-rebase-continue.sh test_expect_success 'the todo command "break" works' ' · line 263

Quoted 10 lines
t/t3418-rebase-continue.sh +6 -0
@@ -254,4 +254,10 @@ test_expect_success 'the todo command "break" works' '
        test_path_is_file execed
 '
 
+test_expect_success '--reschedule-failed-exec' '
+       test_when_finished "git rebase --abort" &&
+       test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
+       grep "^exec false" .git/rebase-merge/git-rebase-todo
+'
+
 test_done

The rest of the patch looks good to me.

#27 lore raw
In reply to Johannes Schindelin via GitGitGadget
Quoted 15 lines · Johannes Schindelin via GitGitGadget

The idea was brought up by Paul Morelle.

To be honest, this idea of rescheduling a failed exec makes so much sense that I wish we had done this from the get-go.

So let's do the next best thing: implement a command-line option and a config setting to make it so.

The obvious intent behind that config setting is to not only give users a way to opt into that behavior already, but also to make it possible to flip the default at a later date, after the feature has been battle-tested and after deprecating the non-rescheduling behavior for a couple of Git versions.

Quoted 5 lines

If the team agrees with my reasoning, then the 3rd patch (introducing -y <cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make much sense, as it would introduce a short option that would become obsolete soon.

Complete side-track: This email showed up for me just five minutes ago, whereas the rest of the series showed up four hours ago, making me think this email had disappeared and trying to figure out how to respond when I didn't have the original. Any ideas why there might be that level of lag?

#23 lore raw
In reply to Stefan Beller
Quoted 9 lines · Stefan Beller

I wonder if it might be better to push this mechanism one layer down: Instead of having a flag that changes the behavior of the "exec" instructions and having a handy '-y' short cut for the new mode, we'd rather have a new type of command that executes&retries a command
...
By having two classes, I would anticipate fewer compatibility issues ('"Exec" behaves differently, and I forgot I had turned on the rescheduling').

It takes us back to the original proposal that started this whole thing.

    cf. <[email protected]>

After re-reading the thread, I still do not quite follow why it was considered better to change the way "exec" is run with the command line option than to implement this as a new insn [*1*], but that is where this series fit in the larger picture, I think.

[Footnote]

*1* The original proposal called it "test" which I do not think was
a great name.

#3 lore raw

Re: [PATCH 1/3] rebase: introduce --reschedule-failed-exec

In reply to Elijah Newren

Hi Elijah,

Quoted 7 lines · Elijah Newren
Quoted 5 lines · Johannes Schindelin via GitGitGadget
(quoted diff) +0 -0
@@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                break;
        }
 
Quoted 7 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1206

Quoted 3 lines
builtin/rebase.c +3 -0
+       if (options.reschedule_failed_exec && !is_interactive(&options))
+               die(_("--reschedule-failed-exec requires an interactive rebase"));
+

I was surprised at first that you checked is_interactive() rather than checking for --exec being specified. But I guess this is because users can manually specify 'exec' lines.

Indeed, that is exactly the reason.

Quoted 4 lines

What if the user specifies an implicitly interactive rebase (i.e. no editing of the todo list, such as with --rebase-merges or --keep-empty, or soon --strategy or --strategy-option) and also doesn't specify --exec?

Then the todo list won't have any exec lines, and the flag is irrelevant (but does not do any harm).

... except in the case that the rebase fails at some stage, the user edits the todo list with git rebase --edit-todo and inserts an exec line.

So I would contend that it still makes sense to allow that flag in those cases, i.e. whenever the user asked for the interactive backend.

Quoted 15 lines

git-legacy-rebase.sh then · line 553

Quoted 10 lines
git-legacy-rebase.sh +3 -0
@@ -534,6 +545,9 @@ then
        #       git-rebase.txt caveats with "unless you know what you are doing"
        test -n "$rebase_merges" &&
                die "$(gettext "error: cannot combine '--preserve-merges' with '--rebase-merges'")"
+
+       test -n "$reschedule_failed_exec" &&
+               die "$(gettext "error: cannot combine '--preserve-merges' with '--reschedule-failed-exec'")"
 fi
 
 if test -n "$rebase_merges"

In the builtin rebase, you checked that --reschedule-failed-exec had to be used with an interactive rebase. Here in the legacy rebase you have no such check at all.

Quoted 3 lines

Not sure if that's an oversight, or if we're at the point where we just start intentionally allowing legacy rebase to lag and soon throw it out. (When do we get to that point?)

Good point. My thinking was that the legacy rebase does not matter all that much anymore, I would expect that we get rid of it in v2.21.0.

But you're right, I should not intentionally diverge the functionality out of sheer laziness.

Will fix.

Quoted 1 line

The rest of the patch looks good to me.

Thanks!
Dscho

#28 lore raw

email lags, was Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically

In reply to Elijah Newren

Hi Elijah,

Quoted 23 lines · Elijah Newren
Quoted 15 lines · Johannes Schindelin via GitGitGadget

The idea was brought up by Paul Morelle.

To be honest, this idea of rescheduling a failed exec makes so much sense that I wish we had done this from the get-go.

So let's do the next best thing: implement a command-line option and a config setting to make it so.

The obvious intent behind that config setting is to not only give users a way to opt into that behavior already, but also to make it possible to flip the default at a later date, after the feature has been battle-tested and after deprecating the non-rescheduling behavior for a couple of Git versions.

Quoted 5 lines

If the team agrees with my reasoning, then the 3rd patch (introducing -y <cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make much sense, as it would introduce a short option that would become obsolete soon.

Quoted 5 lines

Complete side-track: This email showed up for me just five minutes ago, whereas the rest of the series showed up four hours ago, making me think this email had disappeared and trying to figure out how to respond when I didn't have the original. Any ideas why there might be that level of lag?

I have such email woes for roughly half a year now. No idea where this comes from, whether this is some graylisting at work, or whether the <author> via GitGitGadget marks [email protected] as suspect with some mail providers and/or central lists of dubious email addresses.

At first, I thought it was only GMX, but yes, I also see it with GMail now.

Ciao,
Dscho

#24 lore raw
In reply to Stefan Beller

Hi Stefan,

Quoted 11 lines · Stefan Beller
Quoted 10 lines · Johannes Sixt

Am 10.12.18 um 20:04 schrieb Johannes Schindelin via GitGitGadget:

Quoted 4 lines

The idea was brought up by Paul Morelle.

To be honest, this idea of rescheduling a failed exec makes so much sense that I wish we had done this from the get-go.

The status quo was actually not that bad a decision, because it made 'x false' as a substitute for 'break' very convenient.

Quoted 6 lines
Quoted 3 lines

But now that we have a real 'break', I'm very much in favor of flipping the behavior over to rescheduling. (I'm actually not a user of the feature, but the proposed behavior is so compellingly logical.)

In rare cases I had commands that may be dangerous if rerun, but I'd just not run them with -y but with -x.

Please note that 3/3 (i.e. the -y option) is *really* only intended as a "I could do this if anybody *really* wanted to" patch. I actually would strongly prefer not to have this patch in git.git at all.

Quoted 17 lines

That brings me to some confusion I had in the last patch:
To catch a flaky test I surely would be tempted to
git rebase -x make -y "make test"
but that might reschedule a compile failure as well, as a single -y has implications on all other -x's.

I wonder if it might be better to push this mechanism one layer down: Instead of having a flag that changes the behavior of the "exec" instructions and having a handy '-y' short cut for the new mode, we'd rather have a new type of command that executes&retries a command ("exnrt", 'n'), which can still get the '-y' command line flag, but more importantly by having 2 separate sets of commands we'd have one set that is a one-shot, and the other that is retried. Then we can teach the user which is safe and which isn't for rescheduling.

Quoted 3 lines

By having two classes, I would anticipate fewer compatibility issues ('"Exec" behaves differently, and I forgot I had turned on the rescheduling').

As Junio points out, this brings us back to the proposal to have two different exec commands.

I am really unenthusiastic about this idea, as I find it "hard to sell":
what little benefit it would bring to have two commands that pretty much do the same thing almost all the time would be outweighed *by far* by the confusion we would sow by that.

It is amazing to me how much my perspective changed when I actually had to teach Git to new users. Things that I live with easily all of a sudden become these unnecessarily confusing road blocks that make it *so hard* to actually use Git.

Quoted 3 lines

Talking about rescheduling: In the above example the flaky test can flake more than once, so I'd be stuck with keeping 'git rebase --continue'ing after I see the test flaked once again.

No, you would not be stuck with that.

You would read the error message carefully (we do that all the time, yes?) and then run git rebase --edit-todo to remove that line before continuing.

:-)

If you feel very strongly about this, we could introduce a new option for git rebase --skip, say, git rebase --skip --skip-next-commands-too=1. (I specifically used a too-long option name, as you and I are both Germans, known to use too-long names for everything. I do not really intend to use such an awkward option name, if we decide that such an option would be a good idea. Probably more like git rebase --skip-next[=<N>].)

Quoted 3 lines

My workflow with interactive rebase and fixing up things as I go always involves a manual final "make test" to check "for real", which I could lose now, which is nice.

My workflow with git rebase -r --exec "make test" is pretty similar to yours. With the difference that I would want those commands to be rescheduled *even if* the command is flakey. Actually, *in particular* when it is flakey.

I really see myself calling

	git config --global rebase.rescheduleFailedExec true

in all my setups.

Ciao,
Dscho

#4 lore raw

Re: [PATCH 1/3] rebase: introduce --reschedule-failed-exec

In reply to Johannes Schindelin

Hi Dscho,

Quoted 27 lines · Johannes Schindelin

Hi Elijah,

Quoted 7 lines · Elijah Newren
Quoted 5 lines · Johannes Schindelin via GitGitGadget
(quoted diff) +0 -0
@@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                break;
        }
 
Quoted 7 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1206

Quoted 3 lines
builtin/rebase.c +3 -0
+       if (options.reschedule_failed_exec && !is_interactive(&options))
+               die(_("--reschedule-failed-exec requires an interactive rebase"));
+

I was surprised at first that you checked is_interactive() rather than checking for --exec being specified. But I guess this is because users can manually specify 'exec' lines.

Indeed, that is exactly the reason.

Quoted 4 lines

What if the user specifies an implicitly interactive rebase (i.e. no editing of the todo list, such as with --rebase-merges or --keep-empty, or soon --strategy or --strategy-option) and also doesn't specify --exec?

Quoted 5 lines

Then the todo list won't have any exec lines, and the flag is irrelevant (but does not do any harm).

... except in the case that the rebase fails at some stage, the user edits the todo list with git rebase --edit-todo and inserts an exec line.

Ah, good point; hadn't thought of that case. Thanks for explaining.

#25 lore raw
In reply to Johannes Schindelin
Quoted 4 lines

It is amazing to me how much my perspective changed when I actually had to teach Git to new users. Things that I live with easily all of a sudden become these unnecessarily confusing road blocks that make it *so hard* to actually use Git.

I see. Without the -y patch, this series looks good to me.

Quoted 8 lines

My workflow with git rebase -r --exec "make test" is pretty similar to yours. With the difference that I would want those commands to be rescheduled *even if* the command is flakey. Actually, *in particular* when it is flakey.

I really see myself calling

        git config --global rebase.rescheduleFailedExec true

Me, too.

Thanks,
Stefan

#6 lore raw

cover rebase: don't override --no-reschedule-failed-exec with config

In reply to Johannes Schindelin via GitGitGadget

I recently started using rebase.rescheduleFailedExec=true and noticed this bug in its implementation. It's conceptually a relatively simple fix, but as noted in 3/3 rebase being a "start this operation, run other command verbs later" has an unintuitive interaction with our usual "command-line options override config".

!README FIRST!
Everthing after this line has no relevance to this series, it's just a side musing on another (mis-)feature of --reschedule-failed-exec. !/README FIRST!

There's another bug/misfeature I've noticed with rebase.rescheduleFailedExec=true (although maybe it'll be argued by someone that it's a feature). Let's say you run:

    git rebase -x false --reschedule-failed-exec HEAD~2

You'll now land in a state of (according to our helpful PS1 code, but also the rebase state) of 2/4. Aside: Because you're rebasing 2 commits, we have 4 because of the 2x exec. So far so good and all of that's expected.

But now when you do "git rebase --continue" we'll fail as expected, but not as expected (at least I find it funny) bump the count to 3/5, then 4/6, 5/7 etc.

With my "I just read the code" hat on this makes perfect sense. Every time we process a TODO item we bump the count of processed items, and under --reschedule-failed-exec we simply push the command that just failed onto the list, hence the increasing done/TODO count when a command fails.

But I don't see how it makes any sense to expose this as UI via "git status" and __git_ps1. I asked git to process this item of 4 sequencer TODO items. If I fail an item it makes more sense that I just don't get past it, not that under the hood a new identical item is rescheduled for me. Just don't advance past the current item!

Now if I've tried X times to make the "make test" pass for each commit in my 3-commit series I'm going to be on item 10/12 or whatever. There's no way to just look at that and see where I'm at in the sequence.

As an aside it would arguably make more sense to report 1/3 instead of 2/6 for the first commit of 3 with a failing -x "make test", but you can have X number of "exec" items and a manually edited list etc.

So that's probably a no-go but at least once I'm used to it I know if i'm on 4/6 I'm on commit 2/3, with --reschedule-failed-exec you'll have no idea what 12/14 or whatever means for where you are in your 3-patch sequence, it has no relation to the TODO list you edited, just rebase-merge's own internal state.

Getting back on topic: This just seems like needlessly exposing an implementation detail, I also asked to "pick" a commit, but if that item "fails" e.g. due to:

    $ git rebase --continue
    You must edit all merge conflicts and then
    mark them as resolved using git add

We don't push a new "pick" item on the list and inflate the count, so why would we do that for "exec"? Just say the command failed, return its non-zero status from "git rebase --continue", and don't advance.

Maybe it is useful to keep track of the N number of failures, and e.g. report in __git_ps1:

    master|REBASE 2/6
    master|REBASE 2(tries: 1)/6
    master|REBASE 2(tries: 2)/6

Instead of the current:

    master|REBASE 2/6
    master|REBASE 3/7
    master|REBASE 4/8

To say I'm on 2/6, but that I've tried 3 times already and failed to advance past it.

Anyway, I don't have patches for this side-report/rant. Looking at the implementation it seemed more non-trivial than I was willing to quickly fix.

We bump the count fairly early before we even get to there being an "exec" item, to implement this proposed view of the world we'd need to defer that (or go back and edit it once we see "failed exec" and that we're using --reschedule-failed-exec).

I'm not familiar enough with the sequencer internals to know if trying that would lead us down a path of e.g. having inconsistent or bad state if we'd die in the middle of all of that.

Ævar Arnfjörð Bjarmason (3):
rebase tests: camel-case rebase.rescheduleFailedExec consistently rebase tests: use test_unconfig after test_config
rebase: don't override --no-reschedule-failed-exec with config

 Documentation/git-rebase.txt |  8 ++++++++
 sequencer.c                  |  5 +++++
 t/t3418-rebase-continue.sh   | 30 ++++++++++++++++++++++++++++--
 3 files changed, 41 insertions(+), 2 deletions(-)
Signature · 1 line
2.31.0.285.gb40d23e604f
#7 lore raw

PATCH 1/3 rebase tests: camel-case rebase.rescheduleFailedExec consistently

Fix a test added in 906b63942ac (rebase --am: ignore rebase.rescheduleFailedExec, 2019-07-01) to camel-case the configuration variable. This doesn't change the behavior of the test, it's merely to help its human readers.

---
 t/t3418-rebase-continue.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
t/t3418-rebase-continue.sh +2 -2
index 0838f4e798a..fe407e63cf1 100755
@@ -282,8 +282,8 @@ test_expect_success '--reschedule-failed-exec' '
 	test_i18ngrep "has been rescheduled" err
 '
 
-test_expect_success 'rebase.reschedulefailedexec only affects `rebase -i`' '
-	test_config rebase.reschedulefailedexec true &&
+test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '
+	test_config rebase.rescheduleFailedExec true &&
 	test_must_fail git rebase -x false HEAD^ &&
 	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
 	git rebase --abort &&
Signature · 1 line
2.31.0.285.gb40d23e604f
#8 lore raw

PATCH 2/3 rebase tests: use test_unconfig after test_config

In reply to Ævar Arnfjörð Bjarmason

Fix a test added in 906b63942ac (rebase --am: ignore rebase.rescheduleFailedExec, 2019-07-01) to reset its config after it runs. This doesn't matter now since it's the last test in the file, but will in a subsequent commit where I'll add new tests after this one.

---
 t/t3418-rebase-continue.sh | 1 +
 1 file changed, 1 insertion(+)
t/t3418-rebase-continue.sh +1 -0
index fe407e63cf1..ea14ef496cb 100755
@@ -283,6 +283,7 @@ test_expect_success '--reschedule-failed-exec' '
 '
 
 test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '
+	test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
 	test_config rebase.rescheduleFailedExec true &&
 	test_must_fail git rebase -x false HEAD^ &&
 	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
Signature · 1 line
2.31.0.285.gb40d23e604f
#10 lore raw

PATCH 3/3 rebase: don't override --no-reschedule-failed-exec with config

In reply to Ævar Arnfjörð Bjarmason

Fix a bug in how --no-reschedule-failed-exec interacts with rebase.rescheduleFailedExec=true being set in the config. Before this change the --no-reschedule-failed-exec config option would be overridden by the config.

This bug happened because of the particulars of how "rebase" works v.s. most other git commands when it comes to parsing options and config:

When we read the config and parse the CLI options we correctly prefer the --no-reschedule-failed-exec option over rebase.rescheduleFailedExec=true in the config. So far so good.

However the --reschedule-failed-exec option doesn't take effect when the rebase starts (we'd just create a ".git/rebase-merge/reschedule-failed-exec" file if it was true). It only takes effect when the exec command fails, and the user wants to run "rebase --continue".

At that point we'll have forgotten that we asked for --no-reschedule-failed-exec at the start, and will happily re-read the config.

We'll then see that rebase.rescheduleFailedExec=true is set. At that point we have no record of having set --no-reschedule-failed-exec earlier. So the config will effectively override the user having explicitly disabled the option on the command-line.

Even more confusingly: Since rebase accepts different options based on its state there wasn't even a way to get around this with "rebase --continue --no-reschedule-failed-exec" (but you could of course set the config with "rebase -c ...").

I think the least bad way out of this is to declare that for such options and config whatever we decide at the beginning of the rebase goes. So we'll now always create either a "reschedule-failed-exec" or a "no-reschedule-failed-exec file at the start, not just the former if we decided we wanted the feature.

With this new worldview you can no longer change the setting once a rebase has started except by manually removing the state files discussed above. I think making it work like that is the the least confusing thing we can do.

In the future we might want to learn to change the setting in the middle by combining "--edit-todo" with "--[no-]reschedule-failed-exec", we currently don't support combining those options, or any other way to change the state in the middle of the rebase short of manually editing the files in ".git/rebase-merge/*".

The bug being fixed here originally came about because of a combination of the behavior of the code added in d421afa0c66 (rebase:
introduce --reschedule-failed-exec, 2018-12-10) and the addition of the config variable in 969de3ff0e0 (rebase: add a config option to default to --reschedule-failed-exec, 2018-12-10).

---
 Documentation/git-rebase.txt |  8 ++++++++
 sequencer.c                  |  5 +++++
 t/t3418-rebase-continue.sh   | 25 +++++++++++++++++++++++++
 3 files changed, 38 insertions(+)
Documentation/git-rebase.txt +8 -0
index a0487b5cc58..b48e6225769 100644
@@ -622,6 +622,14 @@ See also INCOMPATIBLE OPTIONS below.
 --no-reschedule-failed-exec::
 	Automatically reschedule `exec` commands that failed. This only makes
 	sense in interactive mode (or when an `--exec` option was provided).
++
+Even though this option applies once a rebase is started, it's set for
+the whole rebase at the start based on either the
+`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]
+or "CONFIGURATION" below) or whether this option is
+provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
+start would be overridden by the presence of
+`rebase.rescheduleFailedExec=true` configuration.
 
 INCOMPATIBLE OPTIONS
 --------------------
sequencer.c +5 -0
index 848204d3dc3..59735fdff62 100644
@@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
 static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
+static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-reschedule-failed-exec")
 static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
 static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
 
@@ -2672,6 +2673,8 @@ static int read_populate_opts(struct replay_opts *opts)
 
 		if (file_exists(rebase_path_reschedule_failed_exec()))
 			opts->reschedule_failed_exec = 1;
+		else if (file_exists(rebase_path_no_reschedule_failed_exec()))
+			opts->reschedule_failed_exec = 0;
 
 		if (file_exists(rebase_path_drop_redundant_commits()))
 			opts->drop_redundant_commits = 1;
@@ -2772,6 +2775,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 		write_file(rebase_path_ignore_date(), "%s", "");
 	if (opts->reschedule_failed_exec)
 		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
+	else
+		write_file(rebase_path_no_reschedule_failed_exec(), "%s", "");
 
 	return 0;
 }
t/t3418-rebase-continue.sh +25 -0
index ea14ef496cb..9553d969646 100755
@@ -291,4 +291,29 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '
 	git rebase HEAD^
 '
 
+test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '
+	test_when_finished "git rebase --abort" &&
+	test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&
+	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
+'
+
 test_done
Signature · 1 line
2.31.0.285.gb40d23e604f
#14 lore raw

Re: [PATCH 0/3] rebase: don't override --no-reschedule-failed-exec with config

In reply to Ævar Arnfjörð Bjarmason

Hi Ævar,

Quoted 2 lines · Ævar Arnfjörð Bjarmason

I recently started using rebase.rescheduleFailedExec=true and noticed this bug in its implementation.

Okay, there is a bug. "This bug". Let's read on to find out what it is.

Quoted 1 line

It's conceptually a relatively simple fix,

Wait, hold on, there is a fix, but what is the bug?

Quoted 3 lines

but as noted in 3/3 rebase being a "start this operation, run other command verbs later" has an unintuitive interaction with our usual "command-line options override config".

Oh, so there is a detailed analysis of the bug, whatever it is, in the third patch in this patch series.

But what is the bug.

Quoted 4 lines

!README FIRST!
Everthing after this line has no relevance to this series, it's just a side musing on another (mis-)feature of --reschedule-failed-exec. !/README FIRST!

Hold on! I should read this first? Why is this not on top of the cover letter, then?

Oh, and wait, everything after that has no relevance to this series? Then where is the high-level description of the bug, a motivator to read this patch series?

;-)

I guess I will find out when I have set aside some time to read the patch series, which is on my back burner because nothing I read so far makes this more urgent than other tasks I had planned on addressing this week.

Or maybe you could enhance the cover letter by skipping the rant, and by giving a very rough overview of the bug at the top of the cover letter? Think of the cover letter as an elevator pitch to make me want to spend time on reviewing your patch series.

Ciao,
Dscho

Quoted 2 lines

[... snip rant about the odd progress counting when rescheduling todo commands ...]

#11 lore raw

Re: [PATCH 3/3] rebase: don't override --no-reschedule-failed-exec with config

In reply to Ævar Arnfjörð Bjarmason

Hi Ævar

Quoted 13 lines · Ævar Arnfjörð Bjarmason

Fix a bug in how --no-reschedule-failed-exec interacts with rebase.rescheduleFailedExec=true being set in the config. Before this change the --no-reschedule-failed-exec config option would be overridden by the config.

This bug happened because of the particulars of how "rebase" works v.s. most other git commands when it comes to parsing options and config:

When we read the config and parse the CLI options we correctly prefer the --no-reschedule-failed-exec option over rebase.rescheduleFailedExec=true in the config. So far so good.

Quoted 5 lines

However the --reschedule-failed-exec option doesn't take effect when the rebase starts (we'd just create a ".git/rebase-merge/reschedule-failed-exec" file if it was true). It only takes effect when the exec command fails, and the user wants to run "rebase --continue".

The exec command is rescheduled in the todo file as soon as it fails, we do not wait for the user to run 'rebase --continue' to reschedule it. However if it still fails after restarting or a later exec fails we have the problem you describe.

Quoted 14 lines

At that point we'll have forgotten that we asked for --no-reschedule-failed-exec at the start, and will happily re-read the config.

We'll then see that rebase.rescheduleFailedExec=true is set. At that point we have no record of having set --no-reschedule-failed-exec earlier. So the config will effectively override the user having explicitly disabled the option on the command-line.

Even more confusingly: Since rebase accepts different options based on its state there wasn't even a way to get around this with "rebase --continue --no-reschedule-failed-exec" (but you could of course set the config with "rebase -c ...").

Quoted 5 lines

I think the least bad way out of this is to declare that for such options and config whatever we decide at the beginning of the rebase goes. So we'll now always create either a "reschedule-failed-exec" or a "no-reschedule-failed-exec file at the start, not just the former if we decided we wanted the feature.

Thanks for working on this and for the detailed commit message. I'm not entirely convinced we want yet another state file in .git/rebase-merge. We could we start writing the setting to the file rather than having different files for whether the option is on or off. If we use the contents of the file it could be -1 for 'use config', 0 'off', 1 'on'. The downside is that starting 'rebase --no-reschedule-failed-exec' with a new version of git and then continuing with an old version would do the wrong thing.

Best Wishes

Phillip

Quoted 109 lines

With this new worldview you can no longer change the setting once a rebase has started except by manually removing the state files discussed above. I think making it work like that is the the least confusing thing we can do.

In the future we might want to learn to change the setting in the middle by combining "--edit-todo" with "--[no-]reschedule-failed-exec", we currently don't support combining those options, or any other way to change the state in the middle of the rebase short of manually editing the files in ".git/rebase-merge/*".

The bug being fixed here originally came about because of a combination of the behavior of the code added in d421afa0c66 (rebase:
introduce --reschedule-failed-exec, 2018-12-10) and the addition of the config variable in 969de3ff0e0 (rebase: add a config option to default to --reschedule-failed-exec, 2018-12-10).

---
  Documentation/git-rebase.txt |  8 ++++++++
  sequencer.c                  |  5 +++++
  t/t3418-rebase-continue.sh   | 25 +++++++++++++++++++++++++
  3 files changed, 38 insertions(+)
Documentation/git-rebase.txt +8 -0
index a0487b5cc58..b48e6225769 100644
@@ -622,6 +622,14 @@ See also INCOMPATIBLE OPTIONS below.
  --no-reschedule-failed-exec::
  	Automatically reschedule `exec` commands that failed. This only makes
  	sense in interactive mode (or when an `--exec` option was provided).
++
+Even though this option applies once a rebase is started, it's set for
+the whole rebase at the start based on either the
+`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]
+or "CONFIGURATION" below) or whether this option is
+provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
+start would be overridden by the presence of
+`rebase.rescheduleFailedExec=true` configuration.
 
  INCOMPATIBLE OPTIONS
  --------------------
sequencer.c +5 -0
index 848204d3dc3..59735fdff62 100644
@@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
  static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
  static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
  static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
+static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-reschedule-failed-exec")
  static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
  static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
 
@@ -2672,6 +2673,8 @@ static int read_populate_opts(struct replay_opts *opts)
 
  		if (file_exists(rebase_path_reschedule_failed_exec()))
  			opts->reschedule_failed_exec = 1;
+		else if (file_exists(rebase_path_no_reschedule_failed_exec()))
+			opts->reschedule_failed_exec = 0;
 
  		if (file_exists(rebase_path_drop_redundant_commits()))
  			opts->drop_redundant_commits = 1;
@@ -2772,6 +2775,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
  		write_file(rebase_path_ignore_date(), "%s", "");
  	if (opts->reschedule_failed_exec)
  		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
+	else
+		write_file(rebase_path_no_reschedule_failed_exec(), "%s", "");
 
  	return 0;
  }
t/t3418-rebase-continue.sh +25 -0
index ea14ef496cb..9553d969646 100755
@@ -291,4 +291,29 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '
  	git rebase HEAD^
  '
 
+test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '
+	test_when_finished "git rebase --abort" &&
+	test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&
+	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
+'
+
  test_done
#12 lore raw

Re: [PATCH 3/3] rebase: don't override --no-reschedule-failed-exec with config

Quoted 20 lines · Phillip Wood

Hi Ævar

Quoted 8 lines · Ævar Arnfjörð Bjarmason

Fix a bug in how --no-reschedule-failed-exec interacts with rebase.rescheduleFailedExec=true being set in the config. Before this change the --no-reschedule-failed-exec config option would be overridden by the config.
This bug happened because of the particulars of how "rebase" works v.s. most other git commands when it comes to parsing options and config:
When we read the config and parse the CLI options we correctly

Quoted 8 lines

prefer
the --no-reschedule-failed-exec option over rebase.rescheduleFailedExec=true in the config. So far so good. However the --reschedule-failed-exec option doesn't take effect when the rebase starts (we'd just create a ".git/rebase-merge/reschedule-failed-exec" file if it was true). It only takes effect when the exec command fails, and the user wants to run "rebase --continue".

Quoted 4 lines

The exec command is rescheduled in the todo file as soon as it fails, we do not wait for the user to run 'rebase --continue' to reschedule it. However if it still fails after restarting or a later exec fails we have the problem you describe.

Right, as noted in [1] I grokked those internals eventually, but the commit message is written from the viewpoint of a hypothetical user...

B.t.w. if you've had a chance to read [1] over it would be interesting to get some thoughts on that rambling. So far I only managed to upset Johannes :)

1. http://lore.kernel.org/git/[email protected]

Quoted 18 lines
Quoted 9 lines

At that point we'll have forgotten that we asked for --no-reschedule-failed-exec at the start, and will happily re-read the config.
We'll then see that rebase.rescheduleFailedExec=true is set. At that point we have no record of having set --no-reschedule-failed-exec earlier. So the config will effectively override the user having explicitly disabled the option on the command-line.
Even more confusingly: Since rebase accepts different options based on

Quoted 8 lines

its state there wasn't even a way to get around this with "rebase --continue --no-reschedule-failed-exec" (but you could of course set the config with "rebase -c ...").
I think the least bad way out of this is to declare that for such options and config whatever we decide at the beginning of the rebase goes. So we'll now always create either a "reschedule-failed-exec" or a "no-reschedule-failed-exec file at the start, not just the former if we decided we wanted the feature.

Quoted 8 lines

Thanks for working on this and for the detailed commit message. I'm not entirely convinced we want yet another state file in .git/rebase-merge. We could we start writing the setting to the file rather than having different files for whether the option is on or off. If we use the contents of the file it could be -1 for 'use config', 0 'off', 1 'on'. The downside is that starting 'rebase --no-reschedule-failed-exec' with a new version of git and then
continuing with an old version would do the wrong thing.

Yes I suppose, but it seems much simpler indeed to just represent this sort of tri-state as a ENOENT/no-FOO/FOO neither exists + file pair with how the current code is set up, especially because (as you note) we'd need to phase-in any writing of the content across multiple versions or something, least in-progress rebases across versions subtly behave weirdly.

Well, in this case it's not such a big deal, but I'd rather not establish the pattern for something that *does* matter.

Quoted 106 lines

Best Wishes

Phillip

Quoted 102 lines

With this new worldview you can no longer change the setting once a rebase has started except by manually removing the state files discussed above. I think making it work like that is the the least confusing thing we can do.
In the future we might want to learn to change the setting in the middle by combining "--edit-todo" with "--[no-]reschedule-failed-exec", we currently don't support combining those options, or any other way to change the state in the middle of the rebase short of manually editing the files in ".git/rebase-merge/*".
The bug being fixed here originally came about because of a combination of the behavior of the code added in d421afa0c66 (rebase:
introduce --reschedule-failed-exec, 2018-12-10) and the addition of the config variable in 969de3ff0e0 (rebase: add a config option to default to --reschedule-failed-exec, 2018-12-10).

---
  Documentation/git-rebase.txt |  8 ++++++++
  sequencer.c                  |  5 +++++
  t/t3418-rebase-continue.sh   | 25 +++++++++++++++++++++++++
  3 files changed, 38 insertions(+)
Documentation/git-rebase.txt +8 -0
index a0487b5cc58..b48e6225769 100644
@@ -622,6 +622,14 @@ See also INCOMPATIBLE OPTIONS below.
  --no-reschedule-failed-exec::
  	Automatically reschedule `exec` commands that failed. This only makes
  	sense in interactive mode (or when an `--exec` option was provided).
++
+Even though this option applies once a rebase is started, it's set for
+the whole rebase at the start based on either the
+`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]
+or "CONFIGURATION" below) or whether this option is
+provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
+start would be overridden by the presence of
+`rebase.rescheduleFailedExec=true` configuration.
    INCOMPATIBLE OPTIONS
  --------------------
sequencer.c +1 -0
index 848204d3dc3..59735fdff62 100644
@@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
  static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
  static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
  static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
+static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-reschedule-failed-exec")
  static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
  static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
  @@ -2672,6 +2673,8 @@ static int read_populate_opts(struct

replay_opts *opts)
if (file_exists(rebase_path_reschedule_failed_exec()))
opts->reschedule_failed_exec = 1;

sequencer.c +4 -0
+		else if (file_exists(rebase_path_no_reschedule_failed_exec()))
+			opts->reschedule_failed_exec = 0;
    		if (file_exists(rebase_path_drop_redundant_commits()))
  			opts->drop_redundant_commits = 1;
@@ -2772,6 +2775,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
  		write_file(rebase_path_ignore_date(), "%s", "");
  	if (opts->reschedule_failed_exec)
  		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
+	else
+		write_file(rebase_path_no_reschedule_failed_exec(), "%s", "");
    	return 0;
  }
t/t3418-rebase-continue.sh +24 -1
index ea14ef496cb..9553d969646 100755
@@ -291,4 +291,29 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '
  	git rebase HEAD^
  '
  +test_expect_success 'rebase.rescheduleFailedExec=true &
--no-reschedule-failed-exec' '
+	test_when_finished "git rebase --abort" &&
+	test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&
+	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
+'
+
  test_done
#13 lore raw

Re: [PATCH 3/3] rebase: don't override --no-reschedule-failed-exec with config

Quoted 23 lines · Ævar Arnfjörð Bjarmason
Quoted 20 lines · Phillip Wood

Hi Ævar

Quoted 16 lines · Ævar Arnfjörð Bjarmason

Fix a bug in how --no-reschedule-failed-exec interacts with rebase.rescheduleFailedExec=true being set in the config. Before this change the --no-reschedule-failed-exec config option would be overridden by the config.
This bug happened because of the particulars of how "rebase" works v.s. most other git commands when it comes to parsing options and config:
When we read the config and parse the CLI options we correctly prefer
the --no-reschedule-failed-exec option over rebase.rescheduleFailedExec=true in the config. So far so good. However the --reschedule-failed-exec option doesn't take effect when the rebase starts (we'd just create a ".git/rebase-merge/reschedule-failed-exec" file if it was true). It only takes effect when the exec command fails, and the user wants to run "rebase --continue".

Quoted 7 lines
Quoted 4 lines

The exec command is rescheduled in the todo file as soon as it fails, we do not wait for the user to run 'rebase --continue' to reschedule it. However if it still fails after restarting or a later exec fails we have the problem you describe.

Right, as noted in [1] I grokked those internals eventually, but the commit message is written from the viewpoint of a hypothetical user...

So user will find that the first failed exec is not rescheduled but subsequent ones are which looks like the setting only takes effect after the run 'git rebase --continue' - fair enough.

Quoted 3 lines

B.t.w. if you've had a chance to read [1] over it would be interesting to get some thoughts on that rambling. So far I only managed to upset Johannes :)

I'll try and have a proper look at it tomorrow, I did have a glance the other day - I'm not that keen on the way the prompt behaves either

Quoted 29 lines

1. http://lore.kernel.org/git/[email protected]

Quoted 18 lines
Quoted 9 lines

At that point we'll have forgotten that we asked for --no-reschedule-failed-exec at the start, and will happily re-read the config.
We'll then see that rebase.rescheduleFailedExec=true is set. At that point we have no record of having set --no-reschedule-failed-exec earlier. So the config will effectively override the user having explicitly disabled the option on the command-line.
Even more confusingly: Since rebase accepts different options based on

Quoted 8 lines

its state there wasn't even a way to get around this with "rebase --continue --no-reschedule-failed-exec" (but you could of course set the config with "rebase -c ...").
I think the least bad way out of this is to declare that for such options and config whatever we decide at the beginning of the rebase goes. So we'll now always create either a "reschedule-failed-exec" or a "no-reschedule-failed-exec file at the start, not just the former if we decided we wanted the feature.

Quoted 8 lines

Thanks for working on this and for the detailed commit message. I'm not entirely convinced we want yet another state file in .git/rebase-merge. We could we start writing the setting to the file rather than having different files for whether the option is on or off. If we use the contents of the file it could be -1 for 'use config', 0 'off', 1 'on'. The downside is that starting 'rebase --no-reschedule-failed-exec' with a new version of git and then
continuing with an old version would do the wrong thing.

Quoted 3 lines

Yes I suppose, but it seems much simpler indeed to just represent this sort of tri-state as a ENOENT/no-FOO/FOO neither exists + file pair with how the current code is set up,

I can see that, I'm not a great fan of the current setup though - we're not using shell scripting anymore so there's no need to add a new file for each piece of state (though we need to keep the old ones for backwards compatibility). Having said that I don't object to the current approach if it's easier and preserves backwards compatibility. If you feel like adding support for ENOENT="use config" that would be great so that 'git -c ... rebase --continue' continues to work.

Best Wishes

Phillip

Quoted 114 lines

especially because (as you note) we'd
need to phase-in any writing of the content across multiple versions or something, least in-progress rebases across versions subtly behave weirdly.

Well, in this case it's not such a big deal, but I'd rather not establish the pattern for something that *does* matter.

Quoted 106 lines

Best Wishes

Phillip

Quoted 102 lines

With this new worldview you can no longer change the setting once a rebase has started except by manually removing the state files discussed above. I think making it work like that is the the least confusing thing we can do.
In the future we might want to learn to change the setting in the middle by combining "--edit-todo" with "--[no-]reschedule-failed-exec", we currently don't support combining those options, or any other way to change the state in the middle of the rebase short of manually editing the files in ".git/rebase-merge/*".
The bug being fixed here originally came about because of a combination of the behavior of the code added in d421afa0c66 (rebase:
introduce --reschedule-failed-exec, 2018-12-10) and the addition of the config variable in 969de3ff0e0 (rebase: add a config option to default to --reschedule-failed-exec, 2018-12-10).

---
   Documentation/git-rebase.txt |  8 ++++++++
   sequencer.c                  |  5 +++++
   t/t3418-rebase-continue.sh   | 25 +++++++++++++++++++++++++
   3 files changed, 38 insertions(+)
Documentation/git-rebase.txt +8 -0
index a0487b5cc58..b48e6225769 100644
@@ -622,6 +622,14 @@ See also INCOMPATIBLE OPTIONS below.
   --no-reschedule-failed-exec::
   	Automatically reschedule `exec` commands that failed. This only makes
   	sense in interactive mode (or when an `--exec` option was provided).
++
+Even though this option applies once a rebase is started, it's set for
+the whole rebase at the start based on either the
+`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]
+or "CONFIGURATION" below) or whether this option is
+provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
+start would be overridden by the presence of
+`rebase.rescheduleFailedExec=true` configuration.
     INCOMPATIBLE OPTIONS
   --------------------
sequencer.c +1 -0
index 848204d3dc3..59735fdff62 100644
@@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
   static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
   static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
   static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
+static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-reschedule-failed-exec")
   static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
   static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
   @@ -2672,6 +2673,8 @@ static int read_populate_opts(struct

replay_opts *opts)
if (file_exists(rebase_path_reschedule_failed_exec()))
opts->reschedule_failed_exec = 1;

sequencer.c +4 -0
+		else if (file_exists(rebase_path_no_reschedule_failed_exec()))
+			opts->reschedule_failed_exec = 0;
     		if (file_exists(rebase_path_drop_redundant_commits()))
   			opts->drop_redundant_commits = 1;
@@ -2772,6 +2775,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
   		write_file(rebase_path_ignore_date(), "%s", "");
   	if (opts->reschedule_failed_exec)
   		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
+	else
+		write_file(rebase_path_no_reschedule_failed_exec(), "%s", "");
     	return 0;
   }
t/t3418-rebase-continue.sh +24 -1
index ea14ef496cb..9553d969646 100755
@@ -291,4 +291,29 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '
   	git rebase HEAD^
   '
   +test_expect_success 'rebase.rescheduleFailedExec=true &
--no-reschedule-failed-exec' '
+	test_when_finished "git rebase --abort" &&
+	test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&
+	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
+'
+
   test_done
#15 lore raw

Re: [PATCH 0/3] rebase: don't override --no-reschedule-failed-exec with config

In reply to Ævar Arnfjörð Bjarmason

Hi Ævar

Quoted 38 lines · Ævar Arnfjörð Bjarmason

I recently started using rebase.rescheduleFailedExec=true and noticed this bug in its implementation. It's conceptually a relatively simple fix, but as noted in 3/3 rebase being a "start this operation, run other command verbs later" has an unintuitive interaction with our usual "command-line options override config".

!README FIRST!
Everthing after this line has no relevance to this series, it's just a side musing on another (mis-)feature of --reschedule-failed-exec. !/README FIRST!

There's another bug/misfeature I've noticed with rebase.rescheduleFailedExec=true (although maybe it'll be argued by someone that it's a feature). Let's say you run:

     git rebase -x false --reschedule-failed-exec HEAD~2

You'll now land in a state of (according to our helpful PS1 code, but also the rebase state) of 2/4. Aside: Because you're rebasing 2 commits, we have 4 because of the 2x exec. So far so good and all of that's expected.

But now when you do "git rebase --continue" we'll fail as expected, but not as expected (at least I find it funny) bump the count to 3/5, then 4/6, 5/7 etc.

With my "I just read the code" hat on this makes perfect sense. Every time we process a TODO item we bump the count of processed items, and under --reschedule-failed-exec we simply push the command that just failed onto the list, hence the increasing done/TODO count when a command fails.

But I don't see how it makes any sense to expose this as UI via "git status" and __git_ps1. I asked git to process this item of 4 sequencer TODO items. If I fail an item it makes more sense that I just don't get past it, not that under the hood a new identical item is rescheduled for me. Just don't advance past the current item!

Quoted 8 lines

Now if I've tried X times to make the "make test" pass for each commit in my 3-commit series I'm going to be on item 10/12 or whatever. There's no way to just look at that and see where I'm at in the sequence.

As an aside it would arguably make more sense to report 1/3 instead of 2/6 for the first commit of 3 with a failing -x "make test", but you can have X number of "exec" items and a manually edited list etc.

I think counting the number of picks rather than total commands is probably better especially now that we have reset and label commands as well. Grouping all the execs with the previous pick doesn't seem that unreasonable to me (though I should note that I don't use the prompt from git.git and my prompt prints the command that we've stopped at on a line above the prompt so it is easy for me to see where I am in the list).

Quoted 10 lines

So that's probably a no-go but at least once I'm used to it I know if i'm on 4/6 I'm on commit 2/3, with --reschedule-failed-exec you'll have no idea what 12/14 or whatever means for where you are in your 3-patch sequence, it has no relation to the TODO list you edited, just rebase-merge's own internal state.

Getting back on topic: This just seems like needlessly exposing an implementation detail, I also asked to "pick" a commit, but if that item "fails" e.g. due to:

Quoted 6 lines
     $ git rebase --continue
     You must edit all merge conflicts and then
     mark them as resolved using git add

We don't push a new "pick" item on the list and inflate the count, so why would we do that for "exec"?

We do push a new pick if a pick/merge/reset fails because it would overwrite untracked files.

As an aside I wish I could skip a rescheduled pick or exec with 'git rebase --skip' rather than having to edit the todo list (skipping reset commands is a bad idea though)

Quoted 9 lines

Just say the command failed, return
its non-zero status from "git rebase --continue", and don't advance.

Maybe it is useful to keep track of the N number of failures, and e.g. report in __git_ps1:

     master|REBASE 2/6
     master|REBASE 2(tries: 1)/6
     master|REBASE 2(tries: 2)/6

Something like that would be nice but I think just not incrementing the count when we reschedule a command would be fine.

Quoted 6 lines

Instead of the current:

     master|REBASE 2/6
     master|REBASE 3/7
     master|REBASE 4/8
Quoted 6 lines

To say I'm on 2/6, but that I've tried 3 times already and failed to advance past it.

Anyway, I don't have patches for this side-report/rant. Looking at the implementation it seemed more non-trivial than I was willing to quickly fix.

Yeah, I think the implementation would need a bit of thought.

In summary I agree that it would be good to improve the count so it makes more sense to the user

Best Wishes

Phillip

Quoted 19 lines

We bump the count fairly early before we even get to there being an "exec" item, to implement this proposed view of the world we'd need to defer that (or go back and edit it once we see "failed exec" and that we're using --reschedule-failed-exec).

I'm not familiar enough with the sequencer internals to know if trying that would lead us down a path of e.g. having inconsistent or bad state if we'd die in the middle of all of that.

Ævar Arnfjörð Bjarmason (3):
rebase tests: camel-case rebase.rescheduleFailedExec consistently rebase tests: use test_unconfig after test_config
rebase: don't override --no-reschedule-failed-exec with config

  Documentation/git-rebase.txt |  8 ++++++++
  sequencer.c                  |  5 +++++
  t/t3418-rebase-continue.sh   | 30 ++++++++++++++++++++++++++++--
  3 files changed, 41 insertions(+), 2 deletions(-)
#9 lore raw

Re: [PATCH 2/3] rebase tests: use test_unconfig after test_config

In reply to Ævar Arnfjörð Bjarmason

Hi Ævar

Quoted 16 lines · Ævar Arnfjörð Bjarmason

Fix a test added in 906b63942ac (rebase --am: ignore rebase.rescheduleFailedExec, 2019-07-01) to reset its config after it runs. This doesn't matter now since it's the last test in the file, but will in a subsequent commit where I'll add new tests after this one.

---
  t/t3418-rebase-continue.sh | 1 +
  1 file changed, 1 insertion(+)
t/t3418-rebase-continue.sh +0 -0
index fe407e63cf1..ea14ef496cb 100755

t/t3418-rebase-continue.sh test_expect_success '--reschedule-failed-exec' ' · line 286

Quoted 4 lines
t/t3418-rebase-continue.sh +1 -0
@@ -283,6 +283,7 @@ test_expect_success '--reschedule-failed-exec' '
  '
 
  test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '
+	test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&

I think test_config adds this test_when_finished line itself. See test-lib-functions.sh:

# Set git config, automatically unsetting it after the test is over.

test_config () {
	config_dir=
	if test "$1" = -C
	then
		shift
		config_dir=$1
		shift
	fi
	test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$1'" &&
	git ${config_dir:+-C "$config_dir"} config "$@"
}

Best Wishes

Phillip

Best Wishes

Phillip

Quoted 4 lines
t/t3418-rebase-continue.sh +0 -0
  	test_config rebase.rescheduleFailedExec true &&
  	test_must_fail git rebase -x false HEAD^ &&
  	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
#17 lore raw

PATCH v2 1/2 rebase tests: camel-case rebase.rescheduleFailedExec consistently

In reply to Ævar Arnfjörð Bjarmason

Fix a test added in 906b63942ac (rebase --am: ignore rebase.rescheduleFailedExec, 2019-07-01) to camel-case the configuration variable. This doesn't change the behavior of the test, it's merely to help its human readers.

---
 t/t3418-rebase-continue.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
t/t3418-rebase-continue.sh +2 -2
index 0838f4e798..fe407e63cf 100755
@@ -282,8 +282,8 @@ test_expect_success '--reschedule-failed-exec' '
 	test_i18ngrep "has been rescheduled" err
 '
 
-test_expect_success 'rebase.reschedulefailedexec only affects `rebase -i`' '
-	test_config rebase.reschedulefailedexec true &&
+test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '
+	test_config rebase.rescheduleFailedExec true &&
 	test_must_fail git rebase -x false HEAD^ &&
 	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
 	git rebase --abort &&
Signature · 1 line
2.31.1.584.gf4baedee75
#16 lore raw

v2 cover rebase: don't override --no-reschedule-failed-exec with config

In reply to Ævar Arnfjörð Bjarmason

This fixes a bug where we read config & options when "git rebase -i -x make" starts, and will understand the "--no-reschedule-failed-exec" option, but once a command fails we'll lose track of having had a "--no-reschedule-failed-exec" and will happily re-read the "rebase.rescheduleFailedExec=true" config the user might have.

Thus we'll have config winning over explicit command-line options. This series fixes that bug.

Changes since v1:

* I forgot how test_config works, and was doing a test_when_finished "test_unconfig", which isn't needed, duh! Thanks to Phillip Wood for that & other reviews on this series.

* There was a discussion about how to add yet another rebase machinery state file. I think the consensus is "let's just do it like this", i.e. we could write some tri-state content to the file, but we'd get into back-compat issues with other git versions.

There was a discussiob about how to manage this whole state (a DB, JSON?) in another thread, let's kick the can of how exactly we store state down the line, and just fix the bug using the existing state saving convention for now.

Ævar Arnfjörð Bjarmason (2):
rebase tests: camel-case rebase.rescheduleFailedExec consistently
rebase: don't override --no-reschedule-failed-exec with config

 Documentation/git-rebase.txt |  8 ++++++++
 sequencer.c                  |  5 +++++
 t/t3418-rebase-continue.sh   | 27 +++++++++++++++++++++++++--
 3 files changed, 38 insertions(+), 2 deletions(-)

Range-diff against v1:

1:  002dc72ee7 = 1:  e0dd2cb82a rebase tests: camel-case rebase.rescheduleFailedExec consistently
2:  330b33e7a8 < -:  ---------- rebase tests: use test_unconfig after test_config
3:  e00300d58d ! 2:  7991160de3 rebase: don't override --no-reschedule-failed-exec with config

@@ Commit message
However the --reschedule-failed-exec option doesn't take effect when the rebase starts (we'd just create a ".git/rebase-merge/reschedule-failed-exec" file if it was true). It
- only takes effect when the exec command fails, and the user wants to
- run "rebase --continue".
+ only takes effect when the exec command fails, at which point we'll
+ reschedule the failed "exec" command.

- At that point we'll have forgotten that we asked for
- --no-reschedule-failed-exec at the start, and will happily re-read the
- config.
+ Since we only wrote out the positive
+ ".git/rebase-merge/reschedule-failed-exec" under
+ --reschedule-failed-exec, but nothing with --no-reschedule-failed-exec
+ we'll forget that we asked not to reschedule failed "exec", and would
+ happily re-read the config and see that
+ rebase.rescheduleFailedExec=true is set.

- We'll then see that rebase.rescheduleFailedExec=true is set. At that
- point we have no record of having set --no-reschedule-failed-exec
- earlier. So the config will effectively override the user having
- explicitly disabled the option on the command-line.
+ So the config will effectively override the user having explicitly
+ disabled the option on the command-line.

Even more confusingly: Since rebase accepts different options based on its state there wasn't even a way to get around this with "rebase
@@ t/t3418-rebase-continue.sh: test_expect_success 'rebase.rescheduleFailedExec onl

+test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '
+ test_when_finished "git rebase --abort" &&
-+ test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+ test_config rebase.rescheduleFailedExec true &&
+ test_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&
+ test_must_fail git rebase --continue 2>err &&
@@ t/t3418-rebase-continue.sh: test_expect_success 'rebase.rescheduleFailedExec onl
+test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '
+ test_when_finished "git rebase --abort" &&
+ test_must_fail git rebase -x false HEAD~2 &&
-+ test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+ test_config rebase.rescheduleFailedExec true &&
+ test_must_fail git rebase --continue 2>err &&
+ ! grep "has been rescheduled" err

Signature · 1 line
2.31.1.584.gf4baedee75
#18 lore raw

PATCH v2 2/2 rebase: don't override --no-reschedule-failed-exec with config

Fix a bug in how --no-reschedule-failed-exec interacts with rebase.rescheduleFailedExec=true being set in the config. Before this change the --no-reschedule-failed-exec config option would be overridden by the config.

This bug happened because of the particulars of how "rebase" works v.s. most other git commands when it comes to parsing options and config:

When we read the config and parse the CLI options we correctly prefer the --no-reschedule-failed-exec option over rebase.rescheduleFailedExec=true in the config. So far so good.

However the --reschedule-failed-exec option doesn't take effect when the rebase starts (we'd just create a ".git/rebase-merge/reschedule-failed-exec" file if it was true). It only takes effect when the exec command fails, at which point we'll reschedule the failed "exec" command.

Since we only wrote out the positive ".git/rebase-merge/reschedule-failed-exec" under --reschedule-failed-exec, but nothing with --no-reschedule-failed-exec we'll forget that we asked not to reschedule failed "exec", and would happily re-read the config and see that rebase.rescheduleFailedExec=true is set.

So the config will effectively override the user having explicitly disabled the option on the command-line.

Even more confusingly: Since rebase accepts different options based on its state there wasn't even a way to get around this with "rebase --continue --no-reschedule-failed-exec" (but you could of course set the config with "rebase -c ...").

I think the least bad way out of this is to declare that for such options and config whatever we decide at the beginning of the rebase goes. So we'll now always create either a "reschedule-failed-exec" or a "no-reschedule-failed-exec file at the start, not just the former if we decided we wanted the feature.

With this new worldview you can no longer change the setting once a rebase has started except by manually removing the state files discussed above. I think making it work like that is the the least confusing thing we can do.

In the future we might want to learn to change the setting in the middle by combining "--edit-todo" with "--[no-]reschedule-failed-exec", we currently don't support combining those options, or any other way to change the state in the middle of the rebase short of manually editing the files in ".git/rebase-merge/*".

The bug being fixed here originally came about because of a combination of the behavior of the code added in d421afa0c66 (rebase:
introduce --reschedule-failed-exec, 2018-12-10) and the addition of the config variable in 969de3ff0e0 (rebase: add a config option to default to --reschedule-failed-exec, 2018-12-10).

---
 Documentation/git-rebase.txt |  8 ++++++++
 sequencer.c                  |  5 +++++
 t/t3418-rebase-continue.sh   | 23 +++++++++++++++++++++++
 3 files changed, 36 insertions(+)
Documentation/git-rebase.txt +8 -0
index f08ae27e2a..93afe5ce3c 100644
@@ -623,6 +623,14 @@ See also INCOMPATIBLE OPTIONS below.
 --no-reschedule-failed-exec::
 	Automatically reschedule `exec` commands that failed. This only makes
 	sense in interactive mode (or when an `--exec` option was provided).
++
+Even though this option applies once a rebase is started, it's set for
+the whole rebase at the start based on either the
+`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]
+or "CONFIGURATION" below) or whether this option is
+provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
+start would be overridden by the presence of
+`rebase.rescheduleFailedExec=true` configuration.
 
 INCOMPATIBLE OPTIONS
 --------------------
sequencer.c +5 -0
index b4c63ae207..8a7e7414eb 100644
@@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
 static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
+static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-reschedule-failed-exec")
 static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
 static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
 
@@ -2854,6 +2855,8 @@ static int read_populate_opts(struct replay_opts *opts)
 
 		if (file_exists(rebase_path_reschedule_failed_exec()))
 			opts->reschedule_failed_exec = 1;
+		else if (file_exists(rebase_path_no_reschedule_failed_exec()))
+			opts->reschedule_failed_exec = 0;
 
 		if (file_exists(rebase_path_drop_redundant_commits()))
 			opts->drop_redundant_commits = 1;
@@ -2954,6 +2957,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 		write_file(rebase_path_ignore_date(), "%s", "");
 	if (opts->reschedule_failed_exec)
 		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
+	else
+		write_file(rebase_path_no_reschedule_failed_exec(), "%s", "");
 
 	return 0;
 }
t/t3418-rebase-continue.sh +23 -0
index fe407e63cf..f4c2ee02bc 100755
@@ -290,4 +290,27 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '
 	git rebase HEAD^
 '
 
+test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '
+	test_when_finished "git rebase --abort" &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&
+	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
+'
+
 test_done
Signature · 1 line
2.31.1.584.gf4baedee75
#19 lore raw

Re: [PATCH v2 0/2] rebase: don't override --no-reschedule-failed-exec with config

In reply to Ævar Arnfjörð Bjarmason

Hi Ævar

Quoted 11 lines · Ævar Arnfjörð Bjarmason

This fixes a bug where we read config & options when "git rebase -i -x make" starts, and will understand the "--no-reschedule-failed-exec" option, but once a command fails we'll lose track of having had a "--no-reschedule-failed-exec" and will happily re-read the "rebase.rescheduleFailedExec=true" config the user might have.

Thus we'll have config winning over explicit command-line options. This series fixes that bug.

Changes since v1:

Quoted 8 lines

* I forgot how test_config works, and was doing a test_when_finished "test_unconfig", which isn't needed, duh! Thanks to Phillip Wood for that & other reviews on this series.

* There was a discussion about how to add yet another rebase machinery state file. I think the consensus is "let's just do it like this", i.e. we could write some tri-state content to the file, but we'd get into back-compat issues with other git versions.

These patches look fine to me now

Thanks

Phillip

Quoted 63 lines

There was a discussiob about how to manage this whole state (a DB, JSON?) in another thread, let's kick the can of how exactly we store state down the line, and just fix the bug using the existing state saving convention for now.

Ævar Arnfjörð Bjarmason (2):
rebase tests: camel-case rebase.rescheduleFailedExec consistently
rebase: don't override --no-reschedule-failed-exec with config

  Documentation/git-rebase.txt |  8 ++++++++
  sequencer.c                  |  5 +++++
  t/t3418-rebase-continue.sh   | 27 +++++++++++++++++++++++++--
  3 files changed, 38 insertions(+), 2 deletions(-)

Range-diff against v1:

1:  002dc72ee7 = 1:  e0dd2cb82a rebase tests: camel-case rebase.rescheduleFailedExec consistently
2:  330b33e7a8 < -:  ---------- rebase tests: use test_unconfig after test_config
3:  e00300d58d ! 2:  7991160de3 rebase: don't override --no-reschedule-failed-exec with config

@@ Commit message
However the --reschedule-failed-exec option doesn't take effect when the rebase starts (we'd just create a ".git/rebase-merge/reschedule-failed-exec" file if it was true). It
- only takes effect when the exec command fails, and the user wants to
- run "rebase --continue".
+ only takes effect when the exec command fails, at which point we'll
+ reschedule the failed "exec" command.

- At that point we'll have forgotten that we asked for
- --no-reschedule-failed-exec at the start, and will happily re-read the
- config.
+ Since we only wrote out the positive
+ ".git/rebase-merge/reschedule-failed-exec" under
+ --reschedule-failed-exec, but nothing with --no-reschedule-failed-exec
+ we'll forget that we asked not to reschedule failed "exec", and would
+ happily re-read the config and see that
+ rebase.rescheduleFailedExec=true is set.

- We'll then see that rebase.rescheduleFailedExec=true is set. At that
- point we have no record of having set --no-reschedule-failed-exec
- earlier. So the config will effectively override the user having
- explicitly disabled the option on the command-line.
+ So the config will effectively override the user having explicitly
+ disabled the option on the command-line.

Even more confusingly: Since rebase accepts different options based on its state there wasn't even a way to get around this with "rebase
@@ t/t3418-rebase-continue.sh: test_expect_success 'rebase.rescheduleFailedExec onl

+test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '
+ test_when_finished "git rebase --abort" &&
-+ test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+ test_config rebase.rescheduleFailedExec true &&
+ test_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&
+ test_must_fail git rebase --continue 2>err &&
@@ t/t3418-rebase-continue.sh: test_expect_success 'rebase.rescheduleFailedExec onl
+test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '
+ test_when_finished "git rebase --abort" &&
+ test_must_fail git rebase -x false HEAD~2 &&
-+ test_when_finished "test_unconfig rebase.rescheduleFailedExec" &&
+ test_config rebase.rescheduleFailedExec true &&
+ test_must_fail git rebase --continue 2>err &&
+ ! grep "has been rescheduled" err