Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
62 changes: 60 additions & 2 deletions pkg/cmd/ci/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,10 +358,8 @@ func findMergeBase(workflowDir string) (baseBranch string, mergeBase string, err
branch := strings.TrimSpace(string(branchOut))
if branch != "" && branch != "HEAD" { // not detached
remoteBranch := "origin/" + branch
// Verify the remote branch exists
_, err := exec.Command("git", "-C", workflowDir, "rev-parse", "--verify", remoteBranch).Output()
if err == nil {
// Use merge-base to find common ancestor
shaOut, err := exec.Command("git", "-C", workflowDir, "merge-base", "HEAD", remoteBranch).Output()
if err == nil {
sha := strings.TrimSpace(string(shaOut))
Expand All @@ -370,6 +368,11 @@ func findMergeBase(workflowDir string) (baseBranch string, mergeBase string, err
}
}
}
} else if branch == "HEAD" {
// Detached HEAD: find the closest remote tracking ancestor
if ref, sha, err := findClosestRemoteAncestor(workflowDir); err == nil {
return ref, sha, nil
}
}
}

Expand All @@ -389,6 +392,61 @@ func findMergeBase(workflowDir string) (baseBranch string, mergeBase string, err
return defaultBranch, strings.TrimSpace(string(mergeBaseOut)), nil
}

// findClosestRemoteAncestor walks remote tracking refs to find the one
// closest to HEAD (fewest commits between it and HEAD). This produces
// smaller patches for detached-HEAD workflows like jj.
func findClosestRemoteAncestor(workflowDir string) (refName string, sha string, err error) {
refsOut, err := exec.Command("git", "-C", workflowDir,
"for-each-ref", "refs/remotes/origin/", "--format=%(objectname) %(refname:short)").Output()
if err != nil {
return "", "", err
}

bestRef := ""
bestSHA := ""
bestDist := -1

for _, line := range strings.Split(strings.TrimSpace(string(refsOut)), "\n") {
parts := strings.SplitN(line, " ", 2)
if len(parts) != 2 {
continue
}
refSHA, ref := parts[0], parts[1]

// Skip origin/HEAD (symbolic ref, not a real branch)
if ref == "origin/HEAD" {
continue
}

// Check if this ref is an ancestor of HEAD
err := exec.Command("git", "-C", workflowDir, "merge-base", "--is-ancestor", refSHA, "HEAD").Run()
if err != nil {
continue
}

// Count commits between this ref and HEAD
countOut, err := exec.Command("git", "-C", workflowDir, "rev-list", "--count", refSHA+"..HEAD").Output()
if err != nil {
continue
}

dist := 0
fmt.Sscanf(strings.TrimSpace(string(countOut)), "%d", &dist)
Comment on lines +433 to +434
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unchecked Sscanf error could cause incorrect distance calculation

fmt.Sscanf can fail to parse the count output, but the error is ignored. If parsing fails, dist remains 0, which could incorrectly make this ref appear to be at distance 0 from HEAD. This would cause the algorithm to select the wrong ref as the "closest" ancestor.

Fix: Check the Sscanf error and skip refs with parse failures:

n, err := fmt.Sscanf(strings.TrimSpace(string(countOut)), "%d", &dist)
if err != nil || n != 1 {
    continue
}
Suggested change
dist := 0
fmt.Sscanf(strings.TrimSpace(string(countOut)), "%d", &dist)
dist := 0
n, err := fmt.Sscanf(strings.TrimSpace(string(countOut)), "%d", &dist)
if err != nil || n != 1 {
continue
}

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unchecked Sscanf silently defaults distance to zero

Low Severity

fmt.Sscanf return value is ignored. If parsing fails for any reason, dist stays at its initialized value of 0, which is the minimum possible distance. This would cause the function to select that ref as the "closest" ancestor, even though the actual distance is unknown and could be much larger, resulting in an incorrect merge base selection.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4e171d0. Configure here.


if bestDist < 0 || dist < bestDist {
bestDist = dist
bestRef = ref
bestSHA = refSHA
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Spawns O(n) git processes for every remote ref

Medium Severity

findClosestRemoteAncestor spawns two git processes (merge-base --is-ancestor and rev-list --count) per remote tracking ref. For repositories with hundreds or thousands of remote branches under refs/remotes/origin/, this results in thousands of process forks, potentially causing multi-second or even multi-minute delays in a CLI tool that users run interactively. A more efficient approach (e.g., walking commits from HEAD and checking against a set of known ref SHAs) would be O(distance) rather than O(number-of-refs).

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4e171d0. Configure here.


if bestRef == "" {
return "", "", fmt.Errorf("no remote ancestor found")
}

return bestRef, bestSHA, nil
}

func detectPatch(workflowDir string) *patchInfo {
baseBranch, mergeBase, err := findMergeBase(workflowDir)
if err != nil {
Expand Down
42 changes: 42 additions & 0 deletions pkg/cmd/ci/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,48 @@ func TestFindMergeBase_DetachedHEAD(t *testing.T) {
_ = baseBranch
}

func TestFindMergeBase_DetachedHEAD_WithPushedAncestor(t *testing.T) {
bare := initBareRemote(t)
clone := cloneRepo(t, bare)

// Create a feature branch and push it
run(t, clone, "git", "checkout", "-b", "feature/jj-test")
writeFile(t, filepath.Join(clone, "feature.txt"), "pushed work")
run(t, clone, "git", "add", ".")
run(t, clone, "git", "commit", "-m", "pushed feature commit")
run(t, clone, "git", "push", "-u", "origin", "feature/jj-test")

pushedSHA := run(t, clone, "git", "rev-parse", "HEAD")

// Add two local-only commits (simulating jj workflow)
writeFile(t, filepath.Join(clone, "local1.txt"), "local1")
run(t, clone, "git", "add", ".")
run(t, clone, "git", "commit", "-m", "local commit 1")

writeFile(t, filepath.Join(clone, "local2.txt"), "local2")
run(t, clone, "git", "add", ".")
run(t, clone, "git", "commit", "-m", "local commit 2")

// Detach HEAD (standard jj workflow)
headSHA := run(t, clone, "git", "rev-parse", "HEAD")
run(t, clone, "git", "checkout", headSHA)

baseBranch, mergeBase, err := findMergeBase(clone)
if err != nil {
t.Fatalf("findMergeBase failed: %v", err)
}

// Should find the pushed feature branch as the closest ancestor,
// NOT fall back to origin/main (which would produce a bigger patch)
if mergeBase != pushedSHA {
t.Errorf("expected mergeBase=%s (pushed feature commit), got %s", pushedSHA, mergeBase)
}

if baseBranch != "origin/feature/jj-test" {
t.Errorf("expected baseBranch=origin/feature/jj-test, got %q", baseBranch)
}
}

func TestValidateWorkspacePatch_emptyMergeBase(t *testing.T) {
err := validateWorkspacePatch(&patchInfo{mergeBase: "", content: "x"})
if err == nil {
Expand Down
Loading