Skip to content

Fix bundle transport in shallow checkouts #31346

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
pelikhan merged 3 commits into from
May 10, 2026
+45 −3
Merged

Fix bundle transport in shallow checkouts #31346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d0ac5c
Initial plan
Copilot May 10, 2026
94a4785
Fix bundle transport in shallow checkouts
Copilot May 10, 2026
8d44ce7
Address bundle transport review comments
Copilot May 10, 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
14 changes: 11 additions & 3 deletions actions/setup/js/push_to_pull_request_branch.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,9 +603,17 @@ async function main(config = {}) {
await exec.exec("git", ["fetch", bundleFilePath, `refs/heads/${message.branch}:${bundleRef}`], baseGitOpts);
core.info(`Fetched bundle to ${bundleRef}`);

// Fast-forward the current branch to the bundle tip
await exec.exec("git", ["merge", "--ff-only", bundleRef], baseGitOpts);
core.info("Fast-forwarded branch to bundle tip");
// Point the checked-out branch at the bundle tip directly. In shallow
// checkouts, merge --ff-only can fail to discover the ancestry even
// when the bundle tip is based on the current branch tip and the
// prerequisite exists locally.
Comment on lines +607 to +609
const updateRefArgs = ["update-ref", `refs/heads/${branchName}`, bundleRef];
if (remoteHeadBeforePatch) {
updateRefArgs.push(remoteHeadBeforePatch);
}
await exec.exec("git", updateRefArgs, baseGitOpts);
await exec.exec("git", ["reset", "--hard"], baseGitOpts);
core.info("Updated branch to bundle tip");

// Clean up the temporary ref
try {
Expand Down
34 changes: 34 additions & 0 deletions actions/setup/js/push_to_pull_request_branch.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,6 +1199,40 @@ index 0000000..abc1234
});
});

// ──────────────────────────────────────────────────────
// Bundle Transport Application
// ──────────────────────────────────────────────────────

describe("bundle transport application", () => {
it("should apply bundle transport by updating the branch ref instead of merging", async () => {
const bundlePath = path.join(tempDir, "test.bundle");
const patchPath = createPatchFile("small patch content");
fs.writeFileSync(bundlePath, "bundle content");

const pushSignedCommitsModule = require("./push_signed_commits.cjs");
const pushSignedSpy = vi.spyOn(pushSignedCommitsModule, "pushSignedCommits").mockResolvedValue("bundle-tip");
Comment on lines +1207 to +1213

try {
mockExec.getExecOutput
.mockResolvedValueOnce({ exitCode: 0, stdout: "remote-head\trefs/heads/feature-branch\n", stderr: "" }) // preflight ls-remote
.mockResolvedValueOnce({ exitCode: 0, stdout: "remote-head\n", stderr: "" }) // rev-parse HEAD before bundle
.mockResolvedValueOnce({ exitCode: 0, stdout: "2\n", stderr: "" }); // rev-list --count
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 10, 2026

Choose a reason for hiding this comment

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

[/tdd] The if (remoteHeadBeforePatch) guard means update-ref can be called without the old-value safety argument, but this code path is untested — the new test always exercises the non-empty case.

Consider adding a complementary case:

it("should update branch ref without old-value guard when remoteHeadBeforePatch is absent", async () => {
  mockExec.getExecOutput
    .mockResolvedValueOnce({ exitCode: 0, stdout: "\trefs/heads/feature-branch\n", stderr: "" }) // ls-remote (no SHA)
    .mockResolvedValueOnce({ exitCode: 0, stdout: "", stderr: "" })                              // rev-parse: empty
    .mockResolvedValueOnce({ exitCode: 0, stdout: "2\n", stderr: "" });                          // rev-list count

  // ... exercise handler ...

  expect(mockExec.exec).toHaveBeenCalledWith(
    "git",
    ["update-ref", "refs/heads/feature-branch", "refs/bundles/push-feature-branch"],
    expect.any(Object)
  );
  // No 4th arg: intentional, but documents the safety trade-off
});

Without this, the branch that skips the old-value guard is invisible to the test suite, and the intentional trade-off (ref can be force-updated if remote has advanced concurrently) goes undocumented.


const module = await loadModule();
const handler = await module.main({});
const result = await handler({ branch: "feature-branch", patch_path: patchPath, bundle_path: bundlePath, diff_size: 5 * 1024 }, {});

expect(result.success).toBe(true);
expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", bundlePath, "refs/heads/feature-branch:refs/bundles/push-feature-branch"], expect.any(Object));
expect(mockExec.exec).toHaveBeenCalledWith("git", ["update-ref", "refs/heads/feature-branch", "refs/bundles/push-feature-branch", "remote-head"], expect.any(Object));
expect(mockExec.exec).toHaveBeenCalledWith("git", ["reset", "--hard"], expect.any(Object));
expect(mockExec.exec).not.toHaveBeenCalledWith("git", ["merge", "--ff-only", "refs/bundles/push-feature-branch"], expect.any(Object));
} finally {
pushSignedSpy.mockRestore();
}
});
});

// ──────────────────────────────────────────────────────
// Title Prefix and Labels Validation
// ──────────────────────────────────────────────────────
Expand Down
Loading