Merge pull request #6533 from Shopify/handle-git-errors-in-cloning-gracefully

Fix: Validate directory before git clone in theme init (#21372)

Author: amcaplan

.changeset/clean-dodos-double.md

@@ -0,0 +1,5 @@
+---
+'@shopify/cli-kit': patch
+---
+
+Fix: Validate directory before `git clone` attempts

packages/cli-kit/src/public/node/git.test.ts

@@ -1,5 +1,14 @@
 import * as git from './git.js'
-import {appendFileSync, fileExistsSync, inTemporaryDirectory, readFileSync, writeFileSync} from './fs.js'
+import {
+  appendFileSync,
+  fileExists,
+  fileExistsSync,
+  glob,
+  inTemporaryDirectory,
+  isDirectory,
+  readFileSync,
+  writeFileSync,
+} from './fs.js'
 import {hasGit} from './context/local.js'
 import {beforeEach, describe, expect, test, vi} from 'vitest'
 import simpleGit from 'simple-git'
@@ -38,6 +47,9 @@ vi.mock('./fs.js', async () => {
   return {
     ...fs,
     appendFileSync: vi.fn(),
+    fileExists: vi.fn(),
+    isDirectory: vi.fn(),
+    glob: vi.fn(),
   }
 })
 
@@ -151,6 +163,83 @@ describe('downloadRepository()', async () => {
     expect(mockedClone).toHaveBeenCalledWith('http://repoUrl', destination, options)
     expect(mockCheckout).toHaveBeenCalledWith(expectedLatestTag)
   })
+
+  test('throws when destination exists as a file', async () => {
+    await expect(async () => {
+      // Given
+      const repoUrl = 'http://repoUrl'
+      const destination = 'destination'
+      vi.mocked(fileExists).mockResolvedValue(true)
+      vi.mocked(isDirectory).mockResolvedValue(false)
+
+      // When
+      await git.downloadGitRepository({repoUrl, destination})
+
+      // Then
+    }).rejects.toThrowError(/Can't clone to/)
+  })
+
+  test('throws when destination directory is not empty', async () => {
+    await expect(async () => {
+      // Given
+      const repoUrl = 'http://repoUrl'
+      const destination = 'destination'
+      vi.mocked(fileExists).mockResolvedValue(true)
+      vi.mocked(isDirectory).mockResolvedValue(true)
+      vi.mocked(glob).mockResolvedValue(['file1.txt', 'file2.txt'])
+
+      // When
+      await git.downloadGitRepository({repoUrl, destination})
+
+      // Then
+    }).rejects.toThrowError(/already exists and is not empty/)
+  })
+
+  test('throws when destination contains only hidden files', async () => {
+    await expect(async () => {
+      // Given
+      const repoUrl = 'http://repoUrl'
+      const destination = 'destination'
+      vi.mocked(fileExists).mockResolvedValue(true)
+      vi.mocked(isDirectory).mockResolvedValue(true)
+      vi.mocked(glob).mockResolvedValue(['.git', '.DS_Store'])
+
+      // When
+      await git.downloadGitRepository({repoUrl, destination})
+
+      // Then
+    }).rejects.toThrowError(/already exists and is not empty/)
+  })
+
+  test('succeeds when destination directory is empty', async () => {
+    // Given
+    const repoUrl = 'http://repoUrl'
+    const destination = 'destination'
+    const options: any = {'--recurse-submodules': null}
+    vi.mocked(fileExists).mockResolvedValue(true)
+    vi.mocked(isDirectory).mockResolvedValue(true)
+    vi.mocked(glob).mockResolvedValue([])
+
+    // When
+    await git.downloadGitRepository({repoUrl, destination})
+
+    // Then
+    expect(mockedClone).toHaveBeenCalledWith(repoUrl, destination, options)
+  })
+
+  test('succeeds when destination does not exist', async () => {
+    // Given
+    const repoUrl = 'http://repoUrl'
+    const destination = 'destination'
+    const options: any = {'--recurse-submodules': null}
+    vi.mocked(fileExists).mockResolvedValue(false)
+
+    // When
+    await git.downloadGitRepository({repoUrl, destination})
+
+    // Then
+    expect(mockedClone).toHaveBeenCalledWith(repoUrl, destination, options)
+  })
 })
 
 describe('initializeRepository()', () => {

packages/cli-kit/src/public/node/git.ts

@@ -1,6 +1,15 @@
 /* eslint-disable @typescript-eslint/no-base-to-string */
 import {hasGit, isTerminalInteractive} from './context/local.js'
-import {appendFileSync, detectEOL, fileExistsSync, readFileSync, writeFileSync} from './fs.js'
+import {
+  appendFileSync,
+  detectEOL,
+  fileExists,
+  fileExistsSync,
+  glob,
+  isDirectory,
+  readFileSync,
+  writeFileSync,
+} from './fs.js'
 import {AbortError} from './error.js'
 import {cwd, joinPath} from './path.js'
 import {runWithTimer} from './metadata.js'
@@ -129,6 +138,32 @@ export async function downloadGitRepository(cloneOptions: GitCloneOptions): Prom
     const {repoUrl, destination, progressUpdater, shallow, latestTag} = cloneOptions
     outputDebug(outputContent`Git-cloning repository ${repoUrl} into ${outputToken.path(destination)}...`)
     await ensureGitIsPresentOrAbort()
+
+    // Validate destination directory before attempting to clone
+    if (await fileExists(destination)) {
+      // Check if it's a directory
+      if (!(await isDirectory(destination))) {
+        throw new AbortError(
+          outputContent`Can't clone to ${outputToken.path(destination)}`,
+          "The path exists but isn't a directory.",
+        )
+      }
+
+      // Check if directory is empty
+      const entries = await glob(['*', '.*'], {
+        cwd: destination,
+        deep: 1,
+        onlyFiles: false,
+      })
+
+      if (entries.length > 0) {
+        throw new AbortError(
+          outputContent`Directory ${outputToken.path(destination)} already exists and is not empty`,
+          outputContent`Choose a different name or remove the existing directory first.`,
+        )
+      }
+    }
+
     const [repository, branch] = repoUrl.split('#')
     const options: TaskOptions = {'--recurse-submodules': null}