-
Notifications
You must be signed in to change notification settings - Fork 428
Add git version check for overlay analysis enablement #3370
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
base: main
Are you sure you want to change the base?
Conversation
Overlay analysis depends on `getFileOidsUnderPath`, which uses `git ls-files --format` option that requires Git 2.38.0+. This change adds a check for the git version before enabling overlay analysis. Co-authored-by: henrymercer <14129055+henrymercer@users.noreply.github.com>
- Add test for Windows-style git version format - Add comment clarifying regex extracts major.minor.patch - Replace dynamic import with static import for semver Co-authored-by: henrymercer <14129055+henrymercer@users.noreply.github.com>
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.
Pull request overview
This PR adds Git version checking to ensure overlay analysis is only enabled when Git 2.38.0 or newer is available. Overlay analysis depends on the git ls-files --format option introduced in Git 2.38.0, and this change prevents runtime failures when an older Git version is installed.
Key changes:
- Added Git version detection and comparison utilities
- Integrated version check into overlay analysis enablement logic
- Falls back to full database analysis with a warning when Git is too old
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/git-utils.ts | Added version constants, parsing function, and comparison utilities for Git version checking |
| src/git-utils.test.ts | Added comprehensive test coverage for version parsing and comparison logic |
| src/config-utils.ts | Integrated Git version check into overlay database mode determination |
| src/config-utils.test.ts | Added test cases for overlay fallback scenarios with old/missing Git versions |
| lib/*.js | Generated JavaScript output from TypeScript compilation |
|
@copilot Please address the following feedback: Error handling improvements
Telemetry
|
…metry - Cache the git version to avoid recomputing on repeated calls - Refactor getGitVersion to getGitVersionOrThrow with detailed errors - Add getGitVersion that logs errors and handles caching - Add makeTelemetryDiagnostic helper to diagnostics.ts - Add logGitVersionTelemetry function to log git version telemetry - Call logGitVersionTelemetry in init-action.ts - Add resetCachedGitVersion for testing - Update tests to work with new function signatures and caching Co-authored-by: henrymercer <14129055+henrymercer@users.noreply.github.com>
Refactored bundle-download-telemetry and zstd-availability diagnostics in init-action.ts to use the new makeTelemetryDiagnostic helper function. Also added guard for empty languages array in logGitVersionTelemetry. Co-authored-by: henrymercer <14129055+henrymercer@users.noreply.github.com>
... Addressed in commits c3dc529 and 393c074: Error handling improvements:
Telemetry:
|
mbg
left a comment
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.
I have given this an initial review. Two primary points:
- I am not keen on having the logging/diagnostic code in
git-utils. That should essentially just havegetGitVersionOrThrowin it along with maybe the caching. - The quality of the tests is somewhat poor.
src/git-utils.ts
Outdated
| import type { Config } from "./config-utils"; | ||
| import { addDiagnostic, makeTelemetryDiagnostic } from "./diagnostics"; | ||
| import { Logger } from "./logging"; |
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.
I am not overly keen on having this in git-utils. I think it would make more sense to keep this file for the git-related utility functions and then perform the version check / logging / diagnostic stuff elsewhere.
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.
I've moved the logging/diagnostics elsewhere, so now it's just the ability to get the git version. I think that's not worth moving to a new file.
src/diagnostics.ts
Outdated
| * displayed on the status page or CLI summary table. | ||
| * | ||
| * @param id An identifier under which it makes sense to group this diagnostic message. | ||
| * @param name Display name for the ID. |
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.
| * @param name Display name for the ID. | |
| * @param name Display name for the diagnostic. |
src/config-utils.test.ts
Outdated
| languages: [KnownLanguage.javascript], | ||
| codeqlVersion: CODEQL_OVERLAY_MINIMUM_VERSION, | ||
| gitRoot: "/some/git/root", | ||
| gitVersion: "2.40.0", // Default to a version that supports overlay analysis |
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.
Why is this not GIT_MINIMUM_VERSION_FOR_OVERLAY by default? Having a different version here might mislead someone into thinking that 2.40.0 is the minimum required, and it means we don't test the case for where the git version is exactly 2.38.0.
src/git-utils.test.ts
Outdated
| const version = await gitUtils.getGitVersionOrThrow(); | ||
| t.is(version, "2.40.0"); | ||
| } finally { | ||
| runGitCommandStub.restore(); |
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.
This is not necessary.
src/git-utils.test.ts
Outdated
| runGitCommandStub.restore(); | ||
| gitUtils.resetCachedGitVersion(); |
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.
As above.
src/git-utils.test.ts
Outdated
| const messages: LoggedMessage[] = []; | ||
| const logger = getRecordingLogger(messages); |
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.
As above.
src/git-utils.test.ts
Outdated
| t.true( | ||
| messages.some( | ||
| (m) => | ||
| m.type === "debug" && | ||
| typeof m.message === "string" && | ||
| m.message.includes("Could not determine Git version"), | ||
| ), | ||
| ); |
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.
As above.
src/git-utils.test.ts
Outdated
| runGitCommandStub.restore(); | ||
| gitUtils.resetCachedGitVersion(); |
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.
As above.
src/git-utils.ts
Outdated
| export const GIT_MINIMUM_VERSION_FOR_OVERLAY = "2.38.0"; | ||
|
|
||
| /** Cached git version to avoid recomputing it multiple times. */ | ||
| let cachedGitVersion: string | undefined; |
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.
Observation: This will cache the git version for one step in a workflow (e.g. init), but not across steps (e.g. init and analyze).
I think that's probably OK based on the current usage where logGitVersionTelemetry is only called once in init, but does lead to two points:
- What is the point of caching this if we only use it once right now?
- If we start needing this elsewhere, then we still might not make use of the caching if the other uses are in steps other than
init.
So this only becomes useful if we need the git version multiple times in the same action.
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 caching makes for an easier API since we don't need to pass the git version around the program, but as you mention, we're not really making use of it now. I've removed it.
mbg
left a comment
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.
Thanks! Most of my feedback is addressed now, I think. I responded to the question in one of the conversations and added one new question. Other than those, this looks good now.
src/git-utils.ts
Outdated
| // Git version output can vary: "git version 2.40.0" or "git version 2.40.0.windows.1" | ||
| // We capture just the major.minor.patch portion to ensure semver compatibility. | ||
| const match = stdout.match(/git version (\d+\.\d+\.\d+)/); |
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.
It's probably OK for now, especially if we report the git version in telemetry. We can keep an eye out for unusual version formats that we might care about.
One possible improvement might be to document in the JSDoc that only the first three components are considered or, indeed, return a more structured type than string which also makes it clear that the version might be truncated to just those parts.
src/config-utils.ts
Outdated
| logger.info(`Using Git version ${gitVersion}`); | ||
| await logGitVersionTelemetry(config, gitVersion); | ||
| } catch (e) { | ||
| logger.debug(`Could not determine Git version: ${getErrorMessage(e)}`); |
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.
Question about the log-levels here. The actual failure here is logged at debug level, and so wouldn't ordinarily be visible (especially in dynamic workflows).
Meanwhile, the subsequent message in getOverlayDatabaseMode is logged at warning level and so would show up quite prominently in the log (including workflow annotations).
That could lead to a situation where the actual problem is not easily debuggable. Should this be a warning instead / in addition to the gitVersion === undefined message?
Overlay analysis depends on
getFileOidsUnderPath, which usesgit ls-files --formatoption introduced in Git 2.38.0. This adds a version check to ensure overlay analysis is only enabled when the git version is new enough.Changes
src/git-utils.ts:GIT_MINIMUM_VERSION_FOR_OVERLAYconstant ("2.38.0")getGitVersionOrThrow()function that throws with detailed error messagesgetGitVersion()function with caching and error logginggitVersionAtLeast()function for semver comparisonlogGitVersionTelemetry()function to log git version as telemetry diagnosticresetCachedGitVersion()for testing supportsrc/config-utils.ts: Added git version check ingetOverlayDatabaseMode()that falls back to non-overlay analysis with a warning if git is too oldsrc/diagnostics.ts: AddedmakeTelemetryDiagnostic()helper function for creating telemetry-only diagnosticssrc/init-action.ts:logGitVersionTelemetry()for git version telemetrybundle-download-telemetryandzstd-availabilitydiagnostics to usemakeTelemetryDiagnostic()2.40.0.windows.1), caching behavior, and overlay fallback scenariosWhen git version is insufficient, users will see:
Risk assessment
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, CCR, ...).Products:
analysis-kinds: code-scanning.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.