Re: [PATCH] rebase: mark --update-refs as requiring the merge backend

[Elijah Newren <newren@gmail.com>]

On Fri, Jan 20, 2023 at 7:27 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Derrick Stolee <derrickstolee@github.com> writes:
>
> >> +    if (options.update_refs)
> >> +            imply_merge(&options, "--update-refs");
> >> +
> >
> > This solution is very elegant. The only downside is the lack of warning
> > if --update-refs was implied by rebase.updateRefs=true, but I'm happy to
> > delay implementing that warning in favor of your complete solution here.
>
> If features A and B are incompatible and both can be specified from
> both command line and configuration, ideally I would expect the
> system to operate in one of two ways.

I agree that one of the two ways you highlight below would be ideal.
Should this series be held up on extending the checks to implement
this ideal, or are you just commenting for whoever later circles back
to implement this?

>  I haven't thought it through
> to decide which one I prefer between the two.
>
>  * Take "command line trumps configuration" one step further, so
>    that A that is configured but not asked for from the command
>    line is defeated by B that is asked for from the command line.
>
>    This way, only when A and B are both requested via the
>    configuration, of via the command line, we'd fail the operation
>    by saying A and B are incompatible.  Otherwise, the one that is
>    configured but overridden is turned off (either silently or with
>    a warning).
>
>  * Declare "command line trumps configuration" is only among the
>    same feature.  Regardless of how features A and B that are
>    incompatible are requested, the command will error out, citing
>    incompatibility.  It would be very nice if the warning mentioned
>    where the requests for features A and B came from (e.g. "You
>    asked for -B from the command line, but you have A configured,
>    and both cannot be active at the same time---disable A from the
>    command line, or do not ask for B")
>
>    When A is configured and B is requested from the command line,
>    the command will error out, and the user must defeat A from the
>    command line before the user can use B, e.g. "git cmd --no-A -B".
>
> A knee-jerk reaction to the situation is that the latter feels
> somewhat safer than the former, but when I imagine the actual end
> user who saw the error message, especially the suggested solution
> "disable A from the command line or do not ask for B from the
> command line", may say "well, I asked for B for this invocation
> explicitly with -B from the command line, and you(Git) should be
> able to make it imply --no-A", which amounts to the same thing as
> the former choice.

If it is clear to the user that A and B preclude each other, then I
agree with this sentiment that the former choice (silently ignoring
the config) would avoid a minor frustration for some users and thus
would be better.  But I don't think that's applicable here.  There is
no reason that --whitespace=fix shouldn't be available from the merge
backend other than that we haven't implemented it yet, and it's likely
feasible to implement --update-refs for the apply backend with enough
effort if we thought it was worth it.  So, if a user sets
rebase.updateRefs=true in their config because they always want
included branches updated, but one time they run `git rebase
--whitespace=fix`, they will likely have a negative experience like
the one that inspired this patch.  Perhaps we're forced to choose
between possible frustration by different end users, but if so, I
think trying to debug and figure out "Wait, I switched to this branch
and started tweaking it but it appears to not have some relevant
changes I'm sure I made to it yesterday.  What happened?" is a much
worse frustration than "I have to manually specify --no-A in this
special case".  So, when it's not at all obvious that A and B preclude
each other, I think we're better off giving the error.