merge: use repo_in_merge_bases for octopus up-to-date check#2110
Open
spkrka wants to merge 1 commit intogitgitgadget:masterfrom
Open
merge: use repo_in_merge_bases for octopus up-to-date check#2110spkrka wants to merge 1 commit intogitgitgadget:masterfrom
spkrka wants to merge 1 commit intogitgitgadget:masterfrom
Conversation
The octopus merge path checks whether each remote head is already an ancestor of HEAD by computing all merge-bases via repo_get_merge_bases() and comparing the first result's OID to the remote head. This is more expensive than necessary: repo_get_merge_bases() calls paint_down_to_common() with min_generation=0, performs the full STALE drain, and may run remove_redundant(), when all we need is a yes/no reachability answer. Replace this with repo_in_merge_bases(), which answers the is-ancestor question directly. When generation numbers are available, repo_in_merge_bases() uses can_all_from_reach() -- a DFS bounded by generation number that stops as soon as the target is found or ruled out, without entering paint_down_to_common() at all. Without generation numbers, it still benefits from a tighter min_generation floor. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While reviewing callers of repo_get_merge_bases() for a different patch, I noticed the octopus up-to-date loop in builtin/merge.c computes full merge-bases only to check whether each remote head is an ancestor of HEAD.
The existing code calls repo_get_merge_bases(), takes the first result, frees the list, and compares the OID to the remote head. This is equivalent to an is-ancestor check, which repo_in_merge_bases() answers directly.
Using repo_in_merge_bases() simplifies the code (-14/+4 lines) and avoids unnecessary work: with generation numbers it uses can_all_from_reach() instead of paint_down_to_common(), and without generation numbers it still benefits from a tighter min_generation floor. In practice this only matters for octopus merges on repos with deep history, so the main value here is the simplification.
The comment "Here we have to calculate the individual merge_bases again" dates from 2008 (1c7b76b, "Build in merge"). At the time, in_merge_bases() was the same cost as computing merge bases. Stolee's 2018 generation number work (d7c1ec3) and the later switch to can_all_from_reach (6cc0174) made it significantly cheaper, but this call site was never updated.
CC: Derrick Stolee stolee@gmail.com