[PATCH 0/3] rebase: offer to reschedule failed exec commands automatically

[PATCH 0/3] rebase: offer to reschedule failed exec commands automatically

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

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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 80793bad8d..9dd68f77f6 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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
 --------------------
 
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index a2ab68ed06..a9ab009fdb 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -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()
 	};
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..6d556fc6c8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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 "
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index b97ffdc9dd..5f0f0c5ab8 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -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"
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
index 7e39d22871..a8a44608e0 100644
--- a/git-rebase--common.sh
+++ b/git-rebase--common.sh
@@ -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 () {
diff --git a/sequencer.c b/sequencer.c
index e1a4dd15f1..69bee63e11 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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)) {
diff --git a/sequencer.h b/sequencer.h
index 5071a73563..1f865dae26 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -39,6 +39,7 @@ struct replay_opts {
 	int allow_empty_message;
 	int keep_redundant_commits;
 	int verbose;
+	int reschedule_failed_exec;
 
 	int mainline;
 
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 0210b2ac6f..54b26a9284 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -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
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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(-)

diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f079bf6b7e..331d250e04 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -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.
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6d556fc6c8..06e450b537 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
 }
 
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 5f0f0c5ab8..1b268a5fcc 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -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 () {
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 54b26a9284..bdaa511bb0 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -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
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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(+)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9dd68f77f6..99ca589ffc 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 06e450b537..b707ccf00f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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")),
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 1b268a5fcc..8048891fed 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -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
 		;;
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index bdaa511bb0..25aaacacfc 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -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
 '
 
-- 
gitgitgadget

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

[Johannes Sixt]

Am 10.12.18 um 20:04 schrieb 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.

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

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

[Stefan Beller]

On Mon, Dec 10, 2018 at 2:08 PM Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 10.12.18 um 20:04 schrieb 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.
>
> 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

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

[Elijah Newren]

On Mon, Dec 10, 2018 at 1:18 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 80793bad8d..9dd68f77f6 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -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
>  --------------------
>
> diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
> index a2ab68ed06..a9ab009fdb 100644
> --- a/builtin/rebase--interactive.c
> +++ b/builtin/rebase--interactive.c
> @@ -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()
>         };
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5b3e5baec8..6d556fc6c8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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"));
> +

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?

>         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 "
> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index b97ffdc9dd..5f0f0c5ab8 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -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"

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?)

