Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

From: Elijah Newren <newren@gmail.com>
Date: Jul 16, 2021, 18:39 UTC
Message-ID: <CABPp-BEHNpfm6mJBDZ30wCEraT03p+2-gGZyZcnUV34trAzAzw@mail.gmail.com>
In-Reply-To: <xmqqh7gvs2mu.fsf@gitster.g>

On Thu, Jul 15, 2021 at 2:12 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Thu, Jul 15, 2021 at 12:58 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Elijah Newren <newren@gmail.com> writes:
> >>
> >> > Let me ask two questions:
> >> >
> >> > 1. When is it beneficial for users to set both pull.ff and pull.rebase?
> >> > 2. Is it harmful to users for us to allow both to be set when we will
> >> > just ignore one?
> >> >
> >> > I believe the answer to (1) is "never", and the answer to (2) is "yes".
> >>
> >> I agree (1) never gives you anything, even though it does not hurt,
> >> and (2) is "meh".
> >
> > Okay, let's drop this series then.
>
> Not so fast.  I did have problem with some combinations you hinted
> (vaguely---so it is more like "combinations I thought you hinted"),
> but making sure various combinations of options and configuration
> variables work sensibly is a worthy goal to have, I would think.

It may be a worthy goal, but I cannot implement correct behavior if I cannot determine what correct behavior is.

You've only specified how to handle a subset of the valid combinations in each of your emails, and from those individually or even collectively I cannot deduce rules for handling the others. Reading the dozen+ recent messages in the various recent threads, I think I've figured out your opinion in all but two cases, but I have no idea your intent on those two (I would have thought --rebase override there too, but you excluded that), and I'm rather uncertain I've correctly understood you for the other ones (I really hope gmail doesn't whitespace damage the following table):

   pull.ff  pull.rebase  commandline            action
     *          *        --ff-only --rebase     fast-forward only[1]
     *          *        --rebase --no-ff       rebase[1]
     *          *        --rebase --ff          rebase[1]
     *          *        --ff-only --no-rebase  fast-forward only
     *          *        --no-rebase --no-ff    merge --no-ff
     *          *        --no-rebase --ff       merge --ff

    <unset>     *        --no-rebase            merge --ff
    only        *        --no-rebase            merge --ff[2]
    false       *        --no-rebase            merge --no-ff
    true        *        --no-rebase            merge --ff

    <unset>     *        --rebase               rebase
    only        *        --rebase               rebase[2]
    false       *        --rebase               ?[2]
    true        *        --rebase               ?[2]

     *          *        --ff-only              fast-forward only[1]

     *       <unset>     --no-ff                merge --no-ff
     *        false      --no-ff                merge --no-ff
     *       !false      --no-ff                rebase (ignore --no-ff)[2][3]

     *       <unset>     --ff                   merge --ff
     *        false      --ff                   merge --ff
     *       !false      --ff                   rebase (ignore --ff)[2][3]

[1] https://lore.kernel.org/git/xmqq7dhrtrc2.fsf@gitster.g/
    https://lore.kernel.org/git/c62933fb-96b2-99f5-7169-372f486f6e39@aixigo.com/
[2] https://lore.kernel.org/git/xmqqpmvn5ukj.fsf@gitster.g/
[3] https://lore.kernel.org/git/xmqq8s2b489p.fsf@gitster.g/

It appears you, Phillip, and I all had different opinions about correct behavior and in a few cases though the documentation clearly implied what we thought. So, I'd have to say the documentation is rather unclear as well. However, even if the above table is filled out, it may be complicated enough that I'm at a bit of a loss about how to update the documentation to explain it short of including the table in the documentation.