-
Notifications
You must be signed in to change notification settings - Fork 540
[WIP] Fix checkout PR branch fetch depth issue #50378
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
db21f29
87c39e4
85fbf4e
f78b1d2
c264923
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 |
|---|---|---|
|
|
@@ -26,6 +26,8 @@ describe("checkout_pr_branch.cjs", () => { | |
| // Mock exec | ||
| mockExec = { | ||
| exec: vi.fn().mockResolvedValue(0), | ||
| // Default: repository is shallow, so --depth is preserved | ||
| getExecOutput: vi.fn().mockResolvedValue({ stdout: "true\n", stderr: "", exitCode: 0 }), | ||
| }; | ||
|
|
||
| // Mock context | ||
|
|
@@ -238,6 +240,28 @@ If the pull request is still open, verify that: | |
| expect(mockCore.setFailed).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should omit --depth when the repository is not shallow", async () => { | ||
| mockExec.getExecOutput.mockResolvedValue({ stdout: "false\n", stderr: "", exitCode: 0 }); | ||
|
|
||
| const script = require("./checkout_pr_branch.cjs"); | ||
| await script.main(); | ||
|
|
||
| expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", "origin", "feature-branch"]); | ||
| expect(mockExec.exec).not.toHaveBeenCalledWith("git", ["fetch", "origin", "feature-branch", "--depth=2"]); | ||
| expect(mockCore.setFailed).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should omit --depth for refs/pull fetch when the repository is not shallow", async () => { | ||
| mockExec.getExecOutput.mockResolvedValue({ stdout: "false\n", stderr: "", exitCode: 0 }); | ||
|
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. [/tdd] The second test modifies 💡 Minimal fixAt the top of the const originalEvent = mockContext.eventName;
mockContext.eventName = 'pull_request_target';
// ... assertions ...
mockContext.eventName = originalEvent;Or confirm @copilot please address this. |
||
| mockContext.eventName = "pull_request_target"; | ||
|
|
||
| const script = require("./checkout_pr_branch.cjs"); | ||
| await script.main(); | ||
|
|
||
| expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", "origin", "+refs/pull/123/head:refs/remotes/origin/pr-head"]); | ||
| expect(mockCore.setFailed).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| describe("runtime checkout safety assertions", () => { | ||
| it("should fail when runtime repository context is a fork", async () => { | ||
| mockContext.payload.repository.fork = true; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -251,7 +251,21 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { | |
| } | ||
|
|
||
| if (defaultBranchRef) { | ||
| baseRef = execGitSync(["merge-base", "--", defaultBranchRef, tipRef], { cwd }).trim(); | ||
| try { | ||
| baseRef = execGitSync(["merge-base", "--", defaultBranchRef, tipRef], { cwd }).trim(); | ||
| } catch (mergeBaseError) { | ||
| // A shallow clone (or a `--depth` fetch that grafted history onto an | ||
| // otherwise complete clone) can make the merge-base unreachable. | ||
| // Surface that explicitly instead of the misleading "branch does not | ||
| // exist locally" message. | ||
| if (fs.existsSync(path.join(cwd || process.cwd(), ".git", "shallow"))) { | ||
|
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. [/diagnosing-bugs] Shallow detection here uses 💡 Suggested fixExtract const shallow = await isShallowRepository({ cwd });
if (shallow) {
throw new Error(`${ERR_SYSTEM}: Could not compute merge-base ... shallow clone.`);
}
throw mergeBaseError;This is also more testable — no real @copilot please address this. |
||
| throw new Error( | ||
| `${ERR_SYSTEM}: Could not compute merge-base between ${defaultBranchRef} and ${tipRef} because the repository is a shallow clone (.git/shallow exists). ` + | ||
| "Deepen the clone (checkout.fetch-depth: 0) so the common ancestor is reachable." | ||
| ); | ||
|
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. [/tdd] No test covers the new 💡 Suggested test shapeIn it('reports shallow-clone error when merge-base fails and .git/shallow exists', async () => {
mockExecGitSync.mockImplementationOnce((args) => {
if (args[0] === 'merge-base') throw new Error('fatal: no merge base');
});
mockFsExistsSync.mockReturnValue(true); // simulate .git/shallow present
await expect(generateGitPatch('branch', 'main', { mode: 'full' }))
.rejects.toThrow(/shallow clone/);
});@copilot please address this. |
||
| } | ||
| throw mergeBaseError; | ||
|
Comment on lines
+261
to
+267
|
||
| } | ||
| debugLog(`Strategy 1 (full): Computed merge-base: ${baseRef}`); | ||
| } else { | ||
| // No remote refs available - fall through to Strategy 2 | ||
|
|
@@ -309,6 +323,16 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { | |
| } catch (branchError) { | ||
| // Branch does not exist locally (or pinnedSha failed) | ||
| debugLog(`Strategy 1: Branch '${branchName}' does not exist locally - ${getErrorMessage(branchError)}`); | ||
| // Shallow-clone diagnostics (ERR_SYSTEM errors thrown from the merge-base | ||
| // block above) must reach callers immediately — falling through to Strategy 2 | ||
| // or 3 would produce a misleading "No changes to commit" result instead. | ||
| if (getErrorMessage(branchError).startsWith(ERR_SYSTEM)) { | ||
| return { | ||
| success: false, | ||
| error: getErrorMessage(branchError), | ||
| patchPath: patchPath, | ||
| }; | ||
| } | ||
| if (options.pinnedSha) { | ||
| // SECURITY: When pinnedSha is set, fail closed — do not fall through to | ||
| // other strategies that would resolve a different commit. | ||
|
|
||
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.
isShallowRepository()runs on everydepthArgs()call with no memoization, and it silently falls back tofalsewhengit rev-parse --is-shallow-repositoryexits non-zero (i.e., Git < 2.15). On a genuinely shallow clone with an older git binary,--depthis therefore omitted, recreating the original bug.Consider two improvements:
git rev-parsesubprocess runs at most once.exitCode !== 0, fall back tofs.existsSync(path.join(process.cwd(), ".git", "shallow"))— purely local, no credentials, and already used for this purpose ingenerate_git_patch.cjs.@copilot please address this.