fix(vcs): untracked files didn't update version timestamp correctly

Needed to refactor slightly to add tests for this.

.circleci/config.yml

@@ -184,6 +184,11 @@ jobs:
           docker_layer_caching: true
       - npm_install
       - *attach-workspace
+      - run:
+          name: Configure git (needed for some tests)
+          command: |
+            git config --global user.name "Garden CI"
+            git config --global user.email "[email protected]"
       - run:
           name: Make sure generated docs are up-to-date
           command: npm run check-docs

garden-service/src/vcs/base.ts

@@ -13,7 +13,7 @@ import * as Joi from "joi"
 import { validate } from "../config/common"
 import { join } from "path"
 import { GARDEN_VERSIONFILE_NAME } from "../constants"
-import { pathExists, readFile, writeFile } from "fs-extra"
+import { pathExists, readFile, writeFile, stat } from "fs-extra"
 import { ConfigurationError } from "../exceptions"
 import {
   ExternalSourceType,
@@ -83,9 +83,35 @@ export abstract class VcsHandler {
   constructor(protected projectRoot: string) { }
 
   abstract name: string
-  abstract async getTreeVersion(path: string): Promise<TreeVersion>
+  abstract async getLatestCommit(path: string): Promise<string>
+  abstract async getDirtyFiles(path: string): Promise<string[]>
   abstract async ensureRemoteSource(params: RemoteSourceParams): Promise<string>
-  abstract async updateRemoteSource(params: RemoteSourceParams)
+  abstract async updateRemoteSource(params: RemoteSourceParams): Promise<void>
+
+  async getTreeVersion(path: string) {
+    const commitHash = await this.getLatestCommit(path)
+    const dirtyFiles = await this.getDirtyFiles(path)
+
+    let latestDirty = 0
+
+    // for dirty trees, we append the last modified time of last modified or added file
+    if (dirtyFiles.length) {
+      const stats = await Bluebird.filter(dirtyFiles, (file: string) => pathExists(file))
+        .map((file: string) => stat(file))
+
+      let mtimes = stats.map((s) => Math.round(s.mtime.getTime() / 1000))
+      let latest = mtimes.sort().slice(-1)[0]
+
+      if (latest > latestDirty) {
+        latestDirty = latest
+      }
+    }
+
+    return {
+      latestCommit: commitHash,
+      dirtyTimestamp: latestDirty || null,
+    }
+  }
 
   async resolveTreeVersion(path: string): Promise<TreeVersion> {
     // the version file is used internally to specify versions outside of source control

garden-service/src/vcs/git.ts

@@ -7,21 +7,12 @@
  */
 
 import execa = require("execa")
-import { join } from "path"
-import { ensureDir, pathExists, stat } from "fs-extra"
-import Bluebird = require("bluebird")
+import { join, resolve } from "path"
+import { ensureDir, pathExists } from "fs-extra"
 
 import { NEW_MODULE_VERSION, VcsHandler, RemoteSourceParams } from "./base"
 import { ConfigurationError, RuntimeError } from "../exceptions"
 
-export const helpers = {
-  gitCli: (cwd: string): (cmd: string, args: string[]) => Promise<string> => {
-    return async (cmd, args) => {
-      return execa.stdout("git", [cmd, ...args], { cwd })
-    }
-  },
-}
-
 export function getCommitIdFromRefList(refList: string): string {
   try {
     return refList.split("\n")[0].split("\t")[0]
@@ -47,70 +38,66 @@ export function parseGitUrl(url: string) {
 export class GitHandler extends VcsHandler {
   name = "git"
 
-  async getTreeVersion(path: string) {
-    const git = helpers.gitCli(path)
+  private gitCli(cwd: string) {
+    return async (...args: string[]) => {
+      return execa.stdout("git", args, { cwd })
+    }
+  }
+
+  async getLatestCommit(path: string) {
+    const git = this.gitCli(path)
 
-    let commitHash
     try {
-      commitHash = await git("rev-list", [
+      return await git(
+        "rev-list",
         "--max-count=1",
         "--abbrev-commit",
         "--abbrev=10",
         "HEAD",
-      ]) || NEW_MODULE_VERSION
+      ) || NEW_MODULE_VERSION
     } catch (err) {
       if (err.code === 128) {
         // not in a repo root, use default version
-        commitHash = NEW_MODULE_VERSION
+        return NEW_MODULE_VERSION
       } else {
         throw err
       }
     }
+  }
+
+  async getDirtyFiles(path: string) {
+    const git = this.gitCli(path)
+    let modifiedFiles: string[]
 
-    let latestDirty = 0
-    let modifiedFiles: string[] = []
+    const repoRoot = await git("rev-parse", "--show-toplevel")
 
     try {
-      modifiedFiles = (await git("diff-index", ["--name-only", "HEAD", path])).split("\n")
+      modifiedFiles = (await git("diff-index", "--name-only", "HEAD", path))
+        .split("\n")
+        .filter((f) => f.length > 0)
+        .map(file => resolve(repoRoot, file))
     } catch (err) {
       if (err.code === 128) {
-        // no commit in repo, use default version
-        commitHash = NEW_MODULE_VERSION
+        // no commit in repo
+        modifiedFiles = []
       } else {
         throw err
       }
     }
 
-    const newFiles = (await git("ls-files", ["--other", "--exclude-standard", path])).split("\n")
+    const newFiles = (await git("ls-files", "--other", "--exclude-standard", path))
+      .split("\n")
+      .filter((f) => f.length > 0)
+      .map(file => resolve(path, file))
 
-    const dirtyFiles: string[] = modifiedFiles.concat(newFiles).filter((f) => f.length > 0)
-
-    // for dirty trees, we append the last modified time of last modified or added file
-    if (dirtyFiles.length) {
-      const repoRoot = await git("rev-parse", ["--show-toplevel"])
-      const stats = await Bluebird.map(dirtyFiles, file => join(repoRoot, file))
-        .filter((file: string) => pathExists(file))
-        .map((file: string) => stat(file))
-
-      let mtimes = stats.map((s) => Math.round(s.mtime.getTime() / 1000))
-      let latest = mtimes.sort().slice(-1)[0]
-
-      if (latest > latestDirty) {
-        latestDirty = latest
-      }
-    }
-
-    return {
-      latestCommit: commitHash,
-      dirtyTimestamp: latestDirty || null,
-    }
+    return modifiedFiles.concat(newFiles)
   }
 
   // TODO Better auth handling
   async ensureRemoteSource({ url, name, log, sourceType }: RemoteSourceParams): Promise<string> {
     const remoteSourcesPath = join(this.projectRoot, this.getRemoteSourcesDirname(sourceType))
     await ensureDir(remoteSourcesPath)
-    const git = helpers.gitCli(remoteSourcesPath)
+    const git = this.gitCli(remoteSourcesPath)
 
     const absPath = join(this.projectRoot, this.getRemoteSourcePath(name, url, sourceType))
     const isCloned = await pathExists(absPath)
@@ -120,7 +107,7 @@ export class GitHandler extends VcsHandler {
       const { repositoryUrl, hash } = parseGitUrl(url)
 
       try {
-        await git("clone", ["--depth=1", `--branch=${hash}`, repositoryUrl, absPath])
+        await git("clone", "--depth=1", `--branch=${hash}`, repositoryUrl, absPath)
       } catch (err) {
         entry.setError()
         throw new RuntimeError(`Downloading remote ${sourceType} failed with error: \n\n${err}`, {
@@ -137,23 +124,23 @@ export class GitHandler extends VcsHandler {
 
   async updateRemoteSource({ url, name, sourceType, log }: RemoteSourceParams) {
     const absPath = join(this.projectRoot, this.getRemoteSourcePath(name, url, sourceType))
-    const git = helpers.gitCli(absPath)
+    const git = this.gitCli(absPath)
     const { repositoryUrl, hash } = parseGitUrl(url)
 
     await this.ensureRemoteSource({ url, name, sourceType, log })
 
     const entry = log.info({ section: name, msg: "Getting remote state", status: "active" })
-    await git("remote", ["update"])
+    await git("remote", "update")
 
-    const remoteCommitId = getCommitIdFromRefList(await git("ls-remote", [repositoryUrl, hash]))
-    const localCommitId = getCommitIdFromRefList(await git("show-ref", ["--hash", hash]))
+    const remoteCommitId = getCommitIdFromRefList(await git("ls-remote", repositoryUrl, hash))
+    const localCommitId = getCommitIdFromRefList(await git("show-ref", "--hash", hash))
 
     if (localCommitId !== remoteCommitId) {
       entry.setState(`Fetching from ${url}`)
 
       try {
-        await git("fetch", ["--depth=1", "origin", hash])
-        await git("reset", ["--hard", `origin/${hash}`])
+        await git("fetch", "--depth=1", "origin", hash)
+        await git("reset", "--hard", `origin/${hash}`)
       } catch (err) {
         entry.setError()
         throw new RuntimeError(`Updating remote ${sourceType} failed with error: \n\n${err}`, {

garden-service/test/helpers.ts

@@ -33,7 +33,6 @@ import {
   RunTaskParams,
   SetSecretParams,
 } from "../src/types/plugin/params"
-import { helpers } from "../src/vcs/git"
 import { ModuleVersion } from "../src/vcs/base"
 import { GARDEN_DIR_NAME } from "../src/constants"
 import { EventBus, Events } from "../src/events"
@@ -365,16 +364,16 @@ export const cleanProject = async (projectRoot: string) => {
   return remove(join(projectRoot, GARDEN_DIR_NAME))
 }
 
-export function stubGitCli() {
-  td.replace(helpers, "gitCli", () => async () => "")
+export function stubGitCli(garden: Garden) {
+  td.replace(garden.vcs, "gitCli", () => async () => "")
 }
 
 /**
  * Prevents git cloning. Use if creating a Garden instance with test-project-ext-module-sources
  * or test-project-ext-project-sources as project root.
  */
 export function stubExtSources(garden: Garden) {
-  stubGitCli()
+  stubGitCli(garden)
   const getRemoteSourcesDirname = td.replace(garden.vcs, "getRemoteSourcesDirname")
 
   td.when(getRemoteSourcesDirname("module")).thenReturn(join("mock-dot-garden", "sources", "module"))

garden-service/test/src/commands/update-remote.ts

@@ -24,7 +24,7 @@ describe("UpdateRemoteCommand", () => {
     beforeEach(async () => {
       garden = await makeTestGarden(projectRoot)
       log = garden.log
-      stubGitCli()
+      stubGitCli(garden)
     })
 
     const projectRoot = getDataDir("test-project-ext-project-sources")

garden-service/test/src/garden.ts

@@ -194,7 +194,7 @@ describe("Garden", () => {
     it("should resolve module path to external sources dir if module has a remote source", async () => {
       const projectRoot = resolve(dataDir, "test-project-ext-module-sources")
       const garden = await makeTestGarden(projectRoot)
-      stubGitCli()
+      stubGitCli(garden)
 
       const module = (await (<any>garden).loadModuleConfigs("./module-a"))[0]
       const repoUrlHash = hashRepoUrl(module!.repositoryUrl!)
@@ -260,46 +260,48 @@ describe("Garden", () => {
   describe("loadExtSourcePath", () => {
     let projectRoot: string
 
-    const makeGardenContext = async (root) => {
-      const ctx = await makeTestGarden(root)
-      stubGitCli()
-      return ctx
+    const makeGarden = async (root) => {
+      const garden = await makeTestGarden(root)
+      stubGitCli(garden)
+      return garden
     }
 
     afterEach(async () => {
-      await cleanProject(projectRoot)
+      if (projectRoot) {
+        await cleanProject(projectRoot)
+      }
     })
 
     it("should return the path to the project source if source type is project", async () => {
       projectRoot = getDataDir("test-project-ext-project-sources")
-      const ctx = await makeGardenContext(projectRoot)
+      const garden = await makeGarden(projectRoot)
       const repositoryUrl = "https://github.com/org/repo.git#master"
-      const path = await ctx.loadExtSourcePath({ repositoryUrl, name: "source-a", sourceType: "project" })
+      const path = await garden.loadExtSourcePath({ repositoryUrl, name: "source-a", sourceType: "project" })
       const repoUrlHash = hashRepoUrl(repositoryUrl)
       expect(path).to.equal(join(projectRoot, ".garden", "sources", "project", `source-a--${repoUrlHash}`))
     })
 
     it("should return the path to the module source if source type is module", async () => {
       projectRoot = getDataDir("test-project-ext-module-sources")
-      const ctx = await makeGardenContext(projectRoot)
+      const garden = await makeGarden(projectRoot)
       const repositoryUrl = "https://github.com/org/repo.git#master"
-      const path = await ctx.loadExtSourcePath({ repositoryUrl, name: "module-a", sourceType: "module" })
+      const path = await garden.loadExtSourcePath({ repositoryUrl, name: "module-a", sourceType: "module" })
       const repoUrlHash = hashRepoUrl(repositoryUrl)
       expect(path).to.equal(join(projectRoot, ".garden", "sources", "module", `module-a--${repoUrlHash}`))
     })
 
     it("should return the local path of the project source if linked", async () => {
       projectRoot = getDataDir("test-project-ext-project-sources")
-      const ctx = await makeGardenContext(projectRoot)
+      const garden = await makeGarden(projectRoot)
       const localPath = join(projectRoot, "mock-local-path", "source-a")
 
       const linked: LinkedSource[] = [{
         name: "source-a",
         path: localPath,
       }]
-      await ctx.localConfigStore.set(["linkedProjectSources"], linked)
+      await garden.localConfigStore.set(["linkedProjectSources"], linked)
 
-      const path = await ctx.loadExtSourcePath({
+      const path = await garden.loadExtSourcePath({
         name: "source-a",
         repositoryUrl: "https://github.com/org/repo.git#master",
         sourceType: "project",
@@ -310,16 +312,16 @@ describe("Garden", () => {
 
     it("should return the local path of the module source if linked", async () => {
       projectRoot = getDataDir("test-project-ext-module-sources")
-      const ctx = await makeGardenContext(projectRoot)
+      const garden = await makeGarden(projectRoot)
       const localPath = join(projectRoot, "mock-local-path", "module-a")
 
       const linked: LinkedSource[] = [{
         name: "module-a",
         path: localPath,
       }]
-      await ctx.localConfigStore.set(["linkedModuleSources"], linked)
+      await garden.localConfigStore.set(["linkedModuleSources"], linked)
 
-      const path = await ctx.loadExtSourcePath({
+      const path = await garden.loadExtSourcePath({
         name: "module-a",
         repositoryUrl: "https://github.com/org/repo.git#master",
         sourceType: "module",

garden-service/test/src/vcs/base.ts

@@ -7,6 +7,14 @@ class TestVcsHandler extends VcsHandler {
   name = "test"
   private testVersions: TreeVersions = {}
 
+  async getLatestCommit() {
+    return NEW_MODULE_VERSION
+  }
+
+  async getDirtyFiles() {
+    return []
+  }
+
   async getTreeVersion(path: string) {
     return this.testVersions[path] || {
       latestCommit: NEW_MODULE_VERSION,
@@ -18,12 +26,12 @@ class TestVcsHandler extends VcsHandler {
     this.testVersions[path] = version
   }
 
-  async ensureRemoteSource(_params): Promise<string> {
+  async ensureRemoteSource(): Promise<string> {
     return ""
   }
 
-  async updateRemoteSource(_params) {
-    return ""
+  async updateRemoteSource() {
+    return
   }
 
 }