Repository navigation
speed up av sync on repos with many remote refs - #766
Conversation
Current Aviator status
This PR was merged using Aviator (commit d8ac806).
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the process of fetching and pruning merged branches. It replaces the broad ls-remote call with targeted fetching of individual PR refs, restricts the commit history traversal range when searching for merge commits by finding the fork point, and extends PR close-pattern matching to include commit subjects. The reviewer feedback highlights two important issues: first, git merge-base --octopus can return multiple merge bases, which would produce an invalid revision range and crash git log (suggesting the use of ^branch exclusion instead); second, ls-remote with patterns does support server-side filtering in Git protocol v2, so replacing it with sequential git fetch calls introduces a performance bottleneck that could be avoided by keeping a pattern-aware LsRemote implementation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| revisionRange := []string{"--max-count=10000", tip} | ||
| mergeBaseArgs := append([]string{"merge-base", "--octopus", tip}, branches...) | ||
| if base, err := vm.repo.Git(ctx, mergeBaseArgs...); err == nil { | ||
| if base == tip { | ||
| continue | ||
| } | ||
| revisionRange = []string{"--max-count=10000", base + ".." + tip} | ||
| } |
There was a problem hiding this comment.
Using git merge-base --octopus can return multiple merge bases separated by newlines if there is no single common ancestor. Since vm.repo.Git only trims leading/trailing whitespace, base will contain internal newlines (e.g., "sha1\nsha2"). Constructing base + ".." + tip will result in "sha1\nsha2..tip", which is an invalid revision range and will cause git log to fail with a fatal error.
Instead of using git merge-base to find a single fork point, we can leverage Git's native revision exclusion syntax. Passing ^branch for each branch to git log (e.g., git log tip ^branch1 ^branch2) will automatically exclude all commits reachable from those branches. This is extremely robust, handles multiple branches and complex histories perfectly, and completely eliminates the extra git merge-base process call.
revisionRange := []string{"--max-count=10000", tip}
for _, branch := range branches {
revisionRange = append(revisionRange, "^"+branch)
}| // FetchRemoteRef fetches the given fully-qualified ref from the remote and | ||
| // returns its commit SHA. This is intentionally not implemented with | ||
| // ls-remote: ls-remote patterns cannot filter the server-side ref | ||
| // advertisement (only --heads/--tags can), so on remotes with a very large | ||
| // number of refs (e.g. GitHub repositories with hundreds of thousands of | ||
| // refs/pull/* refs) it downloads the entire advertisement, whereas git fetch | ||
| // sends protocol v2 ref-prefix filters so that the server advertises only | ||
| // the requested ref. | ||
| func (r *Repo) FetchRemoteRef(ctx context.Context, remote string, ref string) (string, error) { | ||
| if _, err := r.Git(ctx, "fetch", "--no-tags", remote, ref); err != nil { | ||
| return "", err | ||
| } | ||
| return ret, nil | ||
| return r.Git(ctx, "rev-parse", "FETCH_HEAD") | ||
| } |
There was a problem hiding this comment.
The comment states that ls-remote patterns cannot filter the server-side ref advertisement. However, in Git protocol v2 (which is supported by GitHub and modern Git clients), git ls-remote with specific ref patterns actually uses the ls-refs command with ref-prefix filters. This performs server-side filtering and only advertises the requested refs, without downloading the entire advertisement or any packfiles/objects.
Instead of completely removing LsRemote and replacing it with sequential git fetch calls (which introduce a significant performance bottleneck), we should keep LsRemote but update it to accept optional patterns. This allows us to batch-query multiple refs in a single network round-trip.
// LsRemote lists references in a remote repository matching the given patterns.
// In Git protocol v2, passing specific ref patterns uses server-side ref-prefix
// filtering, making it extremely fast and efficient even on repositories with
// a very large number of refs.
func (r *Repo) LsRemote(ctx context.Context, remote string, refs ...string) (map[string]string, error) {
args := append([]string{"ls-remote", remote}, refs...)
out, err := r.Run(ctx, &RunOpts{
Args: args,
ExitError: true,
})
if err != nil {
return nil, errors.Errorf("failed to get remote branches: %v", err)
}
ret := make(map[string]string)
for _, line := range out.Lines() {
ss := strings.Split(line, "\t")
if len(ss) != 2 {
return nil, errors.Errorf("failed to parse the ls-remote output: %q", line)
}
ret[ss[1]] = ss[0]
}
return ret, nil
}
6a65a72 to
257b022
Compare
✅ FlexReview StatusCommon Owner:
Review SLO: |
jainankit
left a comment
jainankit
left a comment
There was a problem hiding this comment.
1 non-blocking suggestion
| } | ||
| remoteHash, ok := remoteBranches[fmt.Sprintf("refs/pull/%d/head", avbr.PullRequest.Number)] | ||
| if !ok { | ||
| remoteHash, err := vm.repo.FetchRemoteRef( |
There was a problem hiding this comment.
this fetch is called in a for-loop, you could batch it.
on large github repos the unfiltered ls-remote (365k+ refs on one customer repo) and the 10k-commit go-git trunk walk make even no-op syncs take ~30s+