> diff --git a/git-rebase--common.sh b/git-rebase--common.sh
> index 7e39d22871..a8a44608e0 100644
> --- a/git-rebase--common.sh
> +++ b/git-rebase--common.sh
> @@ -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 () {
> diff --git a/sequencer.c b/sequencer.c
> index e1a4dd15f1..69bee63e11 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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)) {
> diff --git a/sequencer.h b/sequencer.h
> index 5071a73563..1f865dae26 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -39,6 +39,7 @@ struct replay_opts {
>         int allow_empty_message;
>         int keep_redundant_commits;
>         int verbose;
> +       int reschedule_failed_exec;
>
>         int mainline;
>
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 0210b2ac6f..54b26a9284 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -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.

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

[Elijah Newren]

On Mon, Dec 10, 2018 at 3:13 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.
>

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?

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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. <3fb5a7ff-a63a-6fac-1456-4dbc9135d088@gmail.com>

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.

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

[Johannes Schindelin]

Hi Elijah,

On Mon, 10 Dec 2018, Elijah Newren wrote:

> On Mon, Dec 10, 2018 at 1:18 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > @@ -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.

Indeed, that is exactly the reason.

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

> > @@ -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?)

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.

> The rest of the patch looks good to me.

Thanks!
Dscho

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

[Johannes Schindelin]

Hi Elijah,

On Mon, 10 Dec 2018, Elijah Newren wrote:

> On Mon, Dec 10, 2018 at 3:13 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > 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.
> >
> 
> 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 gitgitgadget@gmail.com 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

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

[Johannes Schindelin]

Hi Stefan,

On Mon, 10 Dec 2018, Stefan Beller wrote:

> On Mon, Dec 10, 2018 at 2:08 PM Johannes Sixt <j6t@kdbg.org> wrote:
> >
> > Am 10.12.18 um 20:04 schrieb 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.
> >
> > 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.

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.

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

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.

> 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>]`.)

> 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

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

[Elijah Newren]

Hi Dscho,

On Tue, Dec 11, 2018 at 2:14 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Mon, 10 Dec 2018, Elijah Newren wrote:
>
> > On Mon, Dec 10, 2018 at 1:18 PM Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
> > > @@ -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.
>
> Indeed, that is exactly the reason.
>
> > 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.

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

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

[Stefan Beller]

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

> 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

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

[Æ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!

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(-)

-- 
2.31.0.285.gb40d23e604f

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

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

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3418-rebase-continue.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 0838f4e798a..fe407e63cf1 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -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 &&
-- 
2.31.0.285.gb40d23e604f

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

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

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3418-rebase-continue.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index fe407e63cf1..ea14ef496cb 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -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 &&
-- 
2.31.0.285.gb40d23e604f

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

[Æ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).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-rebase.txt |  8 ++++++++
 sequencer.c                  |  5 +++++
 t/t3418-rebase-continue.sh   | 25 +++++++++++++++++++++++++
 3 files changed, 38 insertions(+)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index a0487b5cc58..b48e6225769 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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
 --------------------
diff --git a/sequencer.c b/sequencer.c
index 848204d3dc3..59735fdff62 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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;
 }
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index ea14ef496cb..9553d969646 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -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
-- 
2.31.0.285.gb40d23e604f

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

Hi Ævar,

On Mon, 22 Mar 2021, Ævar Arnfjörð Bjarmason wrote:

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

> It's conceptually a relatively simple fix,

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

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

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

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

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

[Phillip Wood]

Hi Ævar

On 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote:
> 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".

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.

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

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

> 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).
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   Documentation/git-rebase.txt |  8 ++++++++
>   sequencer.c                  |  5 +++++
>   t/t3418-rebase-continue.sh   | 25 +++++++++++++++++++++++++
>   3 files changed, 38 insertions(+)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index a0487b5cc58..b48e6225769 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -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
>   --------------------
> diff --git a/sequencer.c b/sequencer.c
> index 848204d3dc3..59735fdff62 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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;
>   }
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index ea14ef496cb..9553d969646 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -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
> 

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

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 29 2021, Phillip Wood wrote:

> Hi Ævar
>
> On 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote:
>> 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".
>
> 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/cover.1616411973.git.avarab@gmail.com

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

> Best Wishes
>
> Phillip
>
>> 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).
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   Documentation/git-rebase.txt |  8 ++++++++
>>   sequencer.c                  |  5 +++++
>>   t/t3418-rebase-continue.sh   | 25 +++++++++++++++++++++++++
>>   3 files changed, 38 insertions(+)
>> diff --git a/Documentation/git-rebase.txt
>> b/Documentation/git-rebase.txt
>> index a0487b5cc58..b48e6225769 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -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
>>   --------------------
>> diff --git a/sequencer.c b/sequencer.c
>> index 848204d3dc3..59735fdff62 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -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;
>>   }
>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
>> index ea14ef496cb..9553d969646 100755
>> --- a/t/t3418-rebase-continue.sh
>> +++ b/t/t3418-rebase-continue.sh
>> @@ -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
>> 

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

[Phillip Wood]

On 29/03/2021 17:12, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Mar 29 2021, Phillip Wood wrote:
> 
>> Hi Ævar
>>
>> On 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote:
>>> 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".
>>
>> 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.

> 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

> 1. http://lore.kernel.org/git/cover.1616411973.git.avarab@gmail.com

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

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

> 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.
>> Best Wishes
>>
>> Phillip
>>
>>> 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).
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> ---
>>>    Documentation/git-rebase.txt |  8 ++++++++
>>>    sequencer.c                  |  5 +++++
>>>    t/t3418-rebase-continue.sh   | 25 +++++++++++++++++++++++++
>>>    3 files changed, 38 insertions(+)
>>> diff --git a/Documentation/git-rebase.txt
>>> b/Documentation/git-rebase.txt
>>> index a0487b5cc58..b48e6225769 100644
>>> --- a/Documentation/git-rebase.txt
>>> +++ b/Documentation/git-rebase.txt
>>> @@ -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
>>>    --------------------
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 848204d3dc3..59735fdff62 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -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;
>>>    }
>>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
>>> index ea14ef496cb..9553d969646 100755
>>> --- a/t/t3418-rebase-continue.sh
>>> +++ b/t/t3418-rebase-continue.sh
>>> @@ -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
>>>
> 

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

[Phillip Wood]

Hi Ævar

On 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote:
> 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.

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

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

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)

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

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

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


> 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(-)
> 

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

[Phillip Wood]

Hi Ævar

On 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote:
> 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.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   t/t3418-rebase-continue.sh | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index fe407e63cf1..ea14ef496cb 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -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

>   	test_config rebase.rescheduleFailedExec true &&
>   	test_must_fail git rebase -x false HEAD^ &&
>   	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
> 

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

[Æ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
-- 
2.31.1.584.gf4baedee75

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

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

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3418-rebase-continue.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 0838f4e798..fe407e63cf 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -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 &&
-- 
2.31.1.584.gf4baedee75

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

[Æ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, 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).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-rebase.txt |  8 ++++++++
 sequencer.c                  |  5 +++++
 t/t3418-rebase-continue.sh   | 23 +++++++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f08ae27e2a..93afe5ce3c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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
 --------------------
diff --git a/sequencer.c b/sequencer.c
index b4c63ae207..8a7e7414eb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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;
 }
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index fe407e63cf..f4c2ee02bc 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -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
-- 
2.31.1.584.gf4baedee75

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

[Phillip Wood]

Hi Ævar

On 09/04/2021 09:01, Ævar Arnfjörð Bjarmason wrote:
> 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.

These patches look fine to me now

Thanks

Phillip

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