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

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

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

> In fact, per documentation "--fork-point" looks at the reflog in
> addition to doing the usual walk from the tip. The original design
> description in d96855ff51 ("merge-base: teach "--fork-point" mode",
> 2013-10-23) describes this as computing from a virtual merge-base of all
> the historical tips of refname. They may or may not all be present in
> the reflog (think pruning, non-ff fetching, fast forwarding etc.),
> so filtering by the current contents of the reflog is potentially
> harmful, and it does not seem to fulfill any purpose in the original
> design.

Let me think aloud, using the picture from the log message from that
commit.

                         o---B1
                        /
        ---o---o---B2--o---o---o---Base
                \
                 B3
                  \
                   Derived
    
    where the current tip of the "base" branch is at Base, but earlier
    fetch observed that its tip used to be B3 and then B2 and then B1
    before getting to the current commit, and the branch being rebased
    on top of the latest "base" is based on commit B3.

So the logic tries to find a merge base between "Derived" and a
virtual merge commit across Base, B1, B2 and B3.  And it finds B3.

If for some reason we didn't have B3 in the reflog, then wouldn't
the merge base computation between Derived and a virtual merge
commit across Base, B2 and B2 (but not B3 because we no longer know
about it due to its lack in the reflog) find 'o' that is the parent
of B2 and B3?  Wouldn't that lead to both B3 and Derived replayed
when the user of the fork-point potion rebases the history of
Derived?

Perhaps that is the best we could do with a pruned reflog that lacks
B3, but if that is the case, I wonder if it may be better to fail
the request saying that we cannot find the fork-point (because,
well, your reflog no longer has sufficient information), than
silently give a wrong result, and in this case, we can tell between
a correct result (i.e. the merge base is one of the commits we still
know was at the tip) and a wrong one (i.e. the merge base is not any
of the commits in the reflog).

If we declare --fork-point is the best effort option and may give an
answer that is not better than without the option, then I think this
patch is OK, but that diminishes the value of the option as a
building block, I am afraid.

Callers that are more careful could ask merge-base with --fork-point
(and happily use it knowing that the result is indeed a commit that
used to be at the tip), or fall back to the result merge-base
without --fork-point gives (because you could do no better) and deal
with duplicates that may happen due to the imprecise determination.
With this change, these callers will get result from a call to
"merge-base --fork-point" that may or may not be definite, and they
cannot tell.  For lazy users, making the option itself to fall back
may be simpler to use, and certainly is a valid stance to take when
implementing a convenience option to a Porcelain command, but I do
not know if it is a good idea to throw "merge-base --fork-point"
into that category.



>
> Remove the filtering and add a test for an out-of-reflog merge base.
>
> Reported-by: Ekelhart Jakob <jakob.ekelhart@fsw.at>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
>  builtin/merge-base.c  | 18 +++---------------
>  t/t6010-merge-base.sh |  8 ++++++++
>  2 files changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/merge-base.c b/builtin/merge-base.c
> index 6dbd167d3b..926a7615ea 100644
> --- a/builtin/merge-base.c
> +++ b/builtin/merge-base.c
> @@ -186,23 +186,11 @@ static int handle_fork_point(int argc, const char **argv)
>  	 * There should be one and only one merge base, when we found
>  	 * a common ancestor among reflog entries.
>  	 */
> -	if (!bases || bases->next) {
> +	if (!bases || bases->next)
>  		ret = 1;
> -		goto cleanup_return;
> -	}
> -
> -	/* And the found one must be one of the reflog entries */
> -	for (i = 0; i < revs.nr; i++)
> -		if (&bases->item->object == &revs.commit[i]->object)
> -			break; /* found */
> -	if (revs.nr <= i) {
> -		ret = 1; /* not found */
> -		goto cleanup_return;
> -	}
> -
> -	printf("%s\n", oid_to_hex(&bases->item->object.oid));
> +	else
> +		printf("%s\n", oid_to_hex(&bases->item->object.oid));
>  
> -cleanup_return:
>  	free_commit_list(bases);
>  	return ret;
>  }
> diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
> index 17fffd7998..850463d4f2 100755
> --- a/t/t6010-merge-base.sh
> +++ b/t/t6010-merge-base.sh
> @@ -267,6 +267,14 @@ test_expect_success '--fork-point works with empty reflog' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success '--fork-point works with merge-base outside reflog' '
> +	git -c core.logallrefupdates=false checkout no-reflog &&
> +	git -c core.logallrefupdates=false commit --allow-empty -m "Commit outside reflogs" &&
> +	git rev-parse base >expect &&
> +	git merge-base --fork-point no-reflog derived >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'merge-base --octopus --all for complex tree' '
>  	# Best common ancestor for JE, JAA and JDD is JC
>  	#             JE