-
Notifications
You must be signed in to change notification settings - Fork 14
feat: optimize detached HEAD merge base for jj users #470
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -309,10 +309,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)) | ||
|
|
@@ -321,6 +319,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 | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -340,6 +343,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) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unchecked
|
||
|
|
||
| if bestDist < 0 || dist < bestDist { | ||
| bestDist = dist | ||
| bestRef = ref | ||
| bestSHA = refSHA | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spawns O(n) git processes for every remote refMedium Severity
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 { | ||
|
|
||


There was a problem hiding this comment.
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.Sscanfcan fail to parse the count output, but the error is ignored. If parsing fails,distremains 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:
Spotted by Graphite

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