Re: [PATCH] merge-base: handle --fork-point without reflog

[Stepan Kasal]

Hello,

thank you for this nice and quick fix of this corner case!

Stepan

Re: [PATCH] merge-base: handle --fork-point without reflog

[Junio C Hamano]

Jeff King  writes:

> Subject: merge-base: handle --fork-point without reflog
>
> The --fork-point option looks in the reflog to try to find
> where a derived branch forked from a base branch. However,
> if the reflog for the base branch is totally empty (as it
> commonly is right after cloning, which does not write a
> reflog entry), then our for_each_reflog call will not find
> any entries, and we will come up with no merge base, even
> though there may be one with the current tip of the base.
>
> We can fix this by just adding the current tip to
> our list of collected entries.
>
> Signed-off-by: Jeff King 
> ---
> It would actually be correct to just unconditionally add the ref tip, as
> add_one_commit already drops duplicates. But it would only be necessary
> in other cases if you have a broken reflog which is missing the entry
> that moved us to the current tip.

Makes sense.  And doing conditionally is not much more work ;-)

Thanks.

>
>  builtin/merge-base.c  | 3 +++
>  t/t6010-merge-base.sh | 6 ++
>  2 files changed, 9 insertions(+)
>
> diff --git a/builtin/merge-base.c b/builtin/merge-base.c
> index c0d1822..b572a37 100644
> --- a/builtin/merge-base.c
> +++ b/builtin/merge-base.c
> @@ -173,6 +173,9 @@ static int handle_fork_point(int argc, const char **argv)
>   revs.initial = 1;
>   for_each_reflog_ent(refname, collect_one_reflog_ent, );
>  
> + if (!revs.nr && !get_sha1(refname, sha1))
> + add_one_commit(sha1, );
> +
>   for (i = 0; i < revs.nr; i++)
>   revs.commit[i]->object.flags &= ~TMP_MARK;
>  
> diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
> index e0c5f44..31db7b5 100755
> --- a/t/t6010-merge-base.sh
> +++ b/t/t6010-merge-base.sh
> @@ -260,6 +260,12 @@ test_expect_success 'using reflog to find the fork 
> point' '
>   test_cmp expect3 actual
>  '
>  
> +test_expect_success '--fork-point works with empty reflog' '
> + git -c core.logallrefupdates=false branch no-reflog base &&
> + git merge-base --fork-point no-reflog derived &&
> + test_cmp expect3 actual
> +'
> +
>  test_expect_success 'merge-base --octopus --all for complex tree' '
>   # Best common ancestor for JE, JAA and JDD is JC
>   # JE

[PATCH] merge-base: handle --fork-point without reflog

[Jeff King]

On Wed, Oct 12, 2016 at 12:32:09PM -0400, Jeff King wrote:

> > The problem seems to be that command
> >   git merge-base --fork-point refs/remotes/origin/tmp refs/heads/tmp
> > returns nothing, because the refs are packed.
> 
> The --fork-point option looks in the reflog to notice that the upstream
> branch has been rebased. I don't think clone actually writes reflog
> entries, though, which would explain why it happens only on the first
> pull after clone.
> 
> I suspect the necessary information _is_ there, though. When we update
> the tracking branch, the new reflog entry will show it going from sha1
> X to sha1 Y. So my guess is that --fork-point is looking for the entry
> where it became "X" (which doesn't exist, because clone did not write
> it), but it _could_ find that we came from "X" in the very first reflog
> entry.

Actually, --fork-point gets this case right; it will put the "old" sha1
for the initial reflog entry into the list of base tips. But this
merge-base actually runs before we fetch, so there literally is no
reflog when it runs. And it doesn't get that case right.

Here's a fix. The test I added checks things more directly, but I
confirmed manually that it also fixes the rebase case that you reported.

-- >8 --
Subject: merge-base: handle --fork-point without reflog

The --fork-point option looks in the reflog to try to find
where a derived branch forked from a base branch. However,
if the reflog for the base branch is totally empty (as it
commonly is right after cloning, which does not write a
reflog entry), then our for_each_reflog call will not find
any entries, and we will come up with no merge base, even
though there may be one with the current tip of the base.

We can fix this by just adding the current tip to
our list of collected entries.

Signed-off-by: Jeff King 
---
It would actually be correct to just unconditionally add the ref tip, as
add_one_commit already drops duplicates. But it would only be necessary
in other cases if you have a broken reflog which is missing the entry
that moved us to the current tip.

 builtin/merge-base.c  | 3 +++
 t/t6010-merge-base.sh | 6 ++
 2 files changed, 9 insertions(+)

diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index c0d1822..b572a37 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -173,6 +173,9 @@ static int handle_fork_point(int argc, const char **argv)
revs.initial = 1;
for_each_reflog_ent(refname, collect_one_reflog_ent, );
 
+   if (!revs.nr && !get_sha1(refname, sha1))
+   add_one_commit(sha1, );
+
for (i = 0; i < revs.nr; i++)
revs.commit[i]->object.flags &= ~TMP_MARK;
 
diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index e0c5f44..31db7b5 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh
@@ -260,6 +260,12 @@ test_expect_success 'using reflog to find the fork point' '
test_cmp expect3 actual
 '
 
+test_expect_success '--fork-point works with empty reflog' '
+   git -c core.logallrefupdates=false branch no-reflog base &&
+   git merge-base --fork-point no-reflog derived &&
+   test_cmp expect3 actual
+'
+
 test_expect_success 'merge-base --octopus --all for complex tree' '
# Best common ancestor for JE, JAA and JDD is JC
# JE
-- 
2.10.1.587.g4098016

Re: [PATCH] merge-base

[Daniel Barkalow]

On Wed, 13 Apr 2005, Linus Torvalds wrote:

 I agree. But I did the silly common revision tracking part slightly
 differently and in particular I already made fsck and rev-tree use the 
 same exact code.

I think I only saw a cut-and-paste version, and I didn't want to follow
that pattern.

 Also, I don't see why you did the common parent thing as part of the
 library, since that really does seem to be a very specific to this 
 problem, and neither fsck nor rev-tree really wants it. 

That was just silly; on the other hand, I think I'll eventually want to
have a support-for-merging library file, so that people can write
alternative merging programs which call library functions to pick out
history details. But that obviously shouldn't be the same file that
rev-tree and fsck share, and I'll probably do that by pulling main() out
of the program later.

 Also, the date parsing really is a separate issue from the revision 
 tracking (fsck does not want date parsing, but rev-tool does), so I think 
 you might want to do for date parsing what I just did for the revision.h 
 thing? No point in tying them together.

I think there is some value in having a library file that completely
parses commit-tagged files. Having the date field in struct revision
without the code to parse it in the file that defines the struct seems
poor to me.

 So could I ask you to re-factor it and base it on my current tree? Make 
 the merge-base program have that common parent thing in it, and factor 
 out the common date parsing into parse-date.c or something?

I'm not actually using the date in merge-base, either, so I'll just leave
that alone for now (merge-base is based on generations, not time,
currently).

-Daniel
*This .sig left intentionally blank*

-
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html