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

From: Vas Sudanagunta <vas@commonkarma.org>
Date: Jun 28, 2019, 23:52 UTC
Message-ID: <5862CF8D-2D4E-40B4-81A1-4BA9B59AC52E@commonkarma.org>
In-Reply-To: <xmqq4l498irq.fsf@gitster-ct.c.googlers.com>

Hi, original reporter of the issue here. Some thoughts from a user perspective, from outside the black box of “the machinery":

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.

The intuitive behavior for “rebase.reschedulefailedexec” is that it has no impact if nothing is scheduled.  It has nothing to do with interactivity.

vas

On Jun 28, 2019, at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 21 quoted 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)?
>> 
>> 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 ...

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

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