commit-reach: remove get_reachable_subset()#2141
Open
spkrka wants to merge 1 commit into
Open
Conversation
|
There is an issue in commit 538378a:
|
spkrka
force-pushed
the
krka/remove-get-reachable-subset
branch
from
538378a to
e327567
Compare
Author
|
/cc @derrickstolee |
https://gitgitgadget.github.io/``` |
get_reachable_subset() and tips_reachable_from_bases() answer the same question: given a set of bases and a set of tips, which tips are reachable from at least one base? get_reachable_subset() was introduced in fcb2c07 (2018-11-02) for add_missing_tags() in remote.c. tips_reachable_from_bases() was added in cbfe360 (2023-03-20) as part of the ahead-behind series. The two were never consolidated. With a commit-graph, tips_reachable_from_bases() has the edge: its DFS raises the generation floor as lower targets are found, pruning more aggressively than the static floor in get_reachable_subset(). Without generation numbers, some edge cases may be slower with DFS instead of BFS since the date-ordered prio_queue naturally stays near the top of the graph, but this should not matter in practice -- worst case both visit the full graph down from the bases. Remove get_reachable_subset() and convert its only caller (add_missing_tags in remote.c) to use tips_reachable_from_bases() directly. The flag in remote.c changes from 1 (bit 0) to TMP_MARK (bit 4) because tips_reachable_from_bases() uses SEEN (bit 0) internally. TMP_MARK is already used for deduplication earlier in the same function and is cleared before the reachability check. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
spkrka
force-pushed
the
krka/remove-get-reachable-subset
branch
from
e327567 to
cc940c7
Compare
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
2 participants
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.
commit-reach: remove get_reachable_subset()
I have investigated what core graph algorithms exist and how
they are implemented and found that there is some overlap.
get_reachable_subset() and tips_reachable_from_bases() answer
the same question: which commits in a target set are reachable
from a set of base commits.
This patch converts the two callers of get_reachable_subset()
to use tips_reachable_from_bases() and deletes the now-unused
function.
tips_reachable_from_bases() has a minor algorithmic edge: it sorts
targets by generation and dynamically raises the pruning floor as
targets are found. In practice this makes no measurable difference
where the target set is typically small. Without a commit-graph, both
degrade identically -- all generation numbers are INFINITY, so
neither can prune. The main value is removing ~70 lines of code.
A few design choices I'd appreciate feedback on:
add_missing_tags() converts its sent_tips array to a
commit_list to match the tips_reachable_from_bases() API.
This is O(n) list-node allocations, negligible compared to
the graph walk. An array overload could avoid this, but
didn't seem worth adding for a single call site.
The flag changes from 1 (bit 0) to TMP_MARK (bit 4) because
tips_reachable_from_bases() uses SEEN (bit 0) internally.
TMP_MARK is already used for deduplication earlier in the
same function and is cleared before the reachability block.
I kept this as a single commit since the change is small.
Happy to split into convert-callers + delete-function if
that's preferred.
The test helper and test names are renamed from
get_reachable_subset to tips_reachable_from_bases to match
the function being exercised. test_all_modes already covers
both with and without commit-graph.