"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:
> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
>
> 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.
> @@ -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(),
> };
> @@ -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?
> 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)?
> + 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 ;-).
> if (options.git_am_opts.argc) {
> /* all am options except -q are compatible only with --am */
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index bdaa511bb0..4eff14dae5 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -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'.
> + grep "^exec false" .git/rebase-merge/git-rebase-todo &&
> + git rebase --abort &&
> + git rebase HEAD^
> +'
> +
> test_done
