-
Notifications
You must be signed in to change notification settings - Fork 472
feat: fail fast on implausible commit range in shallow checkout #49003
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
bc5e656
e58029e
620322b
f819bb2
fb98f1c
abf821d
9da9e4a
e1aa6cd
8df79ad
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 |
|---|---|---|
|
|
@@ -244,6 +244,89 @@ function ensureOriginRemoteTrackingRef(branch, options) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Maximum number of commits in a `base..head` range before it is considered | ||
| * implausible for a shallow checkout. A shallow clone with `fetch-depth: 1` | ||
| * will report the entire local history as the range when the base ref is not | ||
| * reachable from the shallow grafts, producing a count in the tens-of-thousands | ||
| * even for a single-commit branch. This threshold guards against that case. | ||
| * | ||
| * Callers can override the threshold via the `maxCommits` option on the | ||
| * functions that accept it. | ||
| */ | ||
| const SHALLOW_RANGE_MAX_COMMITS = 100; | ||
|
|
||
| /** | ||
| * Detect whether a commit range is implausibly large for a shallow checkout. | ||
| * | ||
| * In a shallow clone (`fetch-depth: 1`) the base ref (e.g. `origin/main`) has | ||
| * no traversable ancestry, so `git rev-list base..HEAD` cannot exclude anything | ||
| * and returns essentially the entire repository history. A count far above what | ||
| * a typical PR branch would contain almost certainly means the base ref is not | ||
| * reachable from the shallow grafts rather than representing real branch work. | ||
| * | ||
| * When the range size exceeds `options.maxCommits` (default | ||
| * `SHALLOW_RANGE_MAX_COMMITS`) **and** the repository is shallow, this function | ||
| * emits a `core.warning` with an actionable message and returns | ||
| * `{ implausible: true, commitCount }`. Otherwise it returns | ||
| * `{ implausible: false, commitCount }`. | ||
| * | ||
| * All git failures are treated as non-fatal so callers are never blocked. | ||
| * If `git rev-list` fails, returns `{ implausible: false, commitCount: 0 }`. | ||
| * If `git rev-list` succeeds but `git rev-parse --is-shallow-repository` fails, | ||
| * returns `{ implausible: false, commitCount }` with the measured commit count. | ||
| * | ||
| * @param {string} baseRef - The base ref (exclusive). Example: "origin/main". | ||
| * @param {string} headRef - The head ref (inclusive). Example: "HEAD" or a branch name. | ||
| * @param {Object} [options] | ||
| * @param {string} [options.cwd] - Working directory for git commands. | ||
| * @param {number} [options.maxCommits] - Override the implausibility threshold. | ||
| * @returns {{ implausible: boolean, commitCount: number }} | ||
| */ | ||
| function checkImplausibleShallowRange(baseRef, headRef, options = {}) { | ||
| const { maxCommits = SHALLOW_RANGE_MAX_COMMITS, cwd } = options; | ||
| if (!baseRef || !headRef) return { implausible: false, commitCount: 0 }; | ||
|
|
||
| let commitCount = 0; | ||
| try { | ||
| const countOut = execGitSync(["rev-list", "--count", `${baseRef}..${headRef}`], { | ||
| cwd, | ||
| suppressLogs: true, | ||
| }); | ||
| commitCount = parseInt(countOut.trim(), 10) || 0; | ||
| } catch { | ||
| return { implausible: false, commitCount: 0 }; | ||
| } | ||
|
|
||
| if (!Number.isFinite(commitCount) || commitCount <= maxCommits) { | ||
| return { implausible: false, commitCount }; | ||
| } | ||
|
|
||
| // Count is suspiciously large; only flag it as implausible when the repo is | ||
| // actually shallow — a large honest branch in a full clone is fine. | ||
| let isShallow = false; | ||
| try { | ||
| const shallowOut = execGitSync(["rev-parse", "--is-shallow-repository"], { | ||
| cwd, | ||
| suppressLogs: true, | ||
| }); | ||
| isShallow = shallowOut.trim() === "true"; | ||
| } catch { | ||
| return { implausible: false, commitCount }; | ||
| } | ||
|
|
||
| if (isShallow) { | ||
| core.warning( | ||
| `Shallow checkout produced an implausible commit range of ${commitCount} commits for ${baseRef}..${headRef}. ` + | ||
| `This usually means ${baseRef} is not reachable from the shallow grafts. ` + | ||
| `Increase fetch-depth in your workflow checkout step (e.g. fetch-depth: 0) to resolve this.` | ||
| ); | ||
| return { implausible: true, commitCount }; | ||
| } | ||
|
|
||
| return { implausible: false, commitCount }; | ||
| } | ||
|
|
||
| /** | ||
| * Check whether a commit range contains any merge commits. | ||
| * | ||
|
|
@@ -257,14 +340,26 @@ function ensureOriginRemoteTrackingRef(branch, options) { | |
| * "unknown" as "no merge commits detected" so that a detection failure never | ||
| * blocks the normal patch path. | ||
| * | ||
| * Also returns `false` when the range is implausibly large for a shallow | ||
| * checkout (see `checkImplausibleShallowRange`). In that case a warning is | ||
| * already emitted by the range check, so forcing bundle transport based on | ||
| * phantom merge-commit detections in a huge, unreliable range is avoided. | ||
| * | ||
| * @param {string} baseRef - The base ref (exclusive). Example: "origin/feature". | ||
| * @param {string} headRef - The head ref (inclusive). Example: "feature". | ||
| * @param {Object} [options] | ||
| * @param {string} [options.cwd] - Working directory for the git command. | ||
| * @param {number} [options.maxCommits] - Override the implausibility threshold. | ||
| * @returns {boolean} True if at least one merge commit exists in baseRef..headRef. | ||
| */ | ||
| function hasMergeCommitsInRange(baseRef, headRef, options = {}) { | ||
| if (!baseRef || !headRef) return false; | ||
|
|
||
| // Guard: if the range is implausibly large for a shallow checkout, treat as | ||
| // no merge commits so a false-positive does not force bundle transport. | ||
| const { implausible } = checkImplausibleShallowRange(baseRef, headRef, options); | ||
|
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. Missing test coverage: no test asserts that hasMergeCommitsInRange actually returns false when checkImplausibleShallowRange reports implausible:true. 💡 DetailsThe 14 new tests added in git_helpers.test.cjs exercise checkImplausibleShallowRange directly and linearizeRangeAsCommit's guard, but none call Suggested addition: a test in a |
||
| if (implausible) return false; | ||
|
|
||
| try { | ||
| const out = execGitSync(["rev-list", "--merges", "--count", `${baseRef}..${headRef}`], { | ||
| cwd: options.cwd, | ||
|
|
@@ -615,15 +710,50 @@ async function backfillCommitObjects(execApi, commitShas, options = {}) { | |
| * When omitted, exec calls are made without additional options. | ||
| * @param {string[]} [opts.commitFlags] - Extra flags prepended before `-m` in the `git commit` | ||
| * invocation (e.g. `["--allow-empty", "--no-verify"]`). | ||
| * @param {number} [opts.maxCommits] - Override the implausibility threshold (default | ||
| * `SHALLOW_RANGE_MAX_COMMITS`). Set to `Infinity` to disable the shallow guard. | ||
| * @returns {Promise<string>} The new HEAD SHA after the rewrite. | ||
| * @throws {Error} If the soft reset, staged-changes validation, or recommit fails. | ||
| * @throws {Error} If the soft reset, staged-changes validation, or recommit fails, or if a | ||
| * shallow checkout produces an implausible commit range. | ||
| */ | ||
| async function linearizeRangeAsCommit(baseRef, commitMessage, execApi, opts = {}) { | ||
|
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. linearizeRangeAsCommit re-implements the shallow-range implausibility check with its own inline logic instead of reusing checkImplausibleShallowRange, risking behavior drift between the two guards. 💡 DetailsBoth checkImplausibleShallowRange (line 284) and this inline block (lines 728-753) independently count commits via a rev-list call and probe --is-shallow-repository, but one uses execGitSync (sync spawn) while the other uses execApi.getExecOutput (async exec) with a different error-message format. Any future fix or threshold change in one place silently won't apply to the other — the two implementations already diverge (execGitSync vs async execApi with a distinct ignoreReturnCode option), so drift is already present, not just hypothetical. Suggested fix: refactor the inline block to call a shared async-capable version of checkImplausibleShallowRange so both callers share one source of truth for the guard logic. |
||
| const { gitOpts, commitFlags = [] } = opts; | ||
| const { gitOpts, commitFlags = [], maxCommits = SHALLOW_RANGE_MAX_COMMITS } = opts; | ||
| // Spread gitOpts into exec calls only when it is explicitly provided — passing | ||
| // `undefined` as a third argument changes the arity seen by mocks in tests. | ||
| const execArgs = gitOpts !== undefined ? [gitOpts] : []; | ||
|
|
||
| // Guard against implausibly large commit ranges in shallow checkouts. | ||
| // In a shallow clone with fetch-depth:1 the base ref has no traversable | ||
| // ancestry, so git rev-list cannot exclude anything and returns the entire | ||
| // local history. Synthesizing a rewrite commit from tens-of-thousands of | ||
| // commits is almost certainly wrong and could produce an oversized payload. | ||
| { | ||
| let rangeCommitCount = 0; | ||
| try { | ||
| const { stdout: countOut } = await execApi.getExecOutput("git", ["rev-list", "--count", `${baseRef}..HEAD`], ...execArgs); | ||
| rangeCommitCount = parseInt(countOut.trim(), 10) || 0; | ||
| } catch { | ||
| rangeCommitCount = 0; | ||
| } | ||
| if (Number.isFinite(rangeCommitCount) && rangeCommitCount > maxCommits) { | ||
|
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.
@copilot please address this. |
||
| let isShallow = false; | ||
| try { | ||
| const shallowOpts = gitOpts !== undefined ? { ...gitOpts, ignoreReturnCode: true } : { ignoreReturnCode: true }; | ||
| const { stdout: shallowOut } = await execApi.getExecOutput("git", ["rev-parse", "--is-shallow-repository"], shallowOpts); | ||
| isShallow = shallowOut.trim() === "true"; | ||
| } catch { | ||
| // Non-fatal: if the shallow probe fails, do not block the rewrite. | ||
| } | ||
| if (isShallow) { | ||
| throw new Error( | ||
| `Refusing to linearize an implausible commit range: ${rangeCommitCount} commits in ${baseRef}..HEAD in a shallow checkout. ` + | ||
| `This likely means ${baseRef} is not reachable from the shallow grafts. ` + | ||
| `Increase fetch-depth in your workflow checkout step (e.g. fetch-depth: 0) to resolve this.` | ||
| ); | ||
| } | ||
|
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. The guard in Consider adapting @copilot please address this. |
||
| } | ||
| } | ||
|
|
||
| const { stdout: originalHeadOut } = await execApi.getExecOutput("git", ["rev-parse", "HEAD"], ...execArgs); | ||
| const originalHead = originalHeadOut.trim(); | ||
| if (!originalHead) { | ||
|
|
@@ -654,6 +784,7 @@ module.exports = { | |
| ensureSafeDirectoryTrust, | ||
| execGitSync, | ||
| backfillCommitObjects, | ||
| checkImplausibleShallowRange, | ||
| ensureFullHistoryForBundle, | ||
| ensureOriginRemoteTrackingRef, | ||
| extractBundlePrerequisiteCommits, | ||
|
|
@@ -662,4 +793,5 @@ module.exports = { | |
| hasMergeCommitsInRange, | ||
| isShallowOrSparseCheckout, | ||
| linearizeRangeAsCommit, | ||
| SHALLOW_RANGE_MAX_COMMITS, | ||
| }; | ||
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.
The hardcoded threshold of 100 commits can false-positive on legitimate large branches in a shallow-but-deep checkout (e.g. fetch-depth: 200), causing valid work to be blocked or misreported.
💡 Details
The guard only tests
--is-shallow-repository, not how deep the clone actually is. A repo withfetch-depth: 200is still "shallow" per git's definition, and a genuinely active feature branch can easily accumulate more than 100 real commits against its base. In that case:hasMergeCommitsInRangesilently returnsfalseeven if there truly are merge commits in a 100+ commit range, defeating the bundle-transport detection it exists to protect.linearizeRangeAsCommithard-throws and refuses to linearize valid work, surfacing a confusing error that blames "implausible" history when the range is real.Consider comparing the reported commit count against
git rev-list --count HEAD(the shallow grafted depth) or against the shallow boundary itself, rather than a flat magic number, so the guard only triggers when the count actually exceeds what the clone's real depth could produce.