{"thread":{"id":"65618","subject":"[PATCH] merge: use repo_in_merge_bases for octopus up-to-date check","startedAt":"2026-05-12T06:11:32Z","lastAt":"2026-05-18T14:39:03Z","messageCount":3,"participants":["Kristofer Karlsson via GitGitGadget","Derrick Stolee","Kristofer Karlsson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"543140","messageId":"pull.2110.git.1778566286543.gitgitgadget@gmail.com","threadId":"65618","inReplyTo":null,"subject":"[PATCH] merge: use repo_in_merge_bases for octopus up-to-date check","fromName":"Kristofer Karlsson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-05-12T06:11:26Z","receivedAt":"2026-05-12T06:11:32Z","isPatch":true,"body":"From: Kristofer Karlsson <krka@spotify.com>\n\nThe octopus merge path checks whether each remote head is already\nan ancestor of HEAD by computing all merge-bases via\nrepo_get_merge_bases() and comparing the first result's OID to\nthe remote head.  This is more expensive than necessary:\nrepo_get_merge_bases() calls paint_down_to_common() with\nmin_generation=0, performs the full STALE drain, and may run\nremove_redundant(), when all we need is a yes/no reachability\nanswer.\n\nReplace this with repo_in_merge_bases(), which answers the\nis-ancestor question directly.  When generation numbers are\navailable, repo_in_merge_bases() uses can_all_from_reach() -- a\nDFS bounded by generation number that stops as soon as the target\nis found or ruled out, without entering paint_down_to_common() at\nall.  Without generation numbers, it still benefits from a tighter\nmin_generation floor.\n\nSigned-off-by: Kristofer Karlsson <krka@spotify.com>\n---\n    merge: use repo_in_merge_bases for octopus up-to-date check\n    \n    While reviewing callers of repo_get_merge_bases() for a different patch,\n    I noticed the octopus up-to-date loop in builtin/merge.c computes full\n    merge-bases only to check whether each remote head is an ancestor of\n    HEAD.\n    \n    The existing code calls repo_get_merge_bases(), takes the first result,\n    frees the list, and compares the OID to the remote head. This is\n    equivalent to an is-ancestor check, which repo_in_merge_bases() answers\n    directly.\n    \n    Using repo_in_merge_bases() simplifies the code (-14/+4 lines) and\n    avoids unnecessary work: with generation numbers it uses\n    can_all_from_reach() instead of paint_down_to_common(), and without\n    generation numbers it still benefits from a tighter min_generation\n    floor. In practice this only matters for octopus merges on repos with\n    deep history, so the main value here is the simplification.\n    \n    The comment \"Here we have to calculate the individual merge_bases again\"\n    dates from 2008 (1c7b76be, \"Build in merge\"). At the time,\n    in_merge_bases() was the same cost as computing merge bases. Stolee's\n    2018 generation number work (d7c1ec3e) and the later switch to\n    can_all_from_reach (6cc01743) made it significantly cheaper, but this\n    call site was never updated.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2110%2Fspkrka%2Fmerge-octopus-in-merge-bases-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2110/spkrka/merge-octopus-in-merge-bases-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2110\n\n builtin/merge.c             | 18 ++++--------------\n t/t6408-merge-up-to-date.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 2cbce56f8d..862107cf41 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1735,21 +1735,11 @@ int cmd_merge(int argc,\n \t\tstruct commit_list *j;\n \n \t\tfor (j = remoteheads; j; j = j->next) {\n-\t\t\tstruct commit_list *common_one = NULL;\n-\t\t\tstruct commit *common_item;\n-\n-\t\t\t/*\n-\t\t\t * Here we *have* to calculate the individual\n-\t\t\t * merge_bases again, otherwise \"git merge HEAD^\n-\t\t\t * HEAD^^\" would be missed.\n-\t\t\t */\n-\t\t\tif (repo_get_merge_bases(the_repository, head_commit,\n-\t\t\t\t\t\t j->item, &common_one) < 0)\n+\t\t\tint ret = repo_in_merge_bases(the_repository,\n+\t\t\t\t\t\t      j->item, head_commit);\n+\t\t\tif (ret < 0)\n \t\t\t\texit(128);\n-\n-\t\t\tcommon_item = common_one->item;\n-\t\t\tcommit_list_free(common_one);\n-\t\t\tif (!oideq(&common_item->object.oid, &j->item->object.oid)) {\n+\t\t\tif (!ret) {\n \t\t\t\tup_to_date = 0;\n \t\t\t\tbreak;\n \t\t\t}\ndiff --git a/t/t6408-merge-up-to-date.sh b/t/t6408-merge-up-to-date.sh\nindex 7763c1ba98..be0840efb6 100755\n--- a/t/t6408-merge-up-to-date.sh\n+++ b/t/t6408-merge-up-to-date.sh\n@@ -89,4 +89,14 @@ test_expect_success 'merge fast-forward octopus' '\n \ttest \"$expect\" = \"$current\"\n '\n \n+test_expect_success 'merge octopus already up to date' '\n+\n+\tgit reset --hard c2 &&\n+\ttest_tick &&\n+\tgit merge c0 c1 &&\n+\texpect=$(git rev-parse c2) &&\n+\tcurrent=$(git rev-parse HEAD) &&\n+\ttest \"$expect\" = \"$current\"\n+'\n+\n test_done\n\nbase-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0\n-- \ngitgitgadget\n"},{"id":"543530","messageId":"c5b333f1-0db6-4aec-a369-6503cb924e7f@gmail.com","threadId":"65618","inReplyTo":"pull.2110.git.1778566286543.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge: use repo_in_merge_bases for octopus up-to-date check","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-05-18T12:20:44Z","receivedAt":"2026-05-18T12:20:46Z","isPatch":true,"body":"On 5/12/2026 2:11 AM, Kristofer Karlsson via GitGitGadget wrote:\n> From: Kristofer Karlsson <krka@spotify.com>\n> \n> The octopus merge path checks whether each remote head is already\n> an ancestor of HEAD by computing all merge-bases via\n> repo_get_merge_bases() and comparing the first result's OID to\n> the remote head.  This is more expensive than necessary:\n> repo_get_merge_bases() calls paint_down_to_common() with\n> min_generation=0, performs the full STALE drain, and may run\n> remove_redundant(), when all we need is a yes/no reachability\n> answer.\n> \n> Replace this with repo_in_merge_bases(), which answers the\n> is-ancestor question directly.  When generation numbers are\n> available, repo_in_merge_bases() uses can_all_from_reach() -- a\n> DFS bounded by generation number that stops as soon as the target\n> is found or ruled out, without entering paint_down_to_common() at\n> all.  Without generation numbers, it still benefits from a tighter\n> min_generation floor.\n> \n> Signed-off-by: Kristofer Karlsson <krka@spotify.com>\n> ---\n>     merge: use repo_in_merge_bases for octopus up-to-date check\n>     \n>     While reviewing callers of repo_get_merge_bases() for a different patch,\n>     I noticed the octopus up-to-date loop in builtin/merge.c computes full\n>     merge-bases only to check whether each remote head is an ancestor of\n>     HEAD.\n>     \n>     The existing code calls repo_get_merge_bases(), takes the first result,\n>     frees the list, and compares the OID to the remote head. This is\n>     equivalent to an is-ancestor check, which repo_in_merge_bases() answers\n>     directly.\n>     \n>     Using repo_in_merge_bases() simplifies the code (-14/+4 lines) and\n>     avoids unnecessary work: with generation numbers it uses\n>     can_all_from_reach() instead of paint_down_to_common(), and without\n>     generation numbers it still benefits from a tighter min_generation\n>     floor. In practice this only matters for octopus merges on repos with\n>     deep history, so the main value here is the simplification.\n\nThe code change looks right to me. Do you have any performance numbers\nto share? Or was this motivated mostly as an opportunity for code cleanup?\n\nThanks,\n-Stolee\n\n\n"},{"id":"543534","messageId":"CAL71e4NosWg_UwZ6fn0FuaTS89U6Sm9PWAx=gTjFMzMCsEOw6w@mail.gmail.com","threadId":"65618","inReplyTo":"c5b333f1-0db6-4aec-a369-6503cb924e7f@gmail.com","subject":"Re: [PATCH] merge: use repo_in_merge_bases for octopus up-to-date check","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-05-18T14:38:51Z","receivedAt":"2026-05-18T14:39:03Z","isPatch":true,"body":"Good question! No, this was intended only as a code cleanup and\nsemantic simplification.\nThe code path seems like an edge case, so benchmarking it did not seem\nworthwhile.\nThe main win as I see it is clearer semantics for what it's doing and\nthe optimization is just a bonus.\n\nChecking if repo_get_merge_bases(HEAD, J) == J is better expressed as\nrepo_in_merge_bases(J, HEAD)\n\nrepo_in_merge_bases should probably be renamed to something like\nrepo_is_ancestor, since its comment says:\nIs \"commit\" an ancestor of (i.e. reachable from) the \"reference\"?\nbut I can understand that it may be painful to rename it in practice.\n\nIf I do a mental rename, this becomes simpler to reason about:\n\nChecking if repo_get_merge_bases(HEAD, J) == J is better expressed as\nrepo_is_ancestor(J, HEAD)\n\n- Kristofer\n\nOn Mon, 18 May 2026 at 14:20, Derrick Stolee <stolee@gmail.com> wrote:\n>\n> On 5/12/2026 2:11 AM, Kristofer Karlsson via GitGitGadget wrote:\n> > From: Kristofer Karlsson <krka@spotify.com>\n> >\n> > The octopus merge path checks whether each remote head is already\n> > an ancestor of HEAD by computing all merge-bases via\n> > repo_get_merge_bases() and comparing the first result's OID to\n> > the remote head.  This is more expensive than necessary:\n> > repo_get_merge_bases() calls paint_down_to_common() with\n> > min_generation=0, performs the full STALE drain, and may run\n> > remove_redundant(), when all we need is a yes/no reachability\n> > answer.\n> >\n> > Replace this with repo_in_merge_bases(), which answers the\n> > is-ancestor question directly.  When generation numbers are\n> > available, repo_in_merge_bases() uses can_all_from_reach() -- a\n> > DFS bounded by generation number that stops as soon as the target\n> > is found or ruled out, without entering paint_down_to_common() at\n> > all.  Without generation numbers, it still benefits from a tighter\n> > min_generation floor.\n> >\n> > Signed-off-by: Kristofer Karlsson <krka@spotify.com>\n> > ---\n> >     merge: use repo_in_merge_bases for octopus up-to-date check\n> >\n> >     While reviewing callers of repo_get_merge_bases() for a different patch,\n> >     I noticed the octopus up-to-date loop in builtin/merge.c computes full\n> >     merge-bases only to check whether each remote head is an ancestor of\n> >     HEAD.\n> >\n> >     The existing code calls repo_get_merge_bases(), takes the first result,\n> >     frees the list, and compares the OID to the remote head. This is\n> >     equivalent to an is-ancestor check, which repo_in_merge_bases() answers\n> >     directly.\n> >\n> >     Using repo_in_merge_bases() simplifies the code (-14/+4 lines) and\n> >     avoids unnecessary work: with generation numbers it uses\n> >     can_all_from_reach() instead of paint_down_to_common(), and without\n> >     generation numbers it still benefits from a tighter min_generation\n> >     floor. In practice this only matters for octopus merges on repos with\n> >     deep history, so the main value here is the simplification.\n>\n> The code change looks right to me. Do you have any performance numbers\n> to share? Or was this motivated mostly as an opportunity for code cleanup?\n>\n> Thanks,\n> -Stolee\n>\n>\n"}]}