Re: [PATCH 2/3] merge-base: return fork-point outside reflog

[Junio C Hamano <gitster@pobox.com>]

Michael J Gruber <git@grubix.eu> writes:

> I did not look up the discussion preceeding 4f21454b55 ("merge-base:
> handle --fork-point without reflog", 2016-10-12), but if "merge-base
> --fork-point" were about a "strict reflog" notion then there was nothing
> to fix back then - no reflog, no merge-base candidates. Period.
>
> I don't mind having two modes, say "--reflog" (strict reflog notion) and
> "--fork-point" (reflog plus DAG), but the current implementation is
> neither, and the current documentation clearly is the latter, which is
> what I'm trying to bring the implementaion in line with. Strict mode
> would need a revert of 4f21454b55 (which adds the tip of ref if the
> reflog is empty) for that mode.

Thanks for pointing out a flaw in the logic in my response.

When the user runs "merge-base --fork-point Branch Derived", the
question she is asking is: "what is the merge-base between the
Derived and a virtual commit across all the commits that we know
were at the tip of the Branch at some point and is the merge-base
commit among one of these historical tips of Branch?"

You are correct to point out that loosening the definition of the
"--fork-point" to lose the "and is the merge-base among the one of
these historical tips?" half of the question may be useful, and we
need to introduce "strict" vs "non-strict" modes in order to allow
such a distinction.  I somehow thought that giving and not giving
the "--fork-point" option would be a sufficient clue to express that
distinction, but that is not the same thing and my reasoning was
flawed.  Even when the exact answer of the more strict version of
the "--fork-point" (i.e. what the current implementation computes)
is not available because of missing reflog entries, the merge-base
computation that takes available reflog entries into account but
that does not insist that the resulting base must be among the known
historical tips (i.e. what your patch 2/3 wants to compute) could be
closer to the fork-point than "merge-base Branch Derived" without
"--fork-point" option would compute, and could be more useful as a
"best --onto commit that is guessed automatically" for the purpose
of "rebase".  I agree that there is a value in what your patch 2/3
wants to do when the current one that is more strict would say
"there is no known fork-point"---we would gain a way to say "... but
this is the best guess based on available data that may be better
than getting no answer." which we lack.

Having said all that, I do not agree with your last sentence in the
paragraph I quoted above.  It is a mere implementation detail to
consult the reflog to find out the set of "historical tips of the
Branch"; the current tip by definition is among the commits in that
set, even when the reflog of Branch is missing.  What 4f21454b55 did
was a reasonable "fix" that is still in line with the definition of
"--fork-point" from that point of view.

Whether we add a "looser" version of "--fork-point" to the system or
not, the more strict version should still use the current tip as one
of the historical tips (i.e. those that we would take from the
reflog if the reflog were not empty) in the more "strict" mode.  The
looser version may also want to do so as well.

Thanks.