Address feedback: cache git version, improve error handling, add telemetry

- 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>

src/diagnostics.ts

@@ -185,3 +185,28 @@ export function flushDiagnostics(config: Config) {
   // Reset the unwritten diagnostics array.
   unwrittenDiagnostics = [];
 }
+
+/**
+ * Creates a telemetry-only diagnostic message. This is a convenience function
+ * for creating diagnostics that should only be sent to telemetry and not
+ * 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.
+ * @param attributes Structured metadata about the diagnostic message.
+ * @returns Returns the new telemetry diagnostic message.
+ */
+export function makeTelemetryDiagnostic(
+  id: string,
+  name: string,
+  attributes: { [key: string]: any },
+): DiagnosticMessage {
+  return makeDiagnostic(id, name, {
+    attributes,
+    visibility: {
+      cliSummaryTable: false,
+      statusPage: false,
+      telemetry: true,
+    },
+  });
+}

src/git-utils.test.ts

@@ -398,39 +398,46 @@ test("getFileOidsUnderPath throws on unexpected output format", async (t) => {
   }
 });
 
-test("getGitVersion returns version for valid git output", async (t) => {
+test("getGitVersionOrThrow returns version for valid git output", async (t) => {
   const runGitCommandStub = sinon
     .stub(gitUtils as any, "runGitCommand")
     .resolves("git version 2.40.0\n");
 
   try {
-    const version = await gitUtils.getGitVersion();
+    const version = await gitUtils.getGitVersionOrThrow();
     t.is(version, "2.40.0");
   } finally {
     runGitCommandStub.restore();
   }
 });
 
-test("getGitVersion returns undefined for invalid git output", async (t) => {
+test("getGitVersionOrThrow throws for invalid git output", async (t) => {
   const runGitCommandStub = sinon
     .stub(gitUtils as any, "runGitCommand")
     .resolves("invalid output");
 
   try {
-    const version = await gitUtils.getGitVersion();
-    t.is(version, undefined);
+    await t.throwsAsync(
+      async () => {
+        await gitUtils.getGitVersionOrThrow();
+      },
+      {
+        instanceOf: Error,
+        message: "Could not parse Git version from output: invalid output",
+      },
+    );
   } finally {
     runGitCommandStub.restore();
   }
 });
 
-test("getGitVersion handles Windows-style git output", async (t) => {
+test("getGitVersionOrThrow handles Windows-style git output", async (t) => {
   const runGitCommandStub = sinon
     .stub(gitUtils as any, "runGitCommand")
     .resolves("git version 2.40.0.windows.1\n");
 
   try {
-    const version = await gitUtils.getGitVersion();
+    const version = await gitUtils.getGitVersionOrThrow();
     // Should extract just the major.minor.patch portion
     t.is(version, "2.40.0");
   } finally {
@@ -438,20 +445,79 @@ test("getGitVersion handles Windows-style git output", async (t) => {
   }
 });
 
-test("getGitVersion returns undefined when git command fails", async (t) => {
+test("getGitVersionOrThrow throws when git command fails", async (t) => {
   const runGitCommandStub = sinon
     .stub(gitUtils as any, "runGitCommand")
     .rejects(new Error("git not found"));
 
   try {
-    const version = await gitUtils.getGitVersion();
-    t.is(version, undefined);
+    await t.throwsAsync(
+      async () => {
+        await gitUtils.getGitVersionOrThrow();
+      },
+      {
+        instanceOf: Error,
+        message: "git not found",
+      },
+    );
   } finally {
     runGitCommandStub.restore();
   }
 });
 
+test("getGitVersion returns version and caches it", async (t) => {
+  gitUtils.resetCachedGitVersion();
+  const runGitCommandStub = sinon
+    .stub(gitUtils as any, "runGitCommand")
+    .resolves("git version 2.40.0\n");
+
+  const messages: LoggedMessage[] = [];
+  const logger = getRecordingLogger(messages);
+
+  try {
+    // First call should fetch and cache
+    const version1 = await gitUtils.getGitVersion(logger);
+    t.is(version1, "2.40.0");
+    t.is(runGitCommandStub.callCount, 1);
+
+    // Second call should use cache
+    const version2 = await gitUtils.getGitVersion(logger);
+    t.is(version2, "2.40.0");
+    t.is(runGitCommandStub.callCount, 1); // Should still be 1
+  } finally {
+    runGitCommandStub.restore();
+    gitUtils.resetCachedGitVersion();
+  }
+});
+
+test("getGitVersion returns undefined when version cannot be determined", async (t) => {
+  gitUtils.resetCachedGitVersion();
+  const runGitCommandStub = sinon
+    .stub(gitUtils as any, "runGitCommand")
+    .rejects(new Error("git not found"));
+
+  const messages: LoggedMessage[] = [];
+  const logger = getRecordingLogger(messages);
+
+  try {
+    const version = await gitUtils.getGitVersion(logger);
+    t.is(version, undefined);
+    t.true(
+      messages.some(
+        (m) =>
+          m.type === "debug" &&
+          typeof m.message === "string" &&
+          m.message.includes("Could not determine Git version"),
+      ),
+    );
+  } finally {
+    runGitCommandStub.restore();
+    gitUtils.resetCachedGitVersion();
+  }
+});
+
 test("gitVersionAtLeast returns true for version meeting requirement", async (t) => {
+  gitUtils.resetCachedGitVersion();
   const runGitCommandStub = sinon
     .stub(gitUtils as any, "runGitCommand")
     .resolves("git version 2.40.0\n");
@@ -471,10 +537,12 @@ test("gitVersionAtLeast returns true for version meeting requirement", async (t)
     );
   } finally {
     runGitCommandStub.restore();
+    gitUtils.resetCachedGitVersion();
   }
 });
 
 test("gitVersionAtLeast returns false for version not meeting requirement", async (t) => {
+  gitUtils.resetCachedGitVersion();
   const runGitCommandStub = sinon
     .stub(gitUtils as any, "runGitCommand")
     .resolves("git version 2.30.0\n");
@@ -487,10 +555,12 @@ test("gitVersionAtLeast returns false for version not meeting requirement", asyn
     t.false(result);
   } finally {
     runGitCommandStub.restore();
+    gitUtils.resetCachedGitVersion();
   }
 });
 
 test("gitVersionAtLeast returns false when version cannot be determined", async (t) => {
+  gitUtils.resetCachedGitVersion();
   const runGitCommandStub = sinon
     .stub(gitUtils as any, "runGitCommand")
     .rejects(new Error("git not found"));
@@ -505,10 +575,12 @@ test("gitVersionAtLeast returns false when version cannot be determined", async
       messages.some(
         (m) =>
           m.type === "debug" &&
-          m.message === "Could not determine Git version.",
+          typeof m.message === "string" &&
+          m.message.includes("Could not determine Git version"),
       ),
     );
   } finally {
     runGitCommandStub.restore();
+    gitUtils.resetCachedGitVersion();
   }
 });

src/git-utils.ts

@@ -8,8 +8,14 @@ import {
   getWorkflowEvent,
   getWorkflowEventName,
 } from "./actions-util";
+import type { Config } from "./config-utils";
+import { addDiagnostic, makeTelemetryDiagnostic } from "./diagnostics";
 import { Logger } from "./logging";
-import { ConfigurationError, getRequiredEnvParam } from "./util";
+import {
+  ConfigurationError,
+  getErrorMessage,
+  getRequiredEnvParam,
+} from "./util";
 
 /**
  * Minimum Git version required for overlay analysis. The `git ls-files --format`
@@ -17,28 +23,81 @@ import { ConfigurationError, getRequiredEnvParam } from "./util";
  */
 export const GIT_MINIMUM_VERSION_FOR_OVERLAY = "2.38.0";
 
+/** Cached git version to avoid recomputing it multiple times. */
+let cachedGitVersion: string | undefined;
+
 /**
- * Gets the version of Git installed on the system.
- *
- * @returns The Git version string (e.g., "2.40.0"), or undefined if the
- *          version could not be determined.
+ * Resets the cached git version. This is intended for use in tests only.
  */
-export async function getGitVersion(): Promise<string | undefined> {
+export function resetCachedGitVersion(): void {
+  cachedGitVersion = undefined;
+}
+
+/**
+ * Gets the version of Git installed on the system and throws an error if
+ * the version cannot be determined.
+ *
+ * @returns The Git version string (e.g., "2.40.0").
+ * @throws {Error} if the version could not be determined.
+ */
+export async function getGitVersionOrThrow(): Promise<string> {
+  const stdout = await runGitCommand(
+    undefined,
+    ["--version"],
+    "Failed to get git version.",
+  );
+  // 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+)/);
+  if (match?.[1]) {
+    return match[1];
+  }
+  throw new Error(`Could not parse Git version from output: ${stdout.trim()}`);
+}
+
+/**
+ * Gets the cached Git version, or fetches and caches it if not yet cached.
+ *
+ * @param logger A logger to use for logging errors.
+ * @returns The cached Git version, or undefined if the version could not be determined.
+ */
+export async function getGitVersion(
+  logger: Logger,
+): Promise<string | undefined> {
+  if (cachedGitVersion !== undefined) {
+    return cachedGitVersion;
+  }
   try {
-    const stdout = await runGitCommand(
-      undefined,
-      ["--version"],
-      "Failed to get git version.",
+    cachedGitVersion = await getGitVersionOrThrow();
+    return cachedGitVersion;
+  } catch (e) {
+    logger.debug(`Could not determine Git version: ${getErrorMessage(e)}`);
+    return undefined;
+  }
+}
+
+/**
+ * Logs the Git version as a telemetry diagnostic. Should be called once during
+ * initialization after the config is available.
+ *
+ * @param config The configuration that tells us where to store the diagnostic.
+ * @param logger A logger to use for logging errors.
+ */
+export async function logGitVersionTelemetry(
+  config: Config,
+  logger: Logger,
+): Promise<void> {
+  const version = await getGitVersion(logger);
+  if (version !== undefined) {
+    addDiagnostic(
+      config,
+      config.languages[0],
+      makeTelemetryDiagnostic(
+        "codeql-action/git-version-telemetry",
+        "Git version telemetry",
+        { gitVersion: version },
+      ),
     );
-    // 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+)/);
-    if (match?.[1]) {
-      return match[1];
-    }
-    return undefined;
-  } catch {
-    return undefined;
   }
 }
 
@@ -54,9 +113,8 @@ export async function gitVersionAtLeast(
   requiredVersion: string,
   logger: Logger,
 ): Promise<boolean> {
-  const version = await getGitVersion();
+  const version = await getGitVersion(logger);
   if (version === undefined) {
-    logger.debug("Could not determine Git version.");
     return false;
   }
   logger.debug(`Installed Git version is ${version}.`);

src/init-action.ts

@@ -37,6 +37,7 @@ import {
 import { EnvVar } from "./environment";
 import { Feature, Features } from "./feature-flags";
 import { loadPropertiesFromApi } from "./feature-flags/properties";
+import { logGitVersionTelemetry } from "./git-utils";
 import {
   checkInstallPython311,
   checkPacksForOverlayCompatibility,
@@ -433,6 +434,9 @@ async function run() {
       );
     }
 
+    // Log Git version telemetry
+    await logGitVersionTelemetry(config, logger);
+
     // Forward Go flags
     const goFlags = process.env["GOFLAGS"];
     if (goFlags) {