refactor(git): extract some helper functions (#5902)

* refactor: extract helper function to get the latest commit hash

* refactor: simplify type of `GitCli`'s args

* refactor: remove unnecessary Git args filtering

* chore: document error-throwing of `GitCli`

* refactor: inline unnecessary local variable

* refactor: remove some code smells

* Optimize imports
* Inline unnecessary local variables
* Avoid implicit `any` type

* chore: explicit return type

* refactor(git): extract some git commands as named functions

Author: vvagaytsev

core/src/vcs/git.ts

@@ -8,7 +8,6 @@
 
 import { performance } from "perf_hooks"
 import { isAbsolute, join, normalize, posix, relative, resolve } from "path"
-import { isString } from "lodash-es"
 import fsExtra from "fs-extra"
 import { PassThrough } from "stream"
 import type {
@@ -67,21 +66,23 @@ export function parseGitUrl(url: string) {
         (e.g. https://github.com/org/repo.git#main). Actually got: '${url}'`,
     })
   }
-  const parsed = { repositoryUrl: parts[0], hash: parts[1] }
-  return parsed
+  return { repositoryUrl: parts[0], hash: parts[1] }
 }
 
 export interface GitCli {
-  (...args: (string | undefined)[]): Promise<string[]>
+  /**
+   * @throws ChildProcessError
+   */
+  (...args: string[]): Promise<string[]>
 }
 
 export function gitCli(log: Log, cwd: string, failOnPrompt = false): GitCli {
   /**
    * @throws ChildProcessError
    */
-  return async (...args: (string | undefined)[]) => {
+  return async (...args: string[]) => {
     log.silly(`Calling git with args '${args.join(" ")}' in ${cwd}`)
-    const { stdout } = await exec("git", args.filter(isString), {
+    const { stdout } = await exec("git", args, {
       cwd,
       maxBuffer: 10 * 1024 * 1024,
       env: failOnPrompt ? { GIT_TERMINAL_PROMPT: "0", GIT_ASKPASS: "true" } : undefined,
@@ -90,7 +91,7 @@ export function gitCli(log: Log, cwd: string, failOnPrompt = false): GitCli {
   }
 }
 
-async function getModifiedFiles(git: GitCli, path: string) {
+export async function getModifiedFiles(git: GitCli, path: string): Promise<string[]> {
   try {
     return await git("diff-index", "--name-only", "HEAD", path)
   } catch (err) {
@@ -103,6 +104,26 @@ async function getModifiedFiles(git: GitCli, path: string) {
   }
 }
 
+export async function getLastCommitHash(git: GitCli): Promise<string> {
+  const result = await git("rev-parse", "HEAD")
+  return result[0]
+}
+
+export async function getRepositoryRoot(git: GitCli): Promise<string> {
+  const result = await git("rev-parse", "--show-toplevel")
+  return result[0]
+}
+
+export async function getBranchName(git: GitCli): Promise<string> {
+  const result = await git("rev-parse", "--abbrev-ref", "HEAD")
+  return result[0]
+}
+
+export async function getOriginUrl(git: GitCli): Promise<string> {
+  const result = await git("config", "--get", "remote.origin.url")
+  return result[0]
+}
+
 interface GitSubTreeIncludeExcludeFiles extends BaseIncludeExcludeFiles {
   hasIncludes: boolean
 }
@@ -170,7 +191,7 @@ export class GitHandler extends VcsHandler {
 
       try {
         const git = gitCli(log, path, failOnPrompt)
-        const repoRoot = (await git("rev-parse", "--show-toplevel"))[0]
+        const repoRoot = await getRepositoryRoot(git)
         this.repoRoots.set(path, repoRoot)
         return repoRoot
       } catch (err) {
@@ -572,7 +593,7 @@ export class GitHandler extends VcsHandler {
       const gitLog = log.createLog({ name, showDuration: true }).info("Getting remote state")
       await git("remote", "update")
 
-      const localCommitId = (await git("rev-parse", "HEAD"))[0]
+      const localCommitId = await getLastCommitHash(git)
       const remoteCommitId = isSha1(hash) ? hash : getCommitIdFromRefList(await git("ls-remote", repositoryUrl, hash))
 
       if (localCommitId !== remoteCommitId) {
@@ -679,16 +700,16 @@ export class GitHandler extends VcsHandler {
     }
 
     try {
-      output.branch = (await git("rev-parse", "--abbrev-ref", "HEAD"))[0]
-      output.commitHash = (await git("rev-parse", "HEAD"))[0]
+      output.branch = await getBranchName(git)
+      output.commitHash = await getLastCommitHash(git)
     } catch (err) {
       if (err instanceof ChildProcessError && err.details.code !== 128) {
         throw err
       }
     }
 
     try {
-      output.originUrl = (await git("config", "--get", "remote.origin.url"))[0]
+      output.originUrl = await getOriginUrl(git)
     } catch (err) {
       // Just ignore if not available
       log.silly(`Tried to retrieve git remote.origin.url but encountered an error: ${err}`)

core/test/unit/src/vcs/git.ts

@@ -15,8 +15,7 @@ import { basename, dirname, join, relative, resolve } from "path"
 import type { TestGarden } from "../../../helpers.js"
 import { expectError, getDataDir, makeTestGarden, makeTestGardenA } from "../../../helpers.js"
 import type { GitCli } from "../../../../src/vcs/git.js"
-import { gitCli } from "../../../../src/vcs/git.js"
-import { explainGitError, getCommitIdFromRefList, GitHandler, parseGitUrl } from "../../../../src/vcs/git.js"
+import { explainGitError, getCommitIdFromRefList, gitCli, GitHandler, parseGitUrl } from "../../../../src/vcs/git.js"
 import type { Log } from "../../../../src/logger/log-entry.js"
 import { hashRepoUrl } from "../../../../src/util/ext-source-util.js"
 import { dedent, deline } from "../../../../src/util/string.js"
@@ -147,8 +146,8 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => {
       })
 
       const dirContexts = [
-        { ctx: "when called from repo root", pathFn: (tp) => tp },
-        { ctx: "when called from project root", pathFn: (tp) => resolve(tp, "somedir") },
+        { ctx: "when called from repo root", pathFn: (tp: string): string => tp },
+        { ctx: "when called from project root", pathFn: (tp: string): string => resolve(tp, "somedir") },
       ]
 
       for (const { ctx, pathFn } of dirContexts) {
@@ -1117,8 +1116,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => {
 
   describe("getRepoRoot", () => {
     it("should return the repo root if it is the same as the given path", async () => {
-      const path = tmpPath
-      expect(await handler.getRepoRoot(log, path)).to.equal(tmpPath)
+      expect(await handler.getRepoRoot(log, tmpPath)).to.equal(tmpPath)
     })
 
     it("should return the nearest repo root, given a subpath of that repo", async () => {
@@ -1142,8 +1140,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => {
 
   describe("getPathInfo", () => {
     it("should return empty strings with no commits in repo", async () => {
-      const path = tmpPath
-      const { branch, commitHash } = await handler.getPathInfo(log, path)
+      const { branch, commitHash } = await handler.getPathInfo(log, tmpPath)
       expect(branch).to.equal("")
       expect(commitHash).to.equal("")
     })
@@ -1156,8 +1153,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => {
     })
 
     it("should return empty strings when given a path outside of a repo", async () => {
-      const path = tmpPath
-      const { branch, commitHash, originUrl } = await handler.getPathInfo(log, path)
+      const { branch, commitHash, originUrl } = await handler.getPathInfo(log, tmpPath)
       expect(branch).to.equal("")
       expect(commitHash).to.equal("")
       expect(originUrl).to.equal("")