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

[Elijah Newren <newren@gmail.com>]

On Thu, Jul 15, 2021 at 2:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.