Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 23, 2020, 14:10 UTC
Message-ID: <5fe34fc13163_198be208bd@natae.notmuch>
In-Reply-To: <xmqqsg7waleg.fsf@gitster.c.googlers.com>

Junio C Hamano wrote:

Show 6 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > It's clear --ff doesn't imply a merge, so we shouldn't act as if it was.
> 
> Do you specifically mean --ff, or do you talk collectively about
> anything that goes in opt_ff in the C code?

I meant --ff, but the rationale can be extended to all of opt_ff.

> The "--ff" option means "we are allowing fast-forward, so please do
> not make new commit object unnecessarily, but it is just we are
> allowing---we are not limiting ourselves to fast-forard; feel free
> to create a merge commit if necessary".

Yes. *If* a rebase is not specified.

> So it does imply that the user prefers to merge and does not want to
> rebase.

We could imply that, but currently it doesn't.

Currently this does not do a merge:

  git config pull.rebase true
  git pull --ff

Show 14 quoted lines

> If you meant what opt_ff can relay, then there are "--no-ff" and
> "--ff-only" to consider:
> 
>  - "--no-ff" says "we do not allow fast-forward; when the other side
>    is pure descendant of ours, create a merge commit to make them
>    the second parent, so that our side of the history stays to be
>    the first-parent chain that merged them as a side topic."  It may
>    not say what should happen when the history does not
>    fast-forward, and it _is_ possible to argue, for the sake of
>    argument, that it asks to rebase if not fast-forward (so that
>    their history becomes the primary and we build on top of them)
>    while asking to merge if fast-forward (so that our history stays
>    the primary and we absorb their work as a side branch), but that
>    is a behavior that does not make much sense.

I agree it doesn't make much sense; if the user wants a rebase in case of non-fast-forward, --no-ff is the only way.

>    It is much easier to reason about if we accept that the user who
>    says "--no-ff" expects a merge to happen, not a rebase.

Yes, but currently that's not the case.

Currently this doesn't do a merge:

  git config pull.rebase true
  git pull --no-ff

We would need to change the semantics.

Show 12 quoted lines

>  - "--ff-only" says "when their history is pure descendant of ours,
>    just fast-forward our branch to match their history, and
>    otherwise fail."  This one does not have to imply either merge or
>    rebase, as both would give us identical result (i.e. merge would
>    fast-forward and rebase would replay *no* work of our own on top
>    of theirs.  Either case, the result is that our branch tip now
>    points at the tip of their history).
> 
>    The topic under discussion is based on the "we do not have to
>    give advice between merge and rebase if the history
>    fast-forwards", and anybody in support of the topic would be in
>    agreement with this case.

Yes.

> In any case, I think what we have in 'seen' already is a good
> stopping point for this cycle.

It's not a bad stopping point.

But the next patches are needed too. Up to the first 6 patches should be uncontroversial.

> We are not erroring out any new case and simply not showing an advice
> in a situation that it would not apply---the question "does --ff imply
> merge?" does not have to be answered in order to evaluate the 5-patch
> series we have.

Not my patches.

The patch you introduced regarding rebase_unspecified does depend on what happens next. If we decide to change the semantics of --ff* and imply a merge, then my patch to add REBASE_DEFAULT is needed, and as you can see in another patch series [1], that basically has to revert your patch.

Cheers.

[1] https://lore.kernel.org/git/20201218211026.1937168-8-felipe.contreras@gmail.com/

-- 
Felipe Contreras