{"thread":{"id":"46743","subject":"RE: merge-base not working as expected when base is ahead","startedAt":"2017-09-13T15:07:18Z","lastAt":"2017-11-09T02:49:55Z","messageCount":25,"participants":["Ekelhart Jakob","Michael J Gruber","Johannes Schindelin","Jeff King","Junio C Hamano","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"327942","messageId":"e697e5f0da0845ee92ffdb88a4fa48d9@SFSWW2K12EX02.intern.fsw.at","threadId":"46743","inReplyTo":"c76e76a4ef11480da9995b0bec5a70e1@SFSWW2K12EX02.intern.fsw.at","subject":"RE: merge-base not working as expected when base is ahead","fromName":"Ekelhart Jakob","fromEmail":"jakob.ekelhart@fsw.at","sentAt":"2017-09-13T15:07:09Z","receivedAt":"2017-09-13T15:07:18Z","isPatch":false,"sender":{"key":"jakob.ekelhart@fsw.at","avatar":null},"body":"Dear Git,\n\ngit merge-base --fork-point \"master\" not working if master is already newer then my current branch.\nVery oddly it seems to work whenever you had the expected commit checked out previously - what made it very tricky to detect this problem.\n\nExample:\n - Clone \"https://github.com/jekelhart/GitInfoTry\"\n - Switch to branch \"v1.0.0\"\n - git merge-base --fork-point \"master\"\n - or: git merge-base --fork-point \"origin/master\" \n - expected result: fork point \"fbb1db34c6317a6e8b319c1ec261e97ca1672c22\"\n - but result is empty\n\nIn the repo where we created this example tree in the first place the command returned the expected fork point. If you clone it new and fresh it does not return any result anymore.\n\nWorks, however, on branch \"v2.0.0\". Assumption: because \"master\" is older.(?)\nI think it works locally because the command uses the reflog in addition(!), however, it should work without the local reflog as well. (since the history was not modified in any way)\n\nBR, Jakob\n"},{"id":"328035","messageId":"68633b20-9e2b-ae23-2ede-8728283250f0@grubix.eu","threadId":"46743","inReplyTo":"e697e5f0da0845ee92ffdb88a4fa48d9@SFSWW2K12EX02.intern.fsw.at","subject":"Re: merge-base not working as expected when base is ahead","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-09-14T08:09:30Z","receivedAt":"2017-09-14T08:09:40Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Ekelhart Jakob venit, vidit, dixit 13.09.2017 17:07:\n> Dear Git,\n> \n> git merge-base --fork-point \"master\" not working if master is already newer then my current branch.\n> Very oddly it seems to work whenever you had the expected commit checked out previously - what made it very tricky to detect this problem.\n> \n> Example:\n> - Clone \"https://github.com/jekelhart/GitInfoTry\"\n> - Switch to branch \"v1.0.0\"\n> - git merge-base --fork-point \"master\"\n> - or: git merge-base --fork-point \"origin/master\" \n> - expected result: fork point \"fbb1db34c6317a6e8b319c1ec261e97ca1672c22\"\n> - but result is empty\n> \n> In the repo where we created this example tree in the first place the command returned the expected fork point. If you clone it new and fresh it does not return any result anymore.\n> \n> Works, however, on branch \"v2.0.0\". Assumption: because \"master\" is older.(?)\n> I think it works locally because the command uses the reflog in addition(!), however, it should work without the local reflog as well. (since the history was not modified in any way)\n\nThe documentation lies, unfortunately. It claims that in fork-mode, \"git\nmerge-base\" \"also\" looks at the reflog. In fact, the code explicitely\n*discards* any merge bases that it finds but which are not in the\nreflog. (The merge base that you find for v2.0.0 is in the reflog\nbecause it's the checked out HEAD.)\n\nRemoving the corresponding check gives the merge base for v1.0.0 that\nyou expect, and git still passes all tests. But I'll look at the history\nof these lines before I submit a patch.\n\nMichael\n\n"},{"id":"328048","messageId":"cover.1505394278.git.git@grubix.eu","threadId":"46743","inReplyTo":"68633b20-9e2b-ae23-2ede-8728283250f0@grubix.eu","subject":"[PATCH 0/3] merge-base --fork-point fixes","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-09-14T13:15:17Z","receivedAt":"2017-09-14T13:15:38Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"merge-base --fork-point does not quite work as advertised when the\nreflog is empty or partial. This series brings it in line with the\ndocumentation and, hopefully, with the original intent.\n\nMichael J Gruber (3):\n t6010: test actual test output\n merge-base: return fork-point outside reflog\n merge-base: find fork-point outside partial reflog\n\n builtin/merge-base.c | 20 ++++----------------\n t/t6010-merge-base.sh | 21 +++++++++++++++++++--\n 2 files changed, 23 insertions(+), 18 deletions(-)\n\n-- \n2.14.1.712.gda4591c8a2\n\n"},{"id":"328049","messageId":"5513a1415d11517c28158d9b4212d383a233182f.1505394278.git.git@grubix.eu","threadId":"46743","inReplyTo":"68633b20-9e2b-ae23-2ede-8728283250f0@grubix.eu","subject":"[PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-09-14T13:15:19Z","receivedAt":"2017-09-14T13:15:43Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n2016-10-12) fixed the case without reflog, but only partially:\nThe original code checks whether the merge base candidates are from the\nlist that we started with, which was the list of reflog entries before\n4f21454b55 and the list of reflog entries - or the ref itself if empty -\nafter. The test from 4f21454b55 tested in a situation where the merge\nbase candidate equalled the commit at refname, so it was on this (1\nitem) list by accident.\n\nIn fact, per documentation \"--fork-point\" looks at the reflog in\naddition to doing the usual walk from the tip. The original design\ndescription in d96855ff51 (\"merge-base: teach \"--fork-point\" mode\",\n2013-10-23) describes this as computing from a virtual merge-base of all\nthe historical tips of refname. They may or may not all be present in\nthe reflog (think pruning, non-ff fetching, fast forwarding etc.),\nso filtering by the current contents of the reflog is potentially\nharmful, and it does not seem to fulfill any purpose in the original\ndesign.\n\nRemove the filtering and add a test for an out-of-reflog merge base.\n\nReported-by: Ekelhart Jakob \nSigned-off-by: Michael J Gruber \n---\n builtin/merge-base.c | 18 +++---------------\n t/t6010-merge-base.sh | 8 ++++++++\n 2 files changed, 11 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex 6dbd167d3b..926a7615ea 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -186,23 +186,11 @@ static int handle_fork_point(int argc, const char **argv)\n \t * There should be one and only one merge base, when we found\n \t * a common ancestor among reflog entries.\n \t */\n-\tif (!bases || bases->next) {\n+\tif (!bases || bases->next)\n \t\tret = 1;\n-\t\tgoto cleanup_return;\n-\t}\n-\n-\t/* And the found one must be one of the reflog entries */\n-\tfor (i = 0; i < revs.nr; i++)\n-\t\tif (&bases->item->object == &revs.commit[i]->object)\n-\t\t\tbreak; /* found */\n-\tif (revs.nr <= i) {\n-\t\tret = 1; /* not found */\n-\t\tgoto cleanup_return;\n-\t}\n-\n-\tprintf(\"%s\\n\", oid_to_hex(&bases->item->object.oid));\n+\telse\n+\t\tprintf(\"%s\\n\", oid_to_hex(&bases->item->object.oid));\n \n-cleanup_return:\n \tfree_commit_list(bases);\n \treturn ret;\n }\ndiff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh\nindex 17fffd7998..850463d4f2 100755\n--- a/t/t6010-merge-base.sh\n+++ b/t/t6010-merge-base.sh\n@@ -267,6 +267,14 @@ test_expect_success '--fork-point works with empty reflog' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--fork-point works with merge-base outside reflog' '\n+\tgit -c core.logallrefupdates=false checkout no-reflog &&\n+\tgit -c core.logallrefupdates=false commit --allow-empty -m \"Commit outside reflogs\" &&\n+\tgit rev-parse base >expect &&\n+\tgit merge-base --fork-point no-reflog derived >actual &&\n+\ttest_cmp expect 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.14.1.712.gda4591c8a2\n\n"},{"id":"328050","messageId":"f16c671b268432199a0b2e0d22bb1b786a44bb2a.1505394278.git.git@grubix.eu","threadId":"46743","inReplyTo":"68633b20-9e2b-ae23-2ede-8728283250f0@grubix.eu","subject":"[PATCH 3/3] merge-base: find fork-point outside partial reflog","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-09-14T13:15:20Z","receivedAt":"2017-09-14T13:15:45Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"In fork-point mode, merge-base adds the commit at refname to a list of\ncandidates to walk from only when the reflog is empty. Therefore, it\nfails to find merge bases that are descendants of the most recent reflog\nentry.\n\nAdd the commit at the tip unconditionally so that a merge base on the\nhistory of refname is found in any case, independent of the state of the\nreflog.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/merge-base.c | 2 +-\n t/t6010-merge-base.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex 926a7615ea..b5b4cf7eac 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -174,7 +174,7 @@ 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_oid(refname, &oid))\n+\tif (!get_oid(refname, &oid))\n \t\tadd_one_commit(&oid, &revs);\n \n \tfor (i = 0; i < revs.nr; i++)\ndiff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh\nindex 850463d4f2..78342896c7 100755\n--- a/t/t6010-merge-base.sh\n+++ b/t/t6010-merge-base.sh\n@@ -275,6 +275,14 @@ test_expect_success '--fork-point works with merge-base outside reflog' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--fork-point works with merge-base outside partial reflog' '\n+\tgit -c core.logallrefupdates=true branch partial-reflog base &&\n+\tgit rev-parse no-reflog >.git/refs/heads/partial-reflog &&\n+\tgit rev-parse no-reflog >expect &&\n+\tgit merge-base --fork-point partial-reflog no-reflog >actual &&\n+\ttest_cmp expect 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.14.1.712.gda4591c8a2\n\n"},{"id":"328051","messageId":"80f7dcfaa9162dc795f8c072effea44fa83ea196.1505394278.git.git@grubix.eu","threadId":"46743","inReplyTo":"68633b20-9e2b-ae23-2ede-8728283250f0@grubix.eu","subject":"[PATCH 1/3] t6010: test actual test output","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-09-14T13:15:18Z","receivedAt":"2017-09-14T13:15:47Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n2016-10-12) introduced a fix for merge-base --fork-point without reflog\nand a test. While that test is fine, it did not update expected nor actual\noutput and tested that of the previous test instead.\n\nFix the test to test the merge-base output, not just its return code.\n\nSigned-off-by: Michael J Gruber \n---\n t/t6010-merge-base.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh\nindex 31db7b5f91..17fffd7998 100755\n--- a/t/t6010-merge-base.sh\n+++ b/t/t6010-merge-base.sh\n@@ -262,8 +262,9 @@ test_expect_success 'using reflog to find the fork point' '\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+\tgit rev-parse base >expect &&\n+\tgit merge-base --fork-point no-reflog derived >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'merge-base --octopus --all for complex tree' '\n-- \n2.14.1.712.gda4591c8a2\n\n"},{"id":"328052","messageId":"alpine.DEB.2.21.1.1709141547100.4132@virtualbox","threadId":"46743","inReplyTo":"cover.1505394278.git.git@grubix.eu","subject":"Re: [PATCH 0/3] merge-base --fork-point fixes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-14T13:49:14Z","receivedAt":"2017-09-14T13:49:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Thu, 14 Sep 2017, Michael J Gruber wrote:\n\n> merge-base --fork-point does not quite work as advertised when the\n> reflog is empty or partial. This series brings it in line with the\n> documentation and, hopefully, with the original intent.\n\nThank you so much for working on this. I recently tried to use\n--fork-point in a script of mine and it failed in this surprising way,\ntoo. So here's hoping for this patch series being accepted quickly (and\nsorry in advance for not having time to review...).\n\nCiao,\nDscho\n"},{"id":"328055","messageId":"20170914143450.tmomqf2vs55tk3nz@sigill.intra.peff.net","threadId":"46743","inReplyTo":"80f7dcfaa9162dc795f8c072effea44fa83ea196.1505394278.git.git@grubix.eu","subject":"Re: [PATCH 1/3] t6010: test actual test output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-14T14:34:50Z","receivedAt":"2017-09-14T14:34:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 14, 2017 at 03:15:18PM +0200, Michael J Gruber wrote:\n\n> 4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n> 2016-10-12) introduced a fix for merge-base --fork-point without reflog\n> and a test. While that test is fine, it did not update expected nor actual\n> output and tested that of the previous test instead.\n\nOops. The use of the existing \"expect3\" was intentional (the idea being\nthat we are repeating the identical step from the previous test with the\nslight tweak of not having a reflog). But obviously failing to write to\n\"actual\" at all was a mistake.\n\nThat said, I'm OK with reiterating the expected output.\n\n-Peff\n"},{"id":"328056","messageId":"20170914143746.nnl27a7r4spklooj@sigill.intra.peff.net","threadId":"46743","inReplyTo":"f16c671b268432199a0b2e0d22bb1b786a44bb2a.1505394278.git.git@grubix.eu","subject":"Re: [PATCH 3/3] merge-base: find fork-point outside partial reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-14T14:37:46Z","receivedAt":"2017-09-14T14:37:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 14, 2017 at 03:15:20PM +0200, Michael J Gruber wrote:\n\n> In fork-point mode, merge-base adds the commit at refname to a list of\n> candidates to walk from only when the reflog is empty. Therefore, it\n> fails to find merge bases that are descendants of the most recent reflog\n> entry.\n> \n> Add the commit at the tip unconditionally so that a merge base on the\n> history of refname is found in any case, independent of the state of the\n> reflog.\n\nOK, so the reflog we are interested in here is one where the tip commit\nis not mentioned. I'm not sure how one gets into that state, but I agree\nit makes sense to act as if it's there.\n\n-Peff\n"},{"id":"328057","messageId":"20170914143839.l5u7qof6xgwsojbd@sigill.intra.peff.net","threadId":"46743","inReplyTo":"cover.1505394278.git.git@grubix.eu","subject":"Re: [PATCH 0/3] merge-base --fork-point fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-14T14:38:39Z","receivedAt":"2017-09-14T14:38:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 14, 2017 at 03:15:17PM +0200, Michael J Gruber wrote:\n\n> merge-base --fork-point does not quite work as advertised when the\n> reflog is empty or partial. This series brings it in line with the\n> documentation and, hopefully, with the original intent.\n> \n> Michael J Gruber (3):\n> t6010: test actual test output\n> merge-base: return fork-point outside reflog\n> merge-base: find fork-point outside partial reflog\n\nPatches 1 and 3 look sane to me. Your argument in patch 2 looks\nplausible to me, but I'm not familiar enough with the mechanics of\nfork-point to have a solid opinion.\n\n-Peff\n"},{"id":"328094","messageId":"xmqq60ckzng7.fsf@gitster.mtv.corp.google.com","threadId":"46743","inReplyTo":"5513a1415d11517c28158d9b4212d383a233182f.1505394278.git.git@grubix.eu","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-15T02:48:40Z","receivedAt":"2017-09-15T02:48:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> In fact, per documentation \"--fork-point\" looks at the reflog in\n> addition to doing the usual walk from the tip. The original design\n> description in d96855ff51 (\"merge-base: teach \"--fork-point\" mode\",\n> 2013-10-23) describes this as computing from a virtual merge-base of all\n> the historical tips of refname. They may or may not all be present in\n> the reflog (think pruning, non-ff fetching, fast forwarding etc.),\n> so filtering by the current contents of the reflog is potentially\n> harmful, and it does not seem to fulfill any purpose in the original\n> design.\n\nLet me think aloud, using the picture from the log message from that\ncommit.\n\n o---B1\n /\n ---o---o---B2--o---o---o---Base\n \\\n B3\n \\\n Derived\n \n where the current tip of the \"base\" branch is at Base, but earlier\n fetch observed that its tip used to be B3 and then B2 and then B1\n before getting to the current commit, and the branch being rebased\n on top of the latest \"base\" is based on commit B3.\n\nSo the logic tries to find a merge base between \"Derived\" and a\nvirtual merge commit across Base, B1, B2 and B3. And it finds B3.\n\nIf for some reason we didn't have B3 in the reflog, then wouldn't\nthe merge base computation between Derived and a virtual merge\ncommit across Base, B2 and B2 (but not B3 because we no longer know\nabout it due to its lack in the reflog) find 'o' that is the parent\nof B2 and B3? Wouldn't that lead to both B3 and Derived replayed\nwhen the user of the fork-point potion rebases the history of\nDerived?\n\nPerhaps that is the best we could do with a pruned reflog that lacks\nB3, but if that is the case, I wonder if it may be better to fail\nthe request saying that we cannot find the fork-point (because,\nwell, your reflog no longer has sufficient information), than\nsilently give a wrong result, and in this case, we can tell between\na correct result (i.e. the merge base is one of the commits we still\nknow was at the tip) and a wrong one (i.e. the merge base is not any\nof the commits in the reflog).\n\nIf we declare --fork-point is the best effort option and may give an\nanswer that is not better than without the option, then I think this\npatch is OK, but that diminishes the value of the option as a\nbuilding block, I am afraid.\n\nCallers that are more careful could ask merge-base with --fork-point\n(and happily use it knowing that the result is indeed a commit that\nused to be at the tip), or fall back to the result merge-base\nwithout --fork-point gives (because you could do no better) and deal\nwith duplicates that may happen due to the imprecise determination.\nWith this change, these callers will get result from a call to\n\"merge-base --fork-point\" that may or may not be definite, and they\ncannot tell. For lazy users, making the option itself to fall back\nmay be simpler to use, and certainly is a valid stance to take when\nimplementing a convenience option to a Porcelain command, but I do\nnot know if it is a good idea to throw \"merge-base --fork-point\"\ninto that category.\n\n\n\n>\n> Remove the filtering and add a test for an out-of-reflog merge base.\n>\n> Reported-by: Ekelhart Jakob \n> Signed-off-by: Michael J Gruber \n> ---\n> builtin/merge-base.c | 18 +++---------------\n> t/t6010-merge-base.sh | 8 ++++++++\n> 2 files changed, 11 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/merge-base.c b/builtin/merge-base.c\n> index 6dbd167d3b..926a7615ea 100644\n> --- a/builtin/merge-base.c\n> +++ b/builtin/merge-base.c\n> @@ -186,23 +186,11 @@ static int handle_fork_point(int argc, const char **argv)\n> \t * There should be one and only one merge base, when we found\n> \t * a common ancestor among reflog entries.\n> \t */\n> -\tif (!bases || bases->next) {\n> +\tif (!bases || bases->next)\n> \t\tret = 1;\n> -\t\tgoto cleanup_return;\n> -\t}\n> -\n> -\t/* And the found one must be one of the reflog entries */\n> -\tfor (i = 0; i < revs.nr; i++)\n> -\t\tif (&bases->item->object == &revs.commit[i]->object)\n> -\t\t\tbreak; /* found */\n> -\tif (revs.nr <= i) {\n> -\t\tret = 1; /* not found */\n> -\t\tgoto cleanup_return;\n> -\t}\n> -\n> -\tprintf(\"%s\\n\", oid_to_hex(&bases->item->object.oid));\n> +\telse\n> +\t\tprintf(\"%s\\n\", oid_to_hex(&bases->item->object.oid));\n> \n> -cleanup_return:\n> \tfree_commit_list(bases);\n> \treturn ret;\n> }\n> diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh\n> index 17fffd7998..850463d4f2 100755\n> --- a/t/t6010-merge-base.sh\n> +++ b/t/t6010-merge-base.sh\n> @@ -267,6 +267,14 @@ test_expect_success '--fork-point works with empty reflog' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '--fork-point works with merge-base outside reflog' '\n> +\tgit -c core.logallrefupdates=false checkout no-reflog &&\n> +\tgit -c core.logallrefupdates=false commit --allow-empty -m \"Commit outside reflogs\" &&\n> +\tgit rev-parse base >expect &&\n> +\tgit merge-base --fork-point no-reflog derived >actual &&\n> +\ttest_cmp expect 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":"328110","messageId":"2fe2aae7-49cf-6ad5-1635-bdeab6e60b99@talktalk.net","threadId":"46743","inReplyTo":"xmqq60ckzng7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2017-09-15T09:59:03Z","receivedAt":"2017-09-15T09:59:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 15/09/17 03:48, Junio C Hamano wrote:\n> \n> Michael J Gruber writes:\n> \n>> In fact, per documentation \"--fork-point\" looks at the reflog in\n>> addition to doing the usual walk from the tip. The original design\n>> description in d96855ff51 (\"merge-base: teach \"--fork-point\" mode\",\n>> 2013-10-23) describes this as computing from a virtual merge-base of all\n>> the historical tips of refname. They may or may not all be present in\n>> the reflog (think pruning, non-ff fetching, fast forwarding etc.),\n>> so filtering by the current contents of the reflog is potentially\n>> harmful, and it does not seem to fulfill any purpose in the original\n>> design.\n> \n> Let me think aloud, using the picture from the log message from that\n> commit.\n> \n> o---B1\n> /\n> ---o---o---B2--o---o---o---Base\n> \\\n> B3\n> \\\n> Derived\n> \n> where the current tip of the \"base\" branch is at Base, but earlier\n> fetch observed that its tip used to be B3 and then B2 and then B1\n> before getting to the current commit, and the branch being rebased\n> on top of the latest \"base\" is based on commit B3.\n> \n> So the logic tries to find a merge base between \"Derived\" and a\n> virtual merge commit across Base, B1, B2 and B3. And it finds B3.\n\n\nThanks for this explanation, I've never been sure exactly what\n--fork-point did, after reading this I think I understand it.\n\n> If for some reason we didn't have B3 in the reflog, then wouldn't\n> the merge base computation between Derived and a virtual merge\n> commit across Base, B2 and B2 (but not B3 because we no longer know\n> about it due to its lack in the reflog) find 'o' that is the parent\n> of B2 and B3? Wouldn't that lead to both B3 and Derived replayed\n> when the user of the fork-point potion rebases the history of\n> Derived?\n> \n> Perhaps that is the best we could do with a pruned reflog that lacks\n> B3, but if that is the case, I wonder if it may be better to fail\n> the request saying that we cannot find the fork-point (because,\n> well, your reflog no longer has sufficient information), than\n> silently give a wrong result, and in this case, we can tell between\n> a correct result (i.e. the merge base is one of the commits we still\n> know was at the tip) and a wrong one (i.e. the merge base is not any\n> of the commits in the reflog).\n> \n> If we declare --fork-point is the best effort option and may give an\n> answer that is not better than without the option, then I think this\n> patch is OK, but that diminishes the value of the option as a\n> building block, I am afraid.\n\nI'd tend to agree with you that it would be better to fail rather than\ngive a best effort. I've got a script that I use to sync the branch I'm\nworking on between my desktop and laptop. When it pulls it checks the\nlocal head and the compares it to the remote head before pulling. If\nthey match then it does 'git reset --hard $new_remote', if the local\nhead is descended from the old remote head then it does 'git rebase\n--onto $new_remote $old_remote', otherwise it refuses to update the\nlocal head. If I've understood --fork-point correctly then I could use\n'git rebase --fork-point $remote_branch' if it failed when it couldn't\nfind a merge base in the reflog of the remote branch.\n\n> \n> Callers that are more careful could ask merge-base with --fork-point\n> (and happily use it knowing that the result is indeed a commit that\n> used to be at the tip), or fall back to the result merge-base\n> without --fork-point gives (because you could do no better) and deal\n> with duplicates that may happen due to the imprecise determination.\n> With this change, these callers will get result from a call to\n> \"merge-base --fork-point\" that may or may not be definite, and they\n> cannot tell. For lazy users, making the option itself to fall back\n> may be simpler to use, and certainly is a valid stance to take when\n> implementing a convenience option to a Porcelain command, but I do\n> not know if it is a good idea to throw \"merge-base --fork-point\"\n> into that category.\n> \n\nBest Wishes\n\nPhillip\n"},{"id":"328112","messageId":"089cdbf6-7556-6fba-21f2-6602c5ca8920@grubix.eu","threadId":"46743","inReplyTo":"20170914143450.tmomqf2vs55tk3nz@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] t6010: test actual test output","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-09-15T10:01:27Z","receivedAt":"2017-09-15T10:01:40Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 14.09.2017 16:34:\n> On Thu, Sep 14, 2017 at 03:15:18PM +0200, Michael J Gruber wrote:\n> \n>> 4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n>> 2016-10-12) introduced a fix for merge-base --fork-point without reflog\n>> and a test. While that test is fine, it did not update expected nor actual\n>> output and tested that of the previous test instead.\n> \n> Oops. The use of the existing \"expect3\" was intentional (the idea being\n> that we are repeating the identical step from the previous test with the\n> slight tweak of not having a reflog). But obviously failing to write to\n> \"actual\" at all was a mistake.\n> \n> That said, I'm OK with reiterating the expected output.\n\nexpect3 had a different content, too ;)\n\nMichael\n"},{"id":"328113","messageId":"cd97bb1b-13f3-0856-a250-8f4921b9f6d8@grubix.eu","threadId":"46743","inReplyTo":"xmqq60ckzng7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-09-15T10:23:33Z","receivedAt":"2017-09-15T10:23:46Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 15.09.2017 04:48:\n> Michael J Gruber writes:\n> \n>> In fact, per documentation \"--fork-point\" looks at the reflog in\n>> addition to doing the usual walk from the tip. The original design\n>> description in d96855ff51 (\"merge-base: teach \"--fork-point\" mode\",\n>> 2013-10-23) describes this as computing from a virtual merge-base of all\n>> the historical tips of refname. They may or may not all be present in\n>> the reflog (think pruning, non-ff fetching, fast forwarding etc.),\n>> so filtering by the current contents of the reflog is potentially\n>> harmful, and it does not seem to fulfill any purpose in the original\n>> design.\n> \n> Let me think aloud, using the picture from the log message from that\n> commit.\n> \n> o---B1\n> /\n> ---o---o---B2--o---o---o---Base\n> \\\n> B3\n> \\\n> Derived\n> \n> where the current tip of the \"base\" branch is at Base, but earlier\n> fetch observed that its tip used to be B3 and then B2 and then B1\n> before getting to the current commit, and the branch being rebased\n> on top of the latest \"base\" is based on commit B3.\n> \n> So the logic tries to find a merge base between \"Derived\" and a\n> virtual merge commit across Base, B1, B2 and B3. And it finds B3.\n> \n> If for some reason we didn't have B3 in the reflog, then wouldn't\n> the merge base computation between Derived and a virtual merge\n> commit across Base, B2 and B2 (but not B3 because we no longer know\n> about it due to its lack in the reflog) find 'o' that is the parent\n> of B2 and B3? \n\nYes.\n\n> Wouldn't that lead to both B3 and Derived replayed\n> when the user of the fork-point potion rebases the history of\n> Derived?\n\nReplayed, yes. What that means would depend on how B3 ended up being\n\"off base\" (reset or rebase, e.g.): the replay could lead to a reapply\nwithout conflict, or with conflict, or an empty (discarded) commit,\ndepending on \"how much of B3\" is still \"on base\".\n\n> Perhaps that is the best we could do with a pruned reflog that lacks\n> B3, but if that is the case, I wonder if it may be better to fail\n> the request saying that we cannot find the fork-point (because,\n> well, your reflog no longer has sufficient information), than\n> silently give a wrong result, and in this case, we can tell between\n> a correct result (i.e. the merge base is one of the commits we still\n> know was at the tip) and a wrong one (i.e. the merge base is not any\n> of the commits in the reflog).\n> \n> If we declare --fork-point is the best effort option and may give an\n> answer that is not better than without the option, then I think this\n> patch is OK, but that diminishes the value of the option as a\n> building block, I am afraid.\n> \n> Callers that are more careful could ask merge-base with --fork-point\n> (and happily use it knowing that the result is indeed a commit that\n> used to be at the tip), or fall back to the result merge-base\n> without --fork-point gives (because you could do no better) and deal\n> with duplicates that may happen due to the imprecise determination.\n> With this change, these callers will get result from a call to\n> \"merge-base --fork-point\" that may or may not be definite, and they\n> cannot tell. For lazy users, making the option itself to fall back\n> may be simpler to use, and certainly is a valid stance to take when\n> implementing a convenience option to a Porcelain command, but I do\n> not know if it is a good idea to throw \"merge-base --fork-point\"\n> into that category.\n\nSimply put, \"git merge-base ref commit\" looks at the (graph) history of\nref and considers merge-base candidates that are also in the graph\nhistory of commit. This is the \"graph notion\" of merge-base, and the\nresult is immanently determined by the DAG.\n\nThere is also the \"reflog notion\" where you look at the \"reflog history\"\nof ref. The result depends on the reflog, which itself is \"volatile\"\n(think prune), and such is the result.\n\nNow, the original documentation of merge-base says that \"merge-base\n--fork-point\" looks at the reflog of ref \"also\" (*in addition to*) the\nDAG. That is, the merge-base candidates for \"merge-base --fork-point\"\nare a super-set of those for the plain mode, enhanced by the reflog.\n(graph notion plus reflog notion)\n\nThe original implementation makes sure that \"merge-base --fork-point\"\nlooks *only* at candidates from the reflog. (strict reflog notion)\n\nThat is a discrepancy that we should resolve in any case.\n\nNote that with a \"complete reflog\", the set of reflog merge-base\ncandidates is a superset of the one from the DAG.\n\nI did not look up the discussion preceeding 4f21454b55 (\"merge-base:\nhandle --fork-point without reflog\", 2016-10-12), but if \"merge-base\n--fork-point\" were about a \"strict reflog\" notion then there was nothing\nto fix back then - no reflog, no merge-base candidates. Period.\n\nI don't mind having two modes, say \"--reflog\" (strict reflog notion) and\n\"--fork-point\" (reflog plus DAG), but the current implementation is\nneither, and the current documentation clearly is the latter, which is\nwhat I'm trying to bring the implementaion in line with. Strict mode\nwould need a revert of 4f21454b55 (which adds the tip of ref if the\nreflog is empty) for that mode.\n\nMichael\n"},{"id":"328116","messageId":"20170915112028.ksrpv637jsd7giox@sigill.intra.peff.net","threadId":"46743","inReplyTo":"089cdbf6-7556-6fba-21f2-6602c5ca8920@grubix.eu","subject":"Re: [PATCH 1/3] t6010: test actual test output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-15T11:20:29Z","receivedAt":"2017-09-15T11:20:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 15, 2017 at 12:01:27PM +0200, Michael J Gruber wrote:\n\n> Jeff King venit, vidit, dixit 14.09.2017 16:34:\n> > On Thu, Sep 14, 2017 at 03:15:18PM +0200, Michael J Gruber wrote:\n> > \n> >> 4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n> >> 2016-10-12) introduced a fix for merge-base --fork-point without reflog\n> >> and a test. While that test is fine, it did not update expected nor actual\n> >> output and tested that of the previous test instead.\n> > \n> > Oops. The use of the existing \"expect3\" was intentional (the idea being\n> > that we are repeating the identical step from the previous test with the\n> > slight tweak of not having a reflog). But obviously failing to write to\n> > \"actual\" at all was a mistake.\n> > \n> > That said, I'm OK with reiterating the expected output.\n> \n> expect3 had a different content, too ;)\n\nOh, then it was just horribly broken, then. Oops. :)\n\nThanks for fixing.\n\n-Peff\n"},{"id":"328140","messageId":"xmqqshfnx1kl.fsf@gitster.mtv.corp.google.com","threadId":"46743","inReplyTo":"cd97bb1b-13f3-0856-a250-8f4921b9f6d8@grubix.eu","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-15T18:24:10Z","receivedAt":"2017-09-15T18:24:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> I did not look up the discussion preceeding 4f21454b55 (\"merge-base:\n> handle --fork-point without reflog\", 2016-10-12), but if \"merge-base\n> --fork-point\" were about a \"strict reflog\" notion then there was nothing\n> to fix back then - no reflog, no merge-base candidates. Period.\n>\n> I don't mind having two modes, say \"--reflog\" (strict reflog notion) and\n> \"--fork-point\" (reflog plus DAG), but the current implementation is\n> neither, and the current documentation clearly is the latter, which is\n> what I'm trying to bring the implementaion in line with. Strict mode\n> would need a revert of 4f21454b55 (which adds the tip of ref if the\n> reflog is empty) for that mode.\n\nThanks for pointing out a flaw in the logic in my response.\n\nWhen the user runs \"merge-base --fork-point Branch Derived\", the\nquestion she is asking is: \"what is the merge-base between the\nDerived and a virtual commit across all the commits that we know\nwere at the tip of the Branch at some point and is the merge-base\ncommit among one of these historical tips of Branch?\"\n\nYou are correct to point out that loosening the definition of the\n\"--fork-point\" to lose the \"and is the merge-base among the one of\nthese historical tips?\" half of the question may be useful, and we\nneed to introduce \"strict\" vs \"non-strict\" modes in order to allow\nsuch a distinction. I somehow thought that giving and not giving\nthe \"--fork-point\" option would be a sufficient clue to express that\ndistinction, but that is not the same thing and my reasoning was\nflawed. Even when the exact answer of the more strict version of\nthe \"--fork-point\" (i.e. what the current implementation computes)\nis not available because of missing reflog entries, the merge-base\ncomputation that takes available reflog entries into account but\nthat does not insist that the resulting base must be among the known\nhistorical tips (i.e. what your patch 2/3 wants to compute) could be\ncloser to the fork-point than \"merge-base Branch Derived\" without\n\"--fork-point\" option would compute, and could be more useful as a\n\"best --onto commit that is guessed automatically\" for the purpose\nof \"rebase\". I agree that there is a value in what your patch 2/3\nwants to do when the current one that is more strict would say\n\"there is no known fork-point\"---we would gain a way to say \"... but\nthis is the best guess based on available data that may be better\nthan getting no answer.\" which we lack.\n\nHaving said all that, I do not agree with your last sentence in the\nparagraph I quoted above. It is a mere implementation detail to\nconsult the reflog to find out the set of \"historical tips of the\nBranch\"; the current tip by definition is among the commits in that\nset, even when the reflog of Branch is missing. What 4f21454b55 did\nwas a reasonable \"fix\" that is still in line with the definition of\n\"--fork-point\" from that point of view.\n\nWhether we add a \"looser\" version of \"--fork-point\" to the system or\nnot, the more strict version should still use the current tip as one\nof the historical tips (i.e. those that we would take from the\nreflog if the reflog were not empty) in the more \"strict\" mode. The\nlooser version may also want to do so as well.\n\nThanks.\n\n"},{"id":"328552","messageId":"xmqqshfgk1mr.fsf@gitster.mtv.corp.google.com","threadId":"46743","inReplyTo":"xmqqshfnx1kl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-21T06:27:08Z","receivedAt":"2017-09-21T06:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ... I agree that there is a value in what your patch 2/3\n> wants to do when the current one that is more strict would say\n> \"there is no known fork-point\"---we would gain a way to say \"... but\n> this is the best guess based on available data that may be better\n> than getting no answer.\" which we lack.\n>\n> Having said all that, I do not agree with your last sentence in the\n> paragraph I quoted above. It is a mere implementation detail to\n> consult the reflog to find out the set of \"historical tips of the\n> Branch\"; the current tip by definition is among the commits in that\n> set, even when the reflog of Branch is missing. What 4f21454b55 did\n> was a reasonable \"fix\" that is still in line with the definition of\n> \"--fork-point\" from that point of view.\n>\n> Whether we add a \"looser\" version of \"--fork-point\" to the system or\n> not, the more strict version should still use the current tip as one\n> of the historical tips (i.e. those that we would take from the\n> reflog if the reflog were not empty) in the more \"strict\" mode. The\n> looser version may also want to do so as well.\n\nSo, should I mark this in What's cooking report as \"expecting a\nreroll\", anticipating that a new option would be added to trigger\nthe new & looser behaviour?\n\n"},{"id":"328564","messageId":"5a2fca1d-4edf-965f-4840-58c924c91051@grubix.eu","threadId":"46743","inReplyTo":"xmqqshfgk1mr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-09-21T09:39:12Z","receivedAt":"2017-09-21T09:39:30Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 21.09.2017 08:27:\n> Junio C Hamano writes:\n> \n>> ... I agree that there is a value in what your patch 2/3\n>> wants to do when the current one that is more strict would say\n>> \"there is no known fork-point\"---we would gain a way to say \"... but\n>> this is the best guess based on available data that may be better\n>> than getting no answer.\" which we lack.\n>>\n>> Having said all that, I do not agree with your last sentence in the\n>> paragraph I quoted above. It is a mere implementation detail to\n>> consult the reflog to find out the set of \"historical tips of the\n>> Branch\"; the current tip by definition is among the commits in that\n>> set, even when the reflog of Branch is missing. What 4f21454b55 did\n>> was a reasonable \"fix\" that is still in line with the definition of\n>> \"--fork-point\" from that point of view.\n>>\n>> Whether we add a \"looser\" version of \"--fork-point\" to the system or\n>> not, the more strict version should still use the current tip as one\n>> of the historical tips (i.e. those that we would take from the\n>> reflog if the reflog were not empty) in the more \"strict\" mode. The\n>> looser version may also want to do so as well.\n> \n> So, should I mark this in What's cooking report as \"expecting a\n> reroll\", anticipating that a new option would be added to trigger\n> the new & looser behaviour?\n> \n\nI dunno. Some participants in this thread considered my patch to be a\nfix rather than alternative behaviour. So I hoped for more responses to\nyour response. (Re-adding dscho on cc - our thread graph forked...)\n\nAlso, I'm undecided about about your reflog argument above - if we leave\n\"--fork-point\" to be the current behaviour including Jeff's fix then the\ndocumentation would need an even bigger overhaul, because it's neither\n\"reflog also\" (as claimed in the doc) nor \"reflog only\" (as in the\noriginal implementation) but \"historical tips as inferred from the\ncurrent value and the reflog\".\n\nIn any case, for two modes we need two names for the options. Maybe\n--fork-point and --fork-base because in the loose mode, you may get a\n\"base of a strict fork point\"?\n\nMichael\n"},{"id":"328608","messageId":"xmqqzi9nlcyw.fsf@gitster.mtv.corp.google.com","threadId":"46743","inReplyTo":"5a2fca1d-4edf-965f-4840-58c924c91051@grubix.eu","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-22T01:49:11Z","receivedAt":"2017-09-22T01:49:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Also, I'm undecided about about your reflog argument above - if we leave\n> \"--fork-point\" to be the current behaviour including Jeff's fix then the\n> documentation would need an even bigger overhaul, because it's neither\n> \"reflog also\" (as claimed in the doc) nor \"reflog only\" (as in the\n> original implementation) but \"historical tips as inferred from the\n> current value and the reflog\".\n\nEven though things like \"reflog only\", \"reflog also\", may be\nsomething implementors may care about, they are irrelevant\nimplementation details to the intended audience. \"The bases that\nare not in reflogs are ignored\" _does_ matter, as it affects the\noutcome, and that may be a bit too strict a filter (which this\nseries takes us in a good direction to fix) but what the readers\nneed to know is the real-world implications of the choices made at\nthe implementation detail level, and more importantly, what the\nimplementation is trying to compute.\n\nIt is a documentation bug (with or without these patches) if the\ncurrent text gives an impression that the code is trying to do\nanything but \"guessing the fork point using historical tips\".\n\n> In any case, for two modes we need two names for the options. Maybe\n> --fork-point and --fork-base because in the loose mode, you may get a\n> \"base of a strict fork point\"?\n\nPerhaps.\n"},{"id":"328625","messageId":"4a14c35c-88b0-7bc3-0a6e-c69ed9ec7ee7@grubix.eu","threadId":"46743","inReplyTo":"xmqqzi9nlcyw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-09-22T08:34:52Z","receivedAt":"2017-09-22T08:35:10Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 22.09.2017 03:49:\n> Michael J Gruber writes:\n> \n>> Also, I'm undecided about about your reflog argument above - if we leave\n>> \"--fork-point\" to be the current behaviour including Jeff's fix then the\n>> documentation would need an even bigger overhaul, because it's neither\n>> \"reflog also\" (as claimed in the doc) nor \"reflog only\" (as in the\n>> original implementation) but \"historical tips as inferred from the\n>> current value and the reflog\".\n> \n> Even though things like \"reflog only\", \"reflog also\", may be\n> something implementors may care about, they are irrelevant\n> implementation details to the intended audience. \"The bases that\n> are not in reflogs are ignored\" _does_ matter, as it affects the\n> outcome, and that may be a bit too strict a filter (which this\n> series takes us in a good direction to fix) but what the readers\n> need to know is the real-world implications of the choices made at\n> the implementation detail level, and more importantly, what the\n> implementation is trying to compute.\n> \n> It is a documentation bug (with or without these patches) if the\n> current text gives an impression that the code is trying to do\n> anything but \"guessing the fork point using historical tips\".\n\nI'm still trying to understand what the original intent was: If we\nabstract from the implementation (as we should, as you rightly\nemphasize) and talk about historical tips then we have to ask ourselves:\n- What is \"historical\"?\n- What is tip?\n- Tip of what, i.e. what is a \"branch\"?\n\nIf by \"branch\" we mean the moving branch ref that it is in git then by\nall means, historical tips are the values that that ref ever had, and\nall that we can say is that this includes the current value and the\ncurrent contents of the branch refs's reflog (which may or may not be\n\"complete\").\n\nNote that this notion of \"branch\" is completely independent of the DAG,\nwhereas by definition a \"merge-base\" is a concept that relies on an\nancestry graph.\n\nIf by \"branch\" we mean everything that is \"on\" (or in, think\n\"--contains\") that branch - and I assume that is how most users think\nabout a branch - then it is not clear at all why we should focus on\n\"historical values that that refname had\", which is an implemenation\ndetail in itself (branch refs is how we implement the branch concept).\n\nEspecially, it's not clear why we should exclude for example a commit\nthat is in between two commits that are in the reflog (\"historical\ntips\") of a branch that has been fast forwarded or reset (-A, then fast\nforward to -A-B-C; this excludes B from the list of merge-base candidates).\n\n>> In any case, for two modes we need two names for the options. Maybe\n>> --fork-point and --fork-base because in the loose mode, you may get a\n>> \"base of a strict fork point\"?\n> \n> Perhaps.\n> \n"},{"id":"328628","messageId":"xmqqa81njds0.fsf@gitster.mtv.corp.google.com","threadId":"46743","inReplyTo":"4a14c35c-88b0-7bc3-0a6e-c69ed9ec7ee7@grubix.eu","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-22T09:14:39Z","receivedAt":"2017-09-22T09:14:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> I'm still trying to understand what the original intent was: If we\n> abstract from the implementation (as we should, as you rightly\n> emphasize) and talk about historical tips then we have to ask ourselves:\n> - What is \"historical\"?\n> - What is tip?\n> - Tip of what, i.e. what is a \"branch\"?\n\nThe feature was meant to be a solution for \"upstream rebased the\nbranch I based my work on.\"\n\nSuppose you did\n\n\tgit clone $URL\n\tgit checkout -b mytopic origin/topic\n\twork work work and commit commit commit\n\nand then the next \"git fetch\" found that remote/origin/topic did not\nfast-forward, i.e. your upstream rewound and rebuilt the topic.\n\nNow, running\n\n\tgit rebase origin/topic mytopic\n\nwould be a disaster, as it would try to replay all the commits\nreachable from mytopic on top of the updated origin/topic, but the\nset of commits reachable from mytopic includes those the upstream\nused to have, on top of which you based your work, and you are not\ninterested in replaying them at all.\n\nIf you remember where you forked your topic from (you should be able\nto tell from \"git log mytopic\"), then\n\n\tgit rebase --onto origin/topic $fork_point mytopic\n\nwould be the way to replay \"your\" work on mytopic on top of the\nupdated upstream.\n\nThe reason why \"--fork-point\" exists is because people wanted to\nautomate the \"remember where you forked your topic from\" part.\n\nYour upstream might have rewound the tip of topic many times while\nyour repository did not fetch from it (iow, the reflog for\norigin/topic may not have _all_ the tips of other people observed to\nbe historicallly at the tip of the remote), but that is immaterial\nfor our purpose. As long as you built on top of origin/topic, the\nfork point must be one of the commits _you_ saw at the tip and it\ndoes not matter that you did not constantly fetch to observe all the\nchanges at the remote.\n\nSo the answers of these three questions are:\n\n - \"historical\": _you_ saw in your repository;\n\n - \"tip\": _you_ saw it pointed by the branch you forked your work\n from; and\n\n - \"branch\": whatever you consider you based your work on top of,\n typically a remote tracking branch.\n\nOf course, if you are employing a more advanced workflow, you might\nnot have started your mytopic branch at any of the tip. E.g. if\nyou wanted to extend Ben's fsmonitor topic, you would have forked\nyour own topic like\n\n\tgit checkout -b my-fsmonitor origin/pu^2\n\nafter fetching from me, and --fork-point would not be of much help\nobviously for such a workflow. But such users will use --onto and\nexplicitly specify the fork point so it is outside the scope of the\nfeature.\n\n"},{"id":"329526","messageId":"xmqq7ewckbpk.fsf@gitster.mtv.corp.google.com","threadId":"46743","inReplyTo":"xmqqa81njds0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-03T06:05:43Z","receivedAt":"2017-10-03T06:05:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Michael J Gruber writes:\n>\n>> I'm still trying to understand what the original intent was: If we\n>> abstract from the implementation (as we should, as you rightly\n>> emphasize) and talk about historical tips then we have to ask ourselves:\n>> - What is \"historical\"?\n>> - What is tip?\n>> - Tip of what, i.e. what is a \"branch\"?\n>\n> The feature was meant to be a solution for \"upstream rebased the\n> branch I based my work on.\"\n> ...\n\nSo, what is the status of this thing?\n\nWhile I think 1/3 and 3/3 of these three definitely make sense, I do\nnot think \"fork-point outside reflog\" does as-is If it is not even\npart of the commits that were known to be at the tip some time in\nthe past (including \"right now\"---which is the fix you made with 3/3\nis about), then the patch may make the command return something in\nmore situations, and these extra things that it returns might even\nbe improvements, but they are definitely not \"fork-points\".\n\nTo be quite honest, I am not convinced that the extra output you\nwould get out of the command by removing the latter half of \"which\nare the ancestors that were known to be at the tip?\" would always\ngive better commit to use as the beginning of the topic to be\nrebased, as I do not see any reasoning behind why, unlike the\nfiltered case where there _is_ a strong reasoning (with explained\nlimitation) behind it.\n\nAs long as the code misidentifies and picks a commits deeper than\nnecessary, which will force the user to say \"rebase --skip\", I think\nwe are OK. What we want to absolutely avoid is the opposite;\nsomehow the code misidentifies a commit that is part of the work you\nwant to rebase as a recommended fork-point, which would cause the\nrebase to silently lose changes. I do not think I saw why it won't\nhappen explained in the log message of 2/3 at all.\n"},{"id":"332069","messageId":"b9c2c978a6d3407c8c26509fc82b3920@SFSWW2K12EX02.intern.fsw.at","threadId":"46743","inReplyTo":"xmqq7ewckbpk.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Ekelhart Jakob","fromEmail":"jakob.ekelhart@fsw.at","sentAt":"2017-11-08T08:52:31Z","receivedAt":"2017-11-08T09:02:11Z","isPatch":true,"sender":{"key":"jakob.ekelhart@fsw.at","avatar":null},"body":"Thank you for all the effort to fix this issue. Unfortunately, we are still suffering from this and our workaround just stopped being sufficient.\n\nWe were wondering if there is any way to tell when this fix will be released?\n\nBR Jakob\n\n-----Original Message-----\nFrom: Junio C Hamano *EXTERN* [mailto:gitster@pobox.com] \nSent: Dienstag, 3. Oktober 2017 08:06\nTo: Michael J Gruber \nCc: git@vger.kernel.org; Ekelhart Jakob ; Jeff King ; Johannes Schindelin \nSubject: Re: [PATCH 2/3] merge-base: return fork-point outside reflog\n\nJunio C Hamano writes:\n\n> Michael J Gruber writes:\n>\n>> I'm still trying to understand what the original intent was: If we \n>> abstract from the implementation (as we should, as you rightly\n>> emphasize) and talk about historical tips then we have to ask ourselves:\n>> - What is \"historical\"?\n>> - What is tip?\n>> - Tip of what, i.e. what is a \"branch\"?\n>\n> The feature was meant to be a solution for \"upstream rebased the \n> branch I based my work on.\"\n> ...\n\nSo, what is the status of this thing?\n\nWhile I think 1/3 and 3/3 of these three definitely make sense, I do not think \"fork-point outside reflog\" does as-is If it is not even part of the commits that were known to be at the tip some time in the past (including \"right now\"---which is the fix you made with 3/3 is about), then the patch may make the command return something in more situations, and these extra things that it returns might even be improvements, but they are definitely not \"fork-points\".\n\nTo be quite honest, I am not convinced that the extra output you would get out of the command by removing the latter half of \"which are the ancestors that were known to be at the tip?\" would always give better commit to use as the beginning of the topic to be rebased, as I do not see any reasoning behind why, unlike the filtered case where there _is_ a strong reasoning (with explained\nlimitation) behind it.\n\nAs long as the code misidentifies and picks a commits deeper than necessary, which will force the user to say \"rebase --skip\", I think we are OK. What we want to absolutely avoid is the opposite; somehow the code misidentifies a commit that is part of the work you want to rebase as a recommended fork-point, which would cause the rebase to silently lose changes. I do not think I saw why it won't happen explained in the log message of 2/3 at all.\n\n"},{"id":"332070","messageId":"1a375938-a3dc-d670-dd41-de524ac0f93d@grubix.eu","threadId":"46743","inReplyTo":"b9c2c978a6d3407c8c26509fc82b3920@SFSWW2K12EX02.intern.fsw.at","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-11-08T09:33:41Z","receivedAt":"2017-11-08T09:34:02Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Ekelhart Jakob venit, vidit, dixit 08.11.2017 09:52:\n> Thank you for all the effort to fix this issue. Unfortunately, we are still suffering from this and our workaround just stopped being sufficient.\n> \n> We were wondering if there is any way to tell when this fix will be released?\n> \n> BR Jakob\n\nSoon (TM) :)\n\nTerm start kept me busy, but I'll try and resume dangling topics this\nweek or next.\n\nIt seems the consensus was that current functionality is as designed but\nnot necessarily as expected, and another mode \"--fork-base\" (that does\nwhat I suggested as \"fix\") would meet these expectations. I would reuse\nthe documentation of the current mode as a description of the new mode\nand add documentation for the existing mode ;)\n\nMichael\n"},{"id":"332104","messageId":"xmqq375okvxy.fsf@gitster.mtv.corp.google.com","threadId":"46743","inReplyTo":"1a375938-a3dc-d670-dd41-de524ac0f93d@grubix.eu","subject":"Re: [PATCH 2/3] merge-base: return fork-point outside reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-09T02:49:45Z","receivedAt":"2017-11-09T02:49:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> It seems the consensus was that current functionality is as designed but\n> not necessarily as expected, and another mode \"--fork-base\" (that does\n> what I suggested as \"fix\") would meet these expectations. I would reuse\n> the documentation of the current mode as a description of the new mode\n> and add documentation for the existing mode ;)\n\nIf we are going to have two modes, it would be absolutely necessary\nto explain in the documentation what they compute exactly in terms\nthat the end users understand, so that the users can choose which\none to use.\n\nThe current documentation is not that great because it can get away\nwith \"we automatically by magic find the commit on top of which you\nforked your branch\", but when we have two different kinds of magic,\nthat would no longer be a useful description to help users to\nchoose. We probably need to enhance the description in \"Discussion\non fork-point mode\" as a preliminary clean-up. The attached is my\nattempt.\n\nI'd expect `--fork-base` to be explained in a similar way and to the\nlevel of detail to help users pick which one of the three options\n(i.e. without any option, with fork-point and with the new one) is\nappropriate for their situation. I've been thinking about what your\npatch does, and I cannot come up with a reasonable way to explain\nwhat it computes in the terms the users can get their heads around,\nwith history illustrations.\n\n--- >8 ---\nSubject: merge-base --fork-point doc: clarify the example and failure modes\n\nThe illustrated history used to explain the `--fork-point` mode\nnamed three keypoint commits B3, B2 and B1 from the oldest to the\nnewest, which was hard to read. Relabel them to B0, B1, B2. Also\nillustrate the history after the rebase using the `--fork-point`\nfacility was made.\n\nThe text already mentions use of reflog, but the description is not\nclear what benefit we are trying to gain by using reflog. Clarify\nthat it is to find the commits that were known to be at the tip of\nthe remote-tracking branch. This in turn necessitates users to know\nthe ramifications of the underlying assumptions, namely, expiry of\nreflog entries will make it impossible to determine which commits\nwere at the tip of the remote-tracking branches and we fail when in\ndoubt (instead of giving a random and incorrect result without even\nwarning). Another limitation is that it won't be useful if you did\nnot fork from the tip of a remote-tracking branch but from in the\nmiddle.\n\nDescribe them.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-merge-base.txt | 64 +++++++++++++++++++++++++++++++++++-----\n 1 file changed, 56 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-merge-base.txt b/Documentation/git-merge-base.txt\nindex b968b64c38..a4859c8597 100644\n--- a/Documentation/git-merge-base.txt\n+++ b/Documentation/git-merge-base.txt\n@@ -154,23 +154,71 @@ topic origin/master`, the history of remote-tracking branch\n `origin/master` may have been rewound and rebuilt, leading to a\n history of this shape:\n \n-\t o---B1\n+\t o---B2\n \t /\n-\t---o---o---B2--o---o---o---B (origin/master)\n+\t---o---o---B1--o---o---o---B (origin/master)\n \t \\\n-\t B3\n+\t B0\n \t \\\n-\t Derived (topic)\n+\t D0---D1---D (topic)\n \n-where `origin/master` used to point at commits B3, B2, B1 and now it\n+where `origin/master` used to point at commits B0, B1, B2 and now it\n points at B, and your `topic` branch was started on top of it back\n-when `origin/master` was at B3. This mode uses the reflog of\n-`origin/master` to find B3 as the fork point, so that the `topic`\n-can be rebased on top of the updated `origin/master` by:\n+when `origin/master` was at B0, and you built three commits, D0, D1,\n+and D, on top of it. Imagine that you now want to rebase the work\n+you did on the topic on top of the updated origin/master.\n+\n+In such a case, `git merge-base origin/master topic` would return the\n+parent of B0 in the above picture, but B0^..D is *not* the range of\n+commits you would want to replay on top of B (it includes B0, which\n+is not what you wrote; it is a commit the other side discarded when\n+it moved its tip from B0 to B1). \n+\n+`git merge-base --fork-point origin/master topic` is designed to\n+help in such a case. It takes not only B but also B0, B1, and B2\n+(i.e. old tips of the remote-tracking branches your repository's\n+reflog knows about) into account to see on which commit your topic\n+branch was built and finds B0, allowing you to replay only the\n+commits on your topic, excluding the commits the other side later\n+discarded.\n+\n+Hence\n \n $ fork_point=$(git merge-base --fork-point origin/master topic)\n+\n+will find B0, and\n+\n $ git rebase --onto origin/master $fork_point topic\n \n+will replay D0, D1 and D on top of B to create a new history of this\n+shape:\n+\n+ o---B2\n+ /\n+ ---o---o---B1--o---o---o---B (origin/master)\n+ \\ \\\n+ B0 D0'--D1'--D' (topic - updated)\n+ \\\n+ D0---D1---D (topic - old)\n+\n+A caveat is that older reflog entries in your repository may be\n+expired by `git gc`. If B0 no longer appears in the reflog of the\n+remote-tracking branch `origin/master`, the `--fork-point` mode\n+obviously cannot find it and fails, avoiding to give a random and\n+useless result (such as the parent of B0, like the same command\n+without the `--fork-point` option gives). \n+\n+Also, the remote-tracking branch you use the `--fork-point` mode\n+with must be the one your topic forked from its tip. If you forked\n+from an older commit than the tip, this mode would not find the fork\n+point (imagine in the above sample history B0 did not exist,\n+origin/master started at B1, moved to B2 and then B, and you forked\n+your topic at origin/master^ when origin/master was B1; the shape of\n+the history would be the same as above, without B0, and the parent\n+of B1 is what `git merge-base origin/master topic` correctly finds,\n+but the `--fork-point` mode will not, because it is not one of the\n+commits that used to be at the tip of origin/master).\n+\n \n See also\n --------\n"}]}