Hi Junio and Dscho

On 28/06/2019 12:49, Johannes Schindelin wrote:

Hi Junio,

On Thu, 27 Jun 2019, Junio C Hamano wrote:

quoted

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

quoted

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

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

quoted

quoted

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

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. We could just emit a warning along the lines of "ignoring 
--reschedule-failed-exec without --exec". I'm not sure that we really 
need to error out, unless we think that the missing --exec is an 
indication that the user forgot --exec and so would not want the rebase 
to start, in which case just dying on --reschedule-failed-exec without 
--exec would be fine.

Best Wishes

Phillip

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

quoted

quoted

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

quoted

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

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

quoted

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