From: Phillip Wood Date: Fri, 28 Jun 2019 13:44:07 GMT Subject: Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases Message-ID: In-Reply-To: Hi Junio and Dscho On 28/06/2019 12:49, Johannes Schindelin wrote: > Hi Junio, > > On Thu, 27 Jun 2019, Junio C Hamano wrote: > >> "Johannes Schindelin via GitGitGadget" >> writes: >>> >>> - 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. > >>> 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 , --exec >> ... >> 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. > >>> + 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'. > > 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 > >>> + grep "^exec false" .git/rebase-merge/git-rebase-todo && >>> + git rebase --abort && >>> + git rebase HEAD^ >>> +' >>> + >>> test_done >>