Make rebase.reschedulefailedexec less overzealous

From: Johannes Schindelin <[email protected]>

It does not make sense to stop non-interactive rebases when that config setting is set to true.

builtin/rebase.c +1 -1

@@ -929,7 +930,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		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,
+			 &reschedule_failed_exec,
 			 N_("automatically re-schedule any `exec` that fails")),
 		OPT_END(),
 	};

t/t3418-rebase-continue.sh +5 -0

+	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
+	git rebase --abort &&
+	git rebase HEAD^
+'
+
 test_done

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 4 lines · writes:

From: Johannes Schindelin <[email protected]>

It does not make sense to stop non-interactive rebases when that config setting is set to true.

The reader is assumed to know that that config setting is only about interactive rebases, including "rebase -x", which probably is an OK explanation.

The subject needs a bit more work, though.

"rebase: ignore rebase.reschedulefailedexec unless interative"

or something like that, perhaps.

Quoted 10 lines

builtin/rebase.c +1 -1

@@ -929,7 +930,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		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,
+			 &reschedule_failed_exec,
 			 N_("automatically re-schedule any `exec` that fails")),
 		OPT_END(),
 	};

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

Quoted 5 lines

builtin/rebase.c +1 -1

@@ -1227,8 +1228,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	if (options.reschedule_failed_exec && !is_interactive(&options))
+	if (reschedule_failed_exec > 0 && !is_interactive(&options))

OK, it used to be that we got affected by what came from "options", which was read from the configuration. Now we only pay attention to the command line, which makes sense.

At this point, we have already examined '-x' and called imply_interative(), so this should trigger for '-x' (without '-i'), right?

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

Quoted 1 line

builtin/rebase.c +0 -0

 		die(_("--reschedule-failed-exec requires an interactive rebase"));

I wonder if users understand that '-x' is "an interctive rebase". The documentation can read both ways, and one of these may want to be clarified.

	-x <cmd>, --exec <cmd>
	...
	This uses the --interactive machinery internally, but it can
	be run without an explicit --interactive.

Is it saying that use of interactive machinery is an impelementation detail the users should not concern themselves (in which case, the message given to "die()" above is misleading---not a new problem with this patch, though)? Is it saying "-x" makes it plenty clear that the user wants interactive behaviour, so the users do not need to spell out --interactive in order to ask for it (in which case, "die()" message is fine, but "... internally, but ..." is misleading)?

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

Quoted 2 lines

builtin/rebase.c +2 -0

+	if (reschedule_failed_exec >= 0)
+		options.reschedule_failed_exec = reschedule_failed_exec;

OK, here we recover the bit that is only stored in a local variable and pass it into cmd_rebase__interactive() machinery via the options structure, which lets the codepath after this point oblivious to this change, which is good ;-).

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

Quoted 13 lines

builtin/rebase.c +0 -0

 	if (options.git_am_opts.argc) {
 		/* all am options except -q are compatible only with --am */

t/t3418-rebase-continue.sh +3 -0

index bdaa511bb0..4eff14dae5 100755
@@ -265,4 +265,12 @@ 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_must_fail git rebase -x false HEAD^ &&

These three lines gives us a concise summary of this patch ;-)

- The test title can serve as a starting point for a much better patch title.

- We trigger for '-x' without requiring '-i'.

Quoted 6 lines

t/t3418-rebase-continue.sh +5 -0

+	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
+	git rebase --abort &&
+	git rebase HEAD^
+'
+
 test_done

Hi Junio,

Quoted 10 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>

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

Quoted 3 lines · writes:

builtin/rebase.c +1 -1

-	if (options.reschedule_failed_exec && !is_interactive(&options))
+	if (reschedule_failed_exec > 0 && !is_interactive(&options))

OK, it used to be that we got affected by what came from "options", which was read from the configuration. Now we only pay attention to the command line, which makes sense.

Quoted 3 lines

At this point, we have already examined '-x' and called imply_interative(), so this should trigger for '-x' (without '-i'), right?

Yes, at this point we have done all the parsing and automatic implying, and check for incompatible options.

Quoted 11 lines

Quoted 1 line

  		die(_("--reschedule-failed-exec requires an interactive rebase"));

I wonder if users understand that '-x' is "an interctive rebase". The documentation can read both ways, and one of these may want to be clarified.

	-x <cmd>, --exec <cmd>
	...
	This uses the --interactive machinery internally, but it can
	be run without an explicit --interactive.

Quoted 8 lines

Is it saying that use of interactive machinery is an impelementation detail the users should not concern themselves (in which case, the message given to "die()" above is misleading---not a new problem with this patch, though)? Is it saying "-x" makes it plenty clear that the user wants interactive behaviour, so the users do not need to spell out --interactive in order to ask for it (in which case, "die()" message is fine, but "... internally, but ..." is misleading)?

Hmm. What would you think about:

   		die(_("--reschedule-failed-exec requires --exec or --interactive"));

It is still not _complete_, but at least it should be a ton less confusing.

Quoted 24 lines

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

Quoted 2 lines

builtin/rebase.c +2 -0

+	if (reschedule_failed_exec >= 0)
+		options.reschedule_failed_exec = reschedule_failed_exec;

OK, here we recover the bit that is only stored in a local variable and pass it into cmd_rebase__interactive() machinery via the options structure, which lets the codepath after this point oblivious to this change, which is good ;-).

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

Quoted 13 lines

if (options.git_am_opts.argc) {
/* all am options except -q are compatible only with --am */

t/t3418-rebase-continue.sh +3 -0

index bdaa511bb0..4eff14dae5 100755
@@ -265,4 +265,12 @@ 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_must_fail git rebase -x false HEAD^ &&

These three lines gives us a concise summary of this patch ;-)

Quoted 4 lines

- The test title can serve as a starting point for a much better patch title.

- We trigger for '-x' without requiring '-i'.

I changed the oneline to

	rebase --am: ignore rebase.reschedulefailedexec

This gives credit to the implementation details, as appropriate for commit messages, and the error message still tries to be as helpful as possible for users (who do not necessarily need to know that there are two backends).

At this point, I am _really_ glad that we only have two backends left (for all practical purposes, I don't count --preserve-merges).

Ciao,
Dscho

Quoted 7 lines

Quoted 6 lines

t/t3418-rebase-continue.sh +5 -0

+	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
+	git rebase --abort &&
+	git rebase HEAD^
+'
+
  test_done

Quoted 18 lines

Quoted 17 lines

I wonder if users understand that '-x' is "an interctive rebase". The documentation can read both ways, and one of these may want to be clarified.

	-x <cmd>, --exec <cmd>
	...
	This uses the --interactive machinery internally, but it can
	be run without an explicit --interactive.

Is it saying that use of interactive machinery is an impelementation detail the users should not concern themselves (in which case, the message given to "die()" above is misleading---not a new problem with this patch, though)? Is it saying "-x" makes it plenty clear that the user wants interactive behaviour, so the users do not need to spell out --interactive in order to ask for it (in which case, "die()" message is fine, but "... internally, but ..." is misleading)?

Quoted 3 lines

Hmm. What would you think about:

   		die(_("--reschedule-failed-exec requires --exec or --interactive"));

I was wondering about requiring --exec with --reschedule-failed-exec rather than checking is_interactive() as that would be easier to understand.

One potential problem is if someone has an alias that always sets --reschedule-failed-exec but does not always add --exec to the command line.

Quoted 18 lines

Quoted 17 lines

I wonder if users understand that '-x' is "an interctive rebase". The documentation can read both ways, and one of these may want to be clarified.

	-x <cmd>, --exec <cmd>
	...
	This uses the --interactive machinery internally, but it can
	be run without an explicit --interactive.

Is it saying that use of interactive machinery is an impelementation detail the users should not concern themselves (in which case, the message given to "die()" above is misleading---not a new problem with this patch, though)? Is it saying "-x" makes it plenty clear that the user wants interactive behaviour, so the users do not need to spell out --interactive in order to ask for it (in which case, "die()" message is fine, but "... internally, but ..." is misleading)?

Quoted 3 lines

Hmm. What would you think about:

  		die(_("--reschedule-failed-exec requires --exec or --interactive"));

I was wondering about requiring --exec with --reschedule-failed-exec rather than checking is_interactive() as that would be easier to understand.

One potential problem is if someone has an alias that always sets --reschedule-failed-exec but does not always add --exec to the command line.

Quoted 18 lines · Phillip Wood

Quoted 18 lines

Quoted 17 lines

I wonder if users understand that '-x' is "an interctive rebase". The documentation can read both ways, and one of these may want to be clarified.

	-x <cmd>, --exec <cmd>
	...
	This uses the --interactive machinery internally, but it can
	be run without an explicit --interactive.

Is it saying that use of interactive machinery is an impelementation detail the users should not concern themselves (in which case, the message given to "die()" above is misleading---not a new problem with this patch, though)? Is it saying "-x" makes it plenty clear that the user wants interactive behaviour, so the users do not need to spell out --interactive in order to ask for it (in which case, "die()" message is fine, but "... internally, but ..." is misleading)?

Quoted 3 lines

Quoted 3 lines

Hmm. What would you think about:

    		die(_("--reschedule-failed-exec requires --exec or --interactive"));

I was leaning towards admitting that the use of the interactive machinery in "-x" is not merely an implementation detail and fixing the documentation, leaving the die() message in the patch as-is.

But ...

Quoted 3 lines

I was wondering about requiring --exec with --reschedule-failed-exec rather than checking is_interactive() as that would be easier to understand.

... I find this a reasonable way to think about the issue. The option only matters when we are doing "--exec". And the usual convenience measure we'd use, i.e. with --reschedule-failed-exec we consider that we are implicitly in --exec mode, would not work because there is no default "command" to be executed.

Quoted 3 lines

One potential problem is if someone has an alias that always sets --reschedule-failed-exec but does not always add --exec to the command line.

Such a use case would be hitting this die() already without this topic, wouldn't it? In which case we can say there is no "someone" with such an alias.

It seems unintuitive that including a command to execute between each rebase merge implies an interactive rebase. It’s no more interactive than a sequence of rebase merges without intervening commands. A command failure interrupts a rebase just as a merge conflict does. If the fact that a rebase can be interrupts mid-sequence, then all rebases are interactive.

Quoted 3 lines · Phillip Wood

I was wondering about requiring --exec with --reschedule-failed-exec rather than checking is_interactive() as that would be easier to understand.

... I find this a reasonable way to think about the issue.

Quoted 6 lines · Junio C Hamano

Quoted 3 lines · Phillip Wood

One potential problem is if someone has an alias that always sets --reschedule-failed-exec but does not always add --exec to the command line.

Quoted 3 lines

Such a use case would be hitting this die() already without this topic, wouldn't it? In which case we can say there is no "someone" with such an alias.

It depends what else the alias includes, if it also includes -i/-k/-r/--signoff then it wont have been dying but will if we start requiring --exec and they don't set that.