Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 28, 2019, 13:44 UTC
Message-ID: <e332eb5f-0818-763c-9de0-889644446a35@gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1906281342280.44@tvgsbejvaqbjf.bet>

Hi Junio and Dscho

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

Show 45 quoted lines

> Hi Junio,
> 
> On Thu, 27 Jun 2019, Junio C Hamano wrote:
> 
>> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>> 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 <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

Show 55 quoted lines

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