{"thread":{"id":"54336","subject":"[PATCH] commit-reach: fix in_merge_bases_many bug","startedAt":"2020-10-02T14:59:02Z","lastAt":"2020-10-02T20:08:17Z","messageCount":7,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","Srinidhi Kaushik"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"406821","messageId":"pull.739.git.1601650736489.gitgitgadget@gmail.com","threadId":"54336","inReplyTo":null,"subject":"[PATCH] commit-reach: fix in_merge_bases_many bug","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-02T14:58:56Z","receivedAt":"2020-10-02T14:59:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWay back in f9b8908b (commit.c: use generation numbers for\nin_merge_bases(), 2018-05-01), a heuristic was used to short-circuit\nthe in_merge_bases() walk. This works just fine as long as the\ncaller is checking only two commits, but when there are multiple,\nthere is a possibility that this heuristic is _very wrong_.\n\nSome code moves since then has changed this method to\nrepo_in_merge_bases_many() inside commit-reach.c. The heuristic\ncomputes the minimum generation number of the \"reference\" list, then\ncompares this number to the generation number of the \"commit\".\n\nIn a recent topic, a test was added that used in_merge_bases_many()\nto test if a commit was reachable from a number of commits pulled\nfrom a reflog. However, this highlighted the problem: if any of the\nreference commits have a smaller generation number than the given\ncommit, then the walk is skipped _even if there exist some with\nhigher generation number_.\n\nThis heuristic is wrong! It must check the MAXIMUM generation number\nof the reference commits, not the MINIMUM.\n\nThis highlights a testing gap. t6600-test-reach.sh covers many\nmethods in commit-reach.c, including in_merge_bases() and\nget_merge_bases_many(), but since these methods either restrict to\ntwo input commits or actually look for the full list of merge bases,\nthey don't check this heuristic!\n\nAdd a possible input to \"test-tool reach\" that tests\nin_merge_bases_many() and add tests to t6600-test-reach.sh that\ncover this heuristic. This includes cases for the reference commits\nhaving generation above and below the generation of the input commit,\nbut also having maximum generation below the generation of the input\ncommit.\n\nThe fix itself is to swap min_generation with a max_generation in\nrepo_in_merge_bases_many().\n\nHelped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nReported-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    Fix in_merge_bases_many() with commit-graphs\n    \n    Johannes alerted me to the difficulties Srinidhi was having with \n    in_merge_bases_many() and commit-graphs. Sorry that I hadn't seen that\n    thread and the issues therein.\n    \n    After working with Johannes to investigate what was happening, we found\n    a 2-year-old bug in the generation number checks!\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-739%2Fderrickstolee%2Fin-merge-bases-many-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-739/derrickstolee/in-merge-bases-many-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/739\n\n commit-reach.c        |  8 ++++----\n t/helper/test-reach.c |  2 ++\n t/t6600-test-reach.sh | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 36 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex efd5925cbb..50175b159e 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -321,7 +321,7 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,\n {\n \tstruct commit_list *bases;\n \tint ret = 0, i;\n-\tuint32_t generation, min_generation = GENERATION_NUMBER_INFINITY;\n+\tuint32_t generation, max_generation = GENERATION_NUMBER_ZERO;\n \n \tif (repo_parse_commit(r, commit))\n \t\treturn ret;\n@@ -330,12 +330,12 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,\n \t\t\treturn ret;\n \n \t\tgeneration = commit_graph_generation(reference[i]);\n-\t\tif (generation < min_generation)\n-\t\t\tmin_generation = generation;\n+\t\tif (generation > max_generation)\n+\t\t\tmax_generation = generation;\n \t}\n \n \tgeneration = commit_graph_generation(commit);\n-\tif (generation > min_generation)\n+\tif (generation > max_generation)\n \t\treturn ret;\n \n \tbases = paint_down_to_common(r, commit,\ndiff --git a/t/helper/test-reach.c b/t/helper/test-reach.c\nindex 14a3655442..cda804ed79 100644\n--- a/t/helper/test-reach.c\n+++ b/t/helper/test-reach.c\n@@ -107,6 +107,8 @@ int cmd__reach(int ac, const char **av)\n \t\tprintf(\"%s(A,B):%d\\n\", av[1], ref_newer(&oid_A, &oid_B));\n \telse if (!strcmp(av[1], \"in_merge_bases\"))\n \t\tprintf(\"%s(A,B):%d\\n\", av[1], in_merge_bases(A, B));\n+\telse if (!strcmp(av[1], \"in_merge_bases_many\"))\n+\t\tprintf(\"%s(A,X):%d\\n\", av[1], in_merge_bases_many(A, X_nr, X_array));\n \telse if (!strcmp(av[1], \"is_descendant_of\"))\n \t\tprintf(\"%s(A,X):%d\\n\", av[1], repo_is_descendant_of(r, A, X));\n \telse if (!strcmp(av[1], \"get_merge_bases_many\")) {\ndiff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh\nindex 475564bee7..f807276337 100755\n--- a/t/t6600-test-reach.sh\n+++ b/t/t6600-test-reach.sh\n@@ -110,6 +110,36 @@ test_expect_success 'in_merge_bases:miss' '\n \ttest_three_modes in_merge_bases\n '\n \n+test_expect_success 'in_merge_bases_many:hit' '\n+\tcat >input <<-\\EOF &&\n+\tA:commit-6-8\n+\tX:commit-6-9\n+\tX:commit-5-7\n+\tEOF\n+\techo \"in_merge_bases_many(A,X):1\" >expect &&\n+\ttest_three_modes in_merge_bases_many\n+'\n+\n+test_expect_success 'in_merge_bases_many:miss' '\n+\tcat >input <<-\\EOF &&\n+\tA:commit-6-8\n+\tX:commit-7-7\n+\tX:commit-8-6\n+\tEOF\n+\techo \"in_merge_bases_many(A,X):0\" >expect &&\n+\ttest_three_modes in_merge_bases_many\n+'\n+\n+test_expect_success 'in_merge_bases_many:miss-heuristic' '\n+\tcat >input <<-\\EOF &&\n+\tA:commit-6-8\n+\tX:commit-7-5\n+\tX:commit-6-6\n+\tEOF\n+\techo \"in_merge_bases_many(A,X):0\" >expect &&\n+\ttest_three_modes in_merge_bases_many\n+'\n+\n test_expect_success 'is_descendant_of:hit' '\n \tcat >input <<-\\EOF &&\n \tA:commit-5-7\n\nbase-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc\n-- \ngitgitgadget\n"},{"id":"406823","messageId":"xmqq8scobovn.fsf@gitster.c.googlers.com","threadId":"54336","inReplyTo":"pull.739.git.1601650736489.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-reach: fix in_merge_bases_many bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-02T15:59:08Z","receivedAt":"2020-10-02T15:59:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> Way back in f9b8908b (commit.c: use generation numbers for\n> in_merge_bases(), 2018-05-01), a heuristic was used to short-circuit\n> the in_merge_bases() walk. This works just fine as long as the\n> caller is checking only two commits, but when there are multiple,\n> there is a possibility that this heuristic is _very wrong_.\n\nThanks.  That bug defeats the point of _many() part of the function.\n\n> Some code moves since then has changed this method to\n> repo_in_merge_bases_many() inside commit-reach.c. The heuristic\n> computes the minimum generation number of the \"reference\" list, then\n> compares this number to the generation number of the \"commit\".\n>\n> In a recent topic, a test was added that used in_merge_bases_many()\n> to test if a commit was reachable from a number of commits pulled\n> from a reflog. However, this highlighted the problem: if any of the\n> reference commits have a smaller generation number than the given\n> commit, then the walk is skipped _even if there exist some with\n> higher generation number_.\n>\n> This heuristic is wrong! It must check the MAXIMUM generation number\n> of the reference commits, not the MINIMUM.\n\nCorrect\n\n>     Fix in_merge_bases_many() with commit-graphs\n>     \n>     Johannes alerted me to the difficulties Srinidhi was having with \n>     in_merge_bases_many() and commit-graphs. Sorry that I hadn't seen that\n>     thread and the issues therein.\n>     \n>     After working with Johannes to investigate what was happening, we found\n>     a 2-year-old bug in the generation number checks!\n>     \n>     Thanks, -Stolee\n\nGood.  Thanks.\n\n"},{"id":"406832","messageId":"xmqqv9fs8nre.fsf@gitster.c.googlers.com","threadId":"54336","inReplyTo":"pull.739.git.1601650736489.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-reach: fix in_merge_bases_many bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-02T18:51:33Z","receivedAt":"2020-10-02T18:51:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> The fix itself is to swap min_generation with a max_generation in\n> repo_in_merge_bases_many().\n>\n> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Reported-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n\nThe order of these looked iffy so I rearranged them in chrono order,\nSrinidhi's series found it broken, and with Dscho's help, fix was\nproduced and you sent it with your sign off.\n\n>  commit-reach.c        |  8 ++++----\n>  t/helper/test-reach.c |  2 ++\n>  t/t6600-test-reach.sh | 30 ++++++++++++++++++++++++++++++\n>  3 files changed, 36 insertions(+), 4 deletions(-)\n\nI've applied this and then rebased Srinidhi's latest on top, with\nthe following to re-enable the commit-graph in the codepath.  t5533\nand t6600 passes and when the fix in the message I am responding to\nis reverted, t5533 again fails.\n\nThanks.\n\n    SQUASH??? drop \"hide the breakage under the rug\" hack\n    \n    Derrick and Dscho spotted and fixed the incorrect optimization in\n    in_merge_bases_many() when the commit-graph is in use, so there is\n    no longer a reason why we would want to disable commit-graph locally\n    in this codepath.\n\ndiff --git a/remote.c b/remote.c\nindex 98a578f5dc..8c91188677 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2405,34 +2405,18 @@ static int is_reachable_in_reflog(const char *local, const struct ref *remote)\n \treturn ret;\n }\n \n-/* Toggle the \"commit-graph\" feature; return the previously set state. */\n-static int toggle_commit_graph(struct repository *repo, int disable) {\n-\tint prev = repo->commit_graph_disabled;\n-\trepo->commit_graph_disabled = disable;\n-\treturn prev;\n-}\n-\n /*\n  * Check for reachability of a remote-tracking\n  * ref in the reflog entries of its local ref.\n  */\n static void check_if_includes_upstream(struct ref *remote)\n {\n-\tint prev;\n \tstruct ref *local = get_local_ref(remote->name);\n \tif (!local)\n \t\treturn;\n \n-\t/*\n-\t * TODO: Remove \"toggle_commit_graph()\" calls around the check.\n-\t * Depending on whether \"commit-graph\" enabled or not,\n-\t * \"in_merge_bases_many()\" returns different results;\n-\t * disable it temporarily when the check runs.\n-\t */\n-\tprev = toggle_commit_graph(the_repository, 1);\n \tif (is_reachable_in_reflog(local->name, remote) <= 0)\n \t\tremote->unreachable = 1;\n-\ttoggle_commit_graph(the_repository, prev);\n }\n \n static void apply_cas(struct push_cas_option *cas,\n"},{"id":"406835","messageId":"c6b163dd-d5f4-73a8-43ac-22460d1079ec@gmail.com","threadId":"54336","inReplyTo":"xmqqv9fs8nre.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] commit-reach: fix in_merge_bases_many bug","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-10-02T19:47:28Z","receivedAt":"2020-10-02T19:47:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/2/2020 2:51 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> The fix itself is to swap min_generation with a max_generation in\n>> repo_in_merge_bases_many().\n>>\n>> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n>> Reported-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>\n>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> \n> The order of these looked iffy so I rearranged them in chrono order,\n> Srinidhi's series found it broken, and with Dscho's help, fix was\n> produced and you sent it with your sign off.\n\nSure. I can agree with that chronological order and will keep\nthat in mind for next time.\n\n>>  commit-reach.c        |  8 ++++----\n>>  t/helper/test-reach.c |  2 ++\n>>  t/t6600-test-reach.sh | 30 ++++++++++++++++++++++++++++++\n>>  3 files changed, 36 insertions(+), 4 deletions(-)\n> \n> I've applied this and then rebased Srinidhi's latest on top, with\n> the following to re-enable the commit-graph in the codepath.  t5533\n> and t6600 passes and when the fix in the message I am responding to\n> is reverted, t5533 again fails.\n> \n> Thanks.\n\nThanks for double-checking. I also think that dropping the\n\"hide the error\" patch is prudent.\n\nThanks,\n-Stolee\n"},{"id":"406836","messageId":"20201002200308.GC78209@mail.clickyotomy.dev","threadId":"54336","inReplyTo":"pull.739.git.1601650736489.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-reach: fix in_merge_bases_many bug","fromName":"Srinidhi Kaushik","fromEmail":"shrinidhi.kaushik@gmail.com","sentAt":"2020-10-02T20:03:08Z","receivedAt":"2020-10-02T20:03:17Z","isPatch":true,"sender":{"key":"shrinidhi.kaushik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3019537?v=4"},"body":"Hello,\n\nOn 10/02/2020 14:58, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <dstolee@microsoft.com>\n> \n> Way back in f9b8908b (commit.c: use generation numbers for\n> in_merge_bases(), 2018-05-01), a heuristic was used to short-circuit\n> the in_merge_bases() walk. This works just fine as long as the\n> caller is checking only two commits, but when there are multiple,\n> there is a possibility that this heuristic is _very wrong_.\n> \n> Some code moves since then has changed this method to\n> repo_in_merge_bases_many() inside commit-reach.c. The heuristic\n> computes the minimum generation number of the \"reference\" list, then\n> compares this number to the generation number of the \"commit\".\n> \n> In a recent topic, a test was added that used in_merge_bases_many()\n> to test if a commit was reachable from a number of commits pulled\n> from a reflog. However, this highlighted the problem: if any of the\n> reference commits have a smaller generation number than the given\n> commit, then the walk is skipped _even if there exist some with\n> higher generation number_.\n> \n> This heuristic is wrong! It must check the MAXIMUM generation number\n> of the reference commits, not the MINIMUM.\n> \n> This highlights a testing gap. t6600-test-reach.sh covers many\n> methods in commit-reach.c, including in_merge_bases() and\n> get_merge_bases_many(), but since these methods either restrict to\n> two input commits or actually look for the full list of merge bases,\n> they don't check this heuristic!\n> \n> Add a possible input to \"test-tool reach\" that tests\n> in_merge_bases_many() and add tests to t6600-test-reach.sh that\n> cover this heuristic. This includes cases for the reference commits\n> having generation above and below the generation of the input commit,\n> but also having maximum generation below the generation of the input\n> commit.\n> \n> The fix itself is to swap min_generation with a max_generation in\n> repo_in_merge_bases_many().\n> \n> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Reported-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>     Fix in_merge_bases_many() with commit-graphs\n>     \n>     Johannes alerted me to the difficulties Srinidhi was having with \n>     in_merge_bases_many() and commit-graphs. Sorry that I hadn't seen that\n>     thread and the issues therein.\n>     \n>     After working with Johannes to investigate what was happening, we found\n>     a 2-year-old bug in the generation number checks!\n>     \n>     Thanks, -Stolee\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-739%2Fderrickstolee%2Fin-merge-bases-many-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-739/derrickstolee/in-merge-bases-many-fix-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/739\n> \n>  commit-reach.c        |  8 ++++----\n>  t/helper/test-reach.c |  2 ++\n>  t/t6600-test-reach.sh | 30 ++++++++++++++++++++++++++++++\n>  3 files changed, 36 insertions(+), 4 deletions(-)\n> \n> diff --git a/commit-reach.c b/commit-reach.c\n> index efd5925cbb..50175b159e 100644\n> --- a/commit-reach.c\n> +++ b/commit-reach.c\n> @@ -321,7 +321,7 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,\n>  {\n>  \tstruct commit_list *bases;\n>  \tint ret = 0, i;\n> -\tuint32_t generation, min_generation = GENERATION_NUMBER_INFINITY;\n> +\tuint32_t generation, max_generation = GENERATION_NUMBER_ZERO;\n>  \n>  \tif (repo_parse_commit(r, commit))\n>  \t\treturn ret;\n> @@ -330,12 +330,12 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,\n>  \t\t\treturn ret;\n>  \n>  \t\tgeneration = commit_graph_generation(reference[i]);\n> -\t\tif (generation < min_generation)\n> -\t\t\tmin_generation = generation;\n> +\t\tif (generation > max_generation)\n> +\t\t\tmax_generation = generation;\n>  \t}\n>  \n>  \tgeneration = commit_graph_generation(commit);\n> -\tif (generation > min_generation)\n> +\tif (generation > max_generation)\n>  \t\treturn ret;\n\nThis is correct; good catch.\n\n>  \tbases = paint_down_to_common(r, commit,\n> diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c\n> index 14a3655442..cda804ed79 100644\n> --- a/t/helper/test-reach.c\n> +++ b/t/helper/test-reach.c\n> @@ -107,6 +107,8 @@ int cmd__reach(int ac, const char **av)\n>  \t\tprintf(\"%s(A,B):%d\\n\", av[1], ref_newer(&oid_A, &oid_B));\n>  \telse if (!strcmp(av[1], \"in_merge_bases\"))\n>  \t\tprintf(\"%s(A,B):%d\\n\", av[1], in_merge_bases(A, B));\n> +\telse if (!strcmp(av[1], \"in_merge_bases_many\"))\n> +\t\tprintf(\"%s(A,X):%d\\n\", av[1], in_merge_bases_many(A, X_nr, X_array));\n>  \telse if (!strcmp(av[1], \"is_descendant_of\"))\n>  \t\tprintf(\"%s(A,X):%d\\n\", av[1], repo_is_descendant_of(r, A, X));\n>  \telse if (!strcmp(av[1], \"get_merge_bases_many\")) {\n> diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh\n> index 475564bee7..f807276337 100755\n> --- a/t/t6600-test-reach.sh\n> +++ b/t/t6600-test-reach.sh\n> @@ -110,6 +110,36 @@ test_expect_success 'in_merge_bases:miss' '\n>  \ttest_three_modes in_merge_bases\n>  '\n>  \n> +test_expect_success 'in_merge_bases_many:hit' '\n> +\tcat >input <<-\\EOF &&\n> +\tA:commit-6-8\n> +\tX:commit-6-9\n> +\tX:commit-5-7\n> +\tEOF\n> +\techo \"in_merge_bases_many(A,X):1\" >expect &&\n> +\ttest_three_modes in_merge_bases_many\n> +'\n> +\n> +test_expect_success 'in_merge_bases_many:miss' '\n> +\tcat >input <<-\\EOF &&\n> +\tA:commit-6-8\n> +\tX:commit-7-7\n> +\tX:commit-8-6\n> +\tEOF\n> +\techo \"in_merge_bases_many(A,X):0\" >expect &&\n> +\ttest_three_modes in_merge_bases_many\n> +'\n> +\n> +test_expect_success 'in_merge_bases_many:miss-heuristic' '\n> +\tcat >input <<-\\EOF &&\n> +\tA:commit-6-8\n> +\tX:commit-7-5\n> +\tX:commit-6-6\n> +\tEOF\n> +\techo \"in_merge_bases_many(A,X):0\" >expect &&\n> +\ttest_three_modes in_merge_bases_many\n> +'\n> +\n>  test_expect_success 'is_descendant_of:hit' '\n>  \tcat >input <<-\\EOF &&\n>  \tA:commit-5-7\n> \n> base-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc\n> -- \n> gitgitgadget\n\nThanks Derrick and Johannes, for identifying the problem\nand fixing the bug.\n\n-- \nSrinidhi Kaushik\n"},{"id":"406837","messageId":"xmqqr1qg8kfa.fsf@gitster.c.googlers.com","threadId":"54336","inReplyTo":"c6b163dd-d5f4-73a8-43ac-22460d1079ec@gmail.com","subject":"Re: [PATCH] commit-reach: fix in_merge_bases_many bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-02T20:03:37Z","receivedAt":"2020-10-02T20:03:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> Thanks for double-checking. I also think that dropping the\n> \"hide the error\" patch is prudent.\n\nThanks again for a quick and straight-forward fix.  As I mentioned\nelsewhere in the thread, it appears that we invented duplicate API\nwith parallel implementation in get_reachable_subset(), which seems\nto be a strict superset of in_merge_bases_many(), and that may be\nwhat led to an initial and incorrect \"get_reachable_subset() is not\nbroken the same way as in_merge_bases_many() so use it instead\"\nresponse.  I haven't paid attention to the quality of implementation\nof get_reachable_subset() as much as in_merge_bases_many() (e.g. I\nknow of an obvious way to optimize the latter) so far, but it would\nbe wonderful if we can eventually rewrite in_merge_bases_many() as a\nvery thin wrapper() around get_reachable_subset() without any\ndownside.\n\n"},{"id":"406839","messageId":"ac3ac80d-7f8e-6b16-03fa-1cd38b5d860b@gmail.com","threadId":"54336","inReplyTo":"xmqqr1qg8kfa.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] commit-reach: fix in_merge_bases_many bug","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-10-02T20:08:13Z","receivedAt":"2020-10-02T20:08:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/2/2020 4:03 PM, Junio C Hamano wrote:\n> Derrick Stolee <stolee@gmail.com> writes:\n> \n>> Thanks for double-checking. I also think that dropping the\n>> \"hide the error\" patch is prudent.\n> \n> Thanks again for a quick and straight-forward fix.  As I mentioned\n> elsewhere in the thread, it appears that we invented duplicate API\n> with parallel implementation in get_reachable_subset(), which seems\n> to be a strict superset of in_merge_bases_many(), and that may be\n> what led to an initial and incorrect \"get_reachable_subset() is not\n> broken the same way as in_merge_bases_many() so use it instead\"\n> response.  I haven't paid attention to the quality of implementation\n> of get_reachable_subset() as much as in_merge_bases_many() (e.g. I\n> know of an obvious way to optimize the latter) so far, but it would\n> be wonderful if we can eventually rewrite in_merge_bases_many() as a\n> very thin wrapper() around get_reachable_subset() without any\n> downside.\n\nI have self-assigned myself https://github.com/gitgitgadget/git/issues/740\nwhich will investigate these duplicates and see what can be done.\n\nI know that the initial refactoring focused on keeping the \"public\"\ninterface the same and just creating shims between different method\nprototypes. In this case, it might be worth doing a full replacement\nsince in_merge_bases_many() has so few callers.\n\nI'll be proactive to look for other cases that might be tricky for\nnew contributors to find the \"right\" way to do it.\n\nThanks,\n-Stolee\n\n"}]}