{"thread":{"id":"44286","subject":"Bug with git merge-base and a packed ref","startedAt":"2016-10-12T10:46:36Z","lastAt":"2016-10-13T06:41:58Z","messageCount":6,"participants":["Stepan Kasal","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"303971","messageId":"20161012103716.GA31533@ucw.cz","threadId":"44286","inReplyTo":null,"subject":"Bug with git merge-base and a packed ref","fromName":"Stepan Kasal","fromEmail":"kasal@ucw.cz","sentAt":"2016-10-12T10:37:16Z","receivedAt":"2016-10-12T10:46:36Z","isPatch":false,"sender":{"key":"kasal@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/1481596?v=4"},"body":"Hello,\n\nfirst, I observed a bug with git pull --rebase:\nif the remote branch got rebased and the loval branch was updated,\npull tried to rebase the whole branch, not the local increment.\n\nA reproducer would look like that\n\n# in repo1:\ngit checkout tmp\ncd ..\ngit clone repo1 repo2\ncd repo1\ngit rebase elsewhere tmp\ncd ../repo2\n# edit\ngit commit -a -m 'Another commit'\ngit pull -r\n\nThe last command performs something like\n   git rebase new-origin/tmp\ninstead of\n   git rebase --onto new-origin/tmp old-origin/tmp\n\nI'm using git version 2.10.1.windows.1\n\n\nI tried to debug the issue:\nI found that the bug happens only at the very first pull after clone.\nI was able to reproduce it with git-pull.sh\n\nThe problem seems to be that command\n  git merge-base --fork-point refs/remotes/origin/tmp refs/heads/tmp\nreturns nothing, because the refs are packed.\n\nCould you please fix merge-base so that it understands packed refs?\n\nThanks,\n  Stepan\n"},{"id":"304008","messageId":"20161012163209.oadmm7xsmm7oeumr@sigill.intra.peff.net","threadId":"44286","inReplyTo":"20161012103716.GA31533@ucw.cz","subject":"Re: Bug with git merge-base and a packed ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-12T16:32:09Z","receivedAt":"2016-10-12T16:38:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 12, 2016 at 12:37:16PM +0200, Stepan Kasal wrote:\n\n> A reproducer would look like that\n> \n> # in repo1:\n> git checkout tmp\n> cd ..\n> git clone repo1 repo2\n> cd repo1\n> git rebase elsewhere tmp\n> cd ../repo2\n> # edit\n> git commit -a -m 'Another commit'\n> git pull -r\n> \n> The last command performs something like\n>    git rebase new-origin/tmp\n> instead of\n>    git rebase --onto new-origin/tmp old-origin/tmp\n> \n> I'm using git version 2.10.1.windows.1\n> \n> \n> I tried to debug the issue:\n> I found that the bug happens only at the very first pull after clone.\n> I was able to reproduce it with git-pull.sh\n> \n> The problem seems to be that command\n>   git merge-base --fork-point refs/remotes/origin/tmp refs/heads/tmp\n> returns nothing, because the refs are packed.\n\nThe --fork-point option looks in the reflog to notice that the upstream\nbranch has been rebased. I don't think clone actually writes reflog\nentries, though, which would explain why it happens only on the first\npull after clone.\n\nI suspect the necessary information _is_ there, though. When we update\nthe tracking branch, the new reflog entry will show it going from sha1\nX to sha1 Y. So my guess is that --fork-point is looking for the entry\nwhere it became \"X\" (which doesn't exist, because clone did not write\nit), but it _could_ find that we came from \"X\" in the very first reflog\nentry.\n\nThat's all without looking at the code, though. I don't have time to\nexamine it now, but maybe that can point somebody in the right\ndirection.\n\n> Could you please fix merge-base so that it understands packed refs?\n\nI think the packed-refs thing is probably a red herring. If merge-base\ndidn't understand packed refs, a huge chunk of git would be horribly\nbroken.\n\n-Peff\n"},{"id":"304022","messageId":"20161012193336.GA23072@ucw.cz","threadId":"44286","inReplyTo":"20161012163209.oadmm7xsmm7oeumr@sigill.intra.peff.net","subject":"Re: Bug with git merge-base and a packed ref","fromName":"Stepan Kasal","fromEmail":"kasal@ucw.cz","sentAt":"2016-10-12T19:33:36Z","receivedAt":"2016-10-12T19:35:35Z","isPatch":false,"sender":{"key":"kasal@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/1481596?v=4"},"body":"Hello,\n\nOn Wed, Oct 12, 2016 at 12:32:09PM -0400, Jeff King wrote:\n> The --fork-point option looks in the reflog [...]\n> On Wed, Oct 12, 2016 at 12:37:16PM +0200, Stepan Kasal wrote:\n> > Could you please fix merge-base so that it understands packed refs?\n\nI bet you nailed it; nothing with packed refs.\nThanks for correcting me.\n\nStepan\n"},{"id":"304025","messageId":"20161012201040.pyrp6bktz3fgmqzn@sigill.intra.peff.net","threadId":"44286","inReplyTo":"20161012163209.oadmm7xsmm7oeumr@sigill.intra.peff.net","subject":"[PATCH] merge-base: handle --fork-point without reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-12T20:10:40Z","receivedAt":"2016-10-12T20:17:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 12, 2016 at 12:32:09PM -0400, Jeff King wrote:\n\n> > The problem seems to be that command\n> >   git merge-base --fork-point refs/remotes/origin/tmp refs/heads/tmp\n> > returns nothing, because the refs are packed.\n> \n> The --fork-point option looks in the reflog to notice that the upstream\n> branch has been rebased. I don't think clone actually writes reflog\n> entries, though, which would explain why it happens only on the first\n> pull after clone.\n> \n> I suspect the necessary information _is_ there, though. When we update\n> the tracking branch, the new reflog entry will show it going from sha1\n> X to sha1 Y. So my guess is that --fork-point is looking for the entry\n> where it became \"X\" (which doesn't exist, because clone did not write\n> it), but it _could_ find that we came from \"X\" in the very first reflog\n> entry.\n\nActually, --fork-point gets this case right; it will put the \"old\" sha1\nfor the initial reflog entry into the list of base tips. But this\nmerge-base actually runs before we fetch, so there literally is no\nreflog when it runs. And it doesn't get that case right.\n\nHere's a fix. The test I added checks things more directly, but I\nconfirmed manually that it also fixes the rebase case that you reported.\n\n-- >8 --\nSubject: merge-base: handle --fork-point without reflog\n\nThe --fork-point option looks in the reflog to try to find\nwhere a derived branch forked from a base branch. However,\nif the reflog for the base branch is totally empty (as it\ncommonly is right after cloning, which does not write a\nreflog entry), then our for_each_reflog call will not find\nany entries, and we will come up with no merge base, even\nthough there may be one with the current tip of the base.\n\nWe can fix this by just adding the current tip to\nour list of collected entries.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIt would actually be correct to just unconditionally add the ref tip, as\nadd_one_commit already drops duplicates. But it would only be necessary\nin other cases if you have a broken reflog which is missing the entry\nthat moved us to the current tip.\n\n builtin/merge-base.c  | 3 +++\n t/t6010-merge-base.sh | 6 ++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex c0d1822..b572a37 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -173,6 +173,9 @@ static int handle_fork_point(int argc, const char **argv)\n \trevs.initial = 1;\n \tfor_each_reflog_ent(refname, collect_one_reflog_ent, &revs);\n \n+\tif (!revs.nr && !get_sha1(refname, sha1))\n+\t\tadd_one_commit(sha1, &revs);\n+\n \tfor (i = 0; i < revs.nr; i++)\n \t\trevs.commit[i]->object.flags &= ~TMP_MARK;\n \ndiff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh\nindex e0c5f44..31db7b5 100755\n--- a/t/t6010-merge-base.sh\n+++ b/t/t6010-merge-base.sh\n@@ -260,6 +260,12 @@ test_expect_success 'using reflog to find the fork point' '\n \ttest_cmp expect3 actual\n '\n \n+test_expect_success '--fork-point works with empty reflog' '\n+\tgit -c core.logallrefupdates=false branch no-reflog base &&\n+\tgit merge-base --fork-point no-reflog derived &&\n+\ttest_cmp expect3 actual\n+'\n+\n test_expect_success 'merge-base --octopus --all for complex tree' '\n \t# Best common ancestor for JE, JAA and JDD is JC\n \t#             JE\n-- \n2.10.1.587.g4098016\n\n"},{"id":"304031","messageId":"xmqqd1j52r3m.fsf@gitster.mtv.corp.google.com","threadId":"44286","inReplyTo":"20161012201040.pyrp6bktz3fgmqzn@sigill.intra.peff.net","subject":"Re: [PATCH] merge-base: handle --fork-point without reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T21:29:49Z","receivedAt":"2016-10-12T21:29:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: merge-base: handle --fork-point without reflog\n>\n> The --fork-point option looks in the reflog to try to find\n> where a derived branch forked from a base branch. However,\n> if the reflog for the base branch is totally empty (as it\n> commonly is right after cloning, which does not write a\n> reflog entry), then our for_each_reflog call will not find\n> any entries, and we will come up with no merge base, even\n> though there may be one with the current tip of the base.\n>\n> We can fix this by just adding the current tip to\n> our list of collected entries.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> It would actually be correct to just unconditionally add the ref tip, as\n> add_one_commit already drops duplicates. But it would only be necessary\n> in other cases if you have a broken reflog which is missing the entry\n> that moved us to the current tip.\n\nMakes sense.  And doing conditionally is not much more work ;-)\n\nThanks.\n\n>\n>  builtin/merge-base.c  | 3 +++\n>  t/t6010-merge-base.sh | 6 ++++++\n>  2 files changed, 9 insertions(+)\n>\n> diff --git a/builtin/merge-base.c b/builtin/merge-base.c\n> index c0d1822..b572a37 100644\n> --- a/builtin/merge-base.c\n> +++ b/builtin/merge-base.c\n> @@ -173,6 +173,9 @@ static int handle_fork_point(int argc, const char **argv)\n>  \trevs.initial = 1;\n>  \tfor_each_reflog_ent(refname, collect_one_reflog_ent, &revs);\n>  \n> +\tif (!revs.nr && !get_sha1(refname, sha1))\n> +\t\tadd_one_commit(sha1, &revs);\n> +\n>  \tfor (i = 0; i < revs.nr; i++)\n>  \t\trevs.commit[i]->object.flags &= ~TMP_MARK;\n>  \n> diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh\n> index e0c5f44..31db7b5 100755\n> --- a/t/t6010-merge-base.sh\n> +++ b/t/t6010-merge-base.sh\n> @@ -260,6 +260,12 @@ test_expect_success 'using reflog to find the fork point' '\n>  \ttest_cmp expect3 actual\n>  '\n>  \n> +test_expect_success '--fork-point works with empty reflog' '\n> +\tgit -c core.logallrefupdates=false branch no-reflog base &&\n> +\tgit merge-base --fork-point no-reflog derived &&\n> +\ttest_cmp expect3 actual\n> +'\n> +\n>  test_expect_success 'merge-base --octopus --all for complex tree' '\n>  \t# Best common ancestor for JE, JAA and JDD is JC\n>  \t#             JE\n"},{"id":"304063","messageId":"20161013063418.GA2217@ucw.cz","threadId":"44286","inReplyTo":"20161012201040.pyrp6bktz3fgmqzn@sigill.intra.peff.net","subject":"Re: [PATCH] merge-base: handle --fork-point without reflog","fromName":"Stepan Kasal","fromEmail":"kasal@ucw.cz","sentAt":"2016-10-13T06:34:18Z","receivedAt":"2016-10-13T06:41:58Z","isPatch":true,"sender":{"key":"kasal@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/1481596?v=4"},"body":"Hello,\n\nthank you for this nice and quick fix of this corner case!\n\nStepan\n"}]}