Skip to content

[RB] Optimize git operations run by the CLI #11432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maggie-lou merged 2 commits into from
Mar 10, 2026
Merged

[RB] Optimize git operations run by the CLI #11432

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffc023c
[RB] Optimize git operations
maggie-lou Feb 26, 2026
954553c
PR feedback
maggie-lou Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 48 additions & 63 deletions cli/remotebazel/remotebazel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,18 +253,23 @@ func parseRemote(s string) (*gitRemote, error) {

}

// determineDefaultBranch parses `remoteData` (the output from `git ls-remote --symref origin`)
// determineDefaultBranch parses the output from `git ls-remote --symref origin`
// and returns the HEAD branch for the repo (often `main` or `master).
//
// We expect `remoteData` to contain a string looking like
// We expect the git data to contain a string looking like
// `ref: refs/heads/main HEAD`
// and this function would return `main`.
func determineDefaultBranch(remoteData string) (string, error) {
func determineDefaultBranch(remoteName string) (string, error) {
defaultBranch := os.Getenv("GIT_REPO_DEFAULT_BRANCH")
if defaultBranch != "" {
return defaultBranch, nil
}

remoteData, err := runGit("ls-remote", "--symref", remoteName)
if err != nil {
return "", status.WrapErrorf(err, "git ls-remote --symref %s", remoteName)
}

re := regexp.MustCompile(`ref: refs/heads/(\S+)\s+HEAD`)
match := re.FindStringSubmatch(remoteData)
if len(match) > 1 {
Expand Down Expand Up @@ -332,27 +337,14 @@ func Config() (*RepoConfig, error) {
fetchURL := remote.url
log.Debugf("Using fetch URL: %s", fetchURL)

// Fetching remote data from GitHub can be slow. If we don't need the data
// because the user has passed in enough information manually, skip the fetch.
shouldFetchBaseRef := *runFromBranch == "" && *runFromCommit == ""
shouldFetchDefaultBranch := os.Getenv("GIT_REPO_DEFAULT_BRANCH") == ""
shouldFetchRemote := shouldFetchBaseRef || shouldFetchDefaultBranch
var remoteData string
if shouldFetchRemote {
remoteData, err = runGit("ls-remote", "--symref", remote.name)
if err != nil {
return nil, status.WrapErrorf(err, "git remote show %s", remote.name)
}
}

branch, commit, err := getBaseBranchAndCommit(remoteData)
defaultBranch, err := determineDefaultBranch(remote.name)
if err != nil {
return nil, status.WrapError(err, "get base branch and commit")
return nil, status.WrapError(err, "get default branch")
}

defaultBranch, err := determineDefaultBranch(remoteData)
branch, commit, err := getBaseBranchAndCommit(remote.name, defaultBranch)
if err != nil {
log.Warnf("Failed to fetch default branch: %s", err)
return nil, status.WrapError(err, "get base branch and commit")
}
Comment on lines +340 to 348
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config() now always calls determineDefaultBranch(remote.name), which runs git ls-remote --symref (the slow command noted in the PR). This happens even when --run_from_branch / --run_from_commit are provided and getBaseBranchAndCommit would early-return, so it can negate the intended optimization and also makes default-branch lookup failures fatal. Consider resolving the default branch lazily only when it’s actually needed (e.g. when falling back to the default branch and/or when GIT_REPO_DEFAULT_BRANCH must be populated), and preserve the previous non-fatal behavior if default-branch detection fails.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@maggie-lou maggie-lou Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think it's worth it to always fetch the default branch, because we use it for fallback snapshot behavior. The performance wins of having a fallback snapshot probably outweigh the cost of the git operation


repoConfig := &RepoConfig{
Expand All @@ -373,11 +365,8 @@ func Config() (*RepoConfig, error) {
return repoConfig, nil
}

// getBaseBranchAndCommit returns the git branch and commit that the remote run
// should be based off
//
// remoteData is the output from `git remote show origin`
func getBaseBranchAndCommit(remoteData string) (branch string, commit string, err error) {
// getBaseBranchAndCommit returns the git branch and commit that should be fetched on the remote runner.
func getBaseBranchAndCommit(remoteName string, defaultBranch string) (branch string, commit string, err error) {
Copy link
Member

@bduffany bduffany Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The func comment is outdated now that remoteData is no longer a param

Also I'm a little confused what this means

// getBaseBranchAndCommit returns the git branch and commit that the remote run
// should be based off

I'm confused what it means for the remote run to be "based off" a commit - is it meant to be the merge-base commit between the default branch and the current branch tip?

Copy link
Collaborator Author

@maggie-lou maggie-lou Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment - LMK if that's clearer

branch = *runFromBranch
commit = *runFromCommit
if branch != "" || commit != "" {
Expand All @@ -389,20 +378,17 @@ func getBaseBranchAndCommit(remoteData string) (branch string, commit string, er
return "", "", fmt.Errorf("get current ref: %w", err)
}

currentBranchExistsRemotely := branchExistsRemotely(remoteData, currentBranch)
currentBranchExistsRemotely, err := branchTrackedRemotely(remoteName, currentBranch)
if err != nil {
log.Warnf("Failed to check if branch %s exists remotely. Falling back to running on default branch: %s", currentBranch, err)
Copy link
Member

@bduffany bduffany Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if we fail to check whether the remote branch exists, there's probably a deeper error going on (e.g. no network connection or GitHub is down?)

What do you think about just returning the error here instead?

Copy link
Collaborator Author

@maggie-lou maggie-lou Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can fail if there is no local tracking for the branch, for example if there's a shallow clone

}
if currentBranchExistsRemotely {
currentCommitHash, err := getHeadCommitForLocalBranch("HEAD")
if err != nil {
return "", "", status.WrapError(err, "get current commit hash")
}
currentCommitHash = strings.TrimSuffix(currentCommitHash, "\n")

remoteCommitOutput, err := runGit("branch", "-r", "--contains", currentCommitHash)
if err != nil {
return "", "", status.WrapError(err, fmt.Sprintf("check if commit %s exists remotely", currentCommitHash))
}
currentCommitExistsRemotely := strings.Contains(remoteCommitOutput, fmt.Sprintf("origin/%s", currentBranch))
currentCommitExistsRemotely := commitTrackedInRemoteBranch(remoteName, currentBranch, "HEAD")
if currentCommitExistsRemotely {
currentCommitHash, err := getHeadCommitForLocalBranch("HEAD")
if err != nil {
return "", "", status.WrapError(err, "get current commit hash")
}
branch = currentBranch
commit = currentCommitHash
}
Expand All @@ -412,10 +398,6 @@ func getBaseBranchAndCommit(remoteData string) (branch string, commit string, er
// not be able to fetch it. In this case, use the default branch for the repo.
// Your local changes will be applied as a patchset to the remote runner.
if branch == "" || commit == "" {
defaultBranch, err := determineDefaultBranch(remoteData)
if err != nil {
return "", "", status.WrapError(err, "get default branch")
}
branch = defaultBranch

defaultBranchCommitHash, err := getHeadCommitForLocalBranch(defaultBranch)
Expand Down Expand Up @@ -451,32 +433,35 @@ func getCurrentRef() (string, error) {
return strings.TrimSpace(matches[1]), nil
}

// branchExistsRemotely parses `remoteData` (the output from “git ls-remote --symref origin)
// and returns whether `branch` is tracked remotely.
// branchTrackedRemotely returns whether the given branch exists remotely, as reflected in
// the local git state.
//
// If the branch is tracked remotely, we expect `remoteData` to contain a string looking like
// `abc123 refs/heads/my_branch`
func branchExistsRemotely(remoteData string, branch string) bool {
regex := fmt.Sprintf("\\brefs/heads/%s\\b", regexp.QuoteMeta(branch))
re := regexp.MustCompile(regex)
return re.MatchString(remoteData)
// This will return false if there is a shallow clone and data for the requested branch
// was not fetched.
// This can be incorrect if the branch has been deleted remotely and the local
// git state hasn't been updated, though this case should be rare.
func branchTrackedRemotely(remoteName string, branch string) (bool, error) {
ref := fmt.Sprintf("refs/remotes/%s/%s", remoteName, branch)
_, err := runGit("show-ref", "--verify", ref)
if err != nil {
if strings.Contains(err.Error(), "not a valid ref") {
return false, nil
}
return false, status.WrapErrorf(err, "git show-ref --verify %s", ref)
}
return true, nil
}

// getHeadCommitForRemoteBranch parses `remoteData` (the output from “git ls-remote --symref origin)
// and returns the commit at HEAD for the remote branch.
// commitTrackedInRemoteBranch returns whether the given commit is tracked in the remote branch.
// It is used as a proxy for whether the commit exists remotely, and can be fetched on the remote runner.
//
// We expect `remoteData` to contain a string looking like
//
// `abc123 refs/heads/my_branch`
// and this function would return `abc123`.
func getHeadCommitForRemoteBranch(remoteData string, branch string) (string, error) {
regex := `\n(\S+)\s+refs/heads/` + branch + `\n`
re := regexp.MustCompile(regex)
match := re.FindStringSubmatch(remoteData)
if len(match) > 1 {
return match[1], nil
}
return "", status.NotFoundErrorf("failed to get HEAD commit for remote branch %s from:\n%s", branch, remoteData)
// This will return false if there is a shallow clone and data for the requested branch
// was not fetched.
// This can be incorrect if the branch has been deleted remotely and the local git state hasn't been updated, though this case should be rare.
func commitTrackedInRemoteBranch(remoteName, branch, commit string) bool {
remoteTrackingRef := fmt.Sprintf("refs/remotes/%s/%s", remoteName, branch)
_, err := runGit("merge-base", "--is-ancestor", commit, remoteTrackingRef)
return err == nil
}

// getHeadCommitForLocalBranch returns the commit at HEAD for the local branch.
Expand Down
Loading