-
Notifications
You must be signed in to change notification settings - Fork 990
fix: solve #2516 — push/fetch respects branch tracking remote instead of hardcoding origin #2517
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 all commits
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 |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { isUpstreamMissingError } from "./git-utils"; | ||
| import { parseUpstreamRef } from "./utils/pull-request-url"; | ||
|
|
||
| describe("git-operations error handling", () => { | ||
| describe("isUpstreamMissingError", () => { | ||
|
|
@@ -91,3 +92,90 @@ describe("sync operation logic", () => { | |
| expect(isUpstreamMissingError(pullError.message)).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("tracking remote resolution (#2516)", () => { | ||
| // Reproduces the bug where push/fetch always targeted "origin" instead of | ||
| // the branch's actual tracking remote (e.g. a fork remote added by `gh pr checkout`). | ||
|
|
||
| test("parseUpstreamRef extracts fork remote name from upstream ref", () => { | ||
| // When `gh pr checkout` sets up a fork, the upstream looks like "contributor-fork/feature-branch" | ||
| const result = parseUpstreamRef("contributor-fork/feature-branch"); | ||
| expect(result).toEqual({ | ||
| remoteName: "contributor-fork", | ||
| branchName: "feature-branch", | ||
| }); | ||
| }); | ||
|
|
||
| test("parseUpstreamRef extracts origin remote name", () => { | ||
| const result = parseUpstreamRef("origin/main"); | ||
| expect(result).toEqual({ | ||
| remoteName: "origin", | ||
| branchName: "main", | ||
| }); | ||
| }); | ||
|
|
||
| test("parseUpstreamRef returns null for invalid refs", () => { | ||
| expect(parseUpstreamRef("")).toBeNull(); | ||
| expect(parseUpstreamRef("no-slash")).toBeNull(); | ||
| expect(parseUpstreamRef("/leading-slash")).toBeNull(); | ||
| expect(parseUpstreamRef("trailing/")).toBeNull(); | ||
| }); | ||
|
|
||
| test("getTrackingRemote logic: returns fork remote when tracking fork upstream", () => { | ||
| // This tests the core logic that was broken before the fix. | ||
| // The getTrackingRemote function uses parseUpstreamRef to extract the remote. | ||
| // Before the fix, push/fetch always hardcoded "origin" regardless of tracking. | ||
| const upstreamRef = "my-fork-remote/fix-typo"; | ||
| const parsed = parseUpstreamRef(upstreamRef); | ||
|
|
||
| // Before fix: would always use "origin" — ignoring the parsed remote | ||
| // After fix: uses parsed.remoteName ("my-fork-remote") | ||
| expect(parsed).not.toBeNull(); | ||
| expect(parsed?.remoteName).toBe("my-fork-remote"); | ||
| expect(parsed?.remoteName).not.toBe("origin"); | ||
| }); | ||
|
|
||
| test("getTrackingRemote logic: falls back to origin when no upstream is set", () => { | ||
| // When parseUpstreamRef returns null (no upstream configured), | ||
| // getTrackingRemote should fall back to "origin" | ||
| const parsed = parseUpstreamRef(""); | ||
| expect(parsed).toBeNull(); | ||
| // Fallback behavior: when parsed is null, getTrackingRemote returns "origin" | ||
| }); | ||
|
|
||
| test("push commands should use tracking remote, not hardcoded origin", () => { | ||
| // Verify the push args construction uses the resolved remote | ||
| const upstreamRef = "contributor/feature-branch"; | ||
| const parsed = parseUpstreamRef(upstreamRef); | ||
| expect(parsed).not.toBeNull(); | ||
|
|
||
| const remote = parsed?.remoteName; | ||
| const branch = "feature-branch"; | ||
|
|
||
| // The push command should use the tracking remote | ||
| const pushArgs = ["--set-upstream", remote, `HEAD:refs/heads/${branch}`]; | ||
|
Contributor
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. P2: These tests are tautological: they build expected push/fetch args inline instead of calling the actual git-operation helpers, so they don’t verify the bug fix path. Prompt for AI agents |
||
|
|
||
| expect(pushArgs).toEqual([ | ||
| "--set-upstream", | ||
| "contributor", | ||
| "HEAD:refs/heads/feature-branch", | ||
| ]); | ||
| // Before fix, pushArgs[1] would always be "origin" | ||
| expect(pushArgs[1]).not.toBe("origin"); | ||
| }); | ||
|
|
||
| test("fetch commands should use tracking remote, not hardcoded origin", () => { | ||
| // Verify fetch uses resolved remote | ||
| const upstreamRef = "fork-user/my-branch"; | ||
| const parsed = parseUpstreamRef(upstreamRef); | ||
| expect(parsed).not.toBeNull(); | ||
|
|
||
| const remote = parsed?.remoteName; | ||
| const branch = "my-branch"; | ||
|
|
||
| const fetchArgs = [remote, branch]; | ||
| expect(fetchArgs).toEqual(["fork-user", "my-branch"]); | ||
| // Before fix, fetchArgs[0] would always be "origin" | ||
| expect(fetchArgs[0]).not.toBe("origin"); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,15 +31,31 @@ async function hasUpstreamBranch(git: SimpleGit): Promise<boolean> { | |
| } | ||
| } | ||
|
|
||
| async function getTrackingRemote(git: SimpleGit): Promise<string> { | ||
| try { | ||
| const upstream = ( | ||
| await git.raw(["rev-parse", "--abbrev-ref", "@{upstream}"]) | ||
| ).trim(); | ||
| const parsed = parseUpstreamRef(upstream); | ||
| if (parsed) { | ||
| return parsed.remoteName; | ||
| } | ||
| } catch { | ||
|
Contributor
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. P2: Do not swallow all errors when resolving tracking remote; unexpected git failures are silently converted to (Based on your team's feedback about avoiding empty catch blocks that hide failures.) Prompt for AI agents |
||
| // No upstream configured, fall back to origin | ||
| } | ||
| return "origin"; | ||
| } | ||
|
|
||
| async function fetchCurrentBranch(git: SimpleGit): Promise<void> { | ||
| const branch = (await git.revparse(["--abbrev-ref", "HEAD"])).trim(); | ||
| const remote = await getTrackingRemote(git); | ||
| try { | ||
| await git.fetch(["origin", branch]); | ||
| await git.fetch([remote, branch]); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| if (isUpstreamMissingError(message)) { | ||
| try { | ||
| await git.fetch(["origin"]); | ||
| await git.fetch([remote]); | ||
| } catch (fallbackError) { | ||
| const fallbackMessage = | ||
| fallbackError instanceof Error | ||
|
|
@@ -62,9 +78,11 @@ async function fetchCurrentBranch(git: SimpleGit): Promise<void> { | |
| async function pushWithSetUpstream({ | ||
| git, | ||
| branch, | ||
| remote, | ||
| }: { | ||
| git: SimpleGit; | ||
| branch: string; | ||
| remote?: string; | ||
| }): Promise<void> { | ||
| const trimmedBranch = branch.trim(); | ||
| if (!trimmedBranch || trimmedBranch === "HEAD") { | ||
|
|
@@ -75,11 +93,13 @@ async function pushWithSetUpstream({ | |
| }); | ||
| } | ||
|
|
||
| const targetRemote = remote ?? (await getTrackingRemote(git)); | ||
|
|
||
| // Use HEAD refspec to avoid resolving the branch name as a local ref. | ||
| // This is more reliable for worktrees where upstream tracking isn't set yet. | ||
| await git.push([ | ||
| "--set-upstream", | ||
| "origin", | ||
| targetRemote, | ||
| `HEAD:refs/heads/${trimmedBranch}`, | ||
| ]); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
P2: The fallback test does not assert that the resolved remote is "origin", so it cannot catch regressions in fallback behavior.
Prompt for AI agents