Merge pull request #4905 from Shopify/lukeh/theme-pull-git-dir

Ensure git directory is clean when running `theme pull`

.changeset/red-tips-mix.md

@@ -0,0 +1,6 @@
+---
+'@shopify/cli-kit': patch
+'@shopify/theme': patch
+---
+
+Ensure git directory is clean when running `theme pull`

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

@@ -296,14 +296,32 @@ describe('ensureInsideGitDirectory()', () => {
     // Then
     await expect(git.ensureInsideGitDirectory()).resolves.toBeUndefined()
   })
+})
+
+describe('insideGitDirectory()', () => {
+  test('returns true if inside a git directory', async () => {
+    // Given
+    mockedCheckIsRepo.mockResolvedValue(true)
+
+    // Then
+    await expect(git.insideGitDirectory()).resolves.toBe(true)
+  })
+
+  test('returns false if not inside a git directory', async () => {
+    // Given
+    mockedCheckIsRepo.mockResolvedValue(false)
+
+    // Then
+    await expect(git.insideGitDirectory()).resolves.toBe(false)
+  })
 
   test('passes the directory option to simple git', async () => {
     // Given
     const directory = '/test/directory'
     mockedCheckIsRepo.mockResolvedValue(true)
 
     // When
-    await git.ensureInsideGitDirectory(directory)
+    await git.insideGitDirectory(directory)
 
     // Then
     expect(simpleGit).toHaveBeenCalledWith({baseDir: directory})

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

@@ -268,11 +268,23 @@ export class OutsideGitDirectoryError extends AbortError {}
 export async function ensureInsideGitDirectory(directory?: string): Promise<void> {
   // eslint-disable-next-line @typescript-eslint/ban-ts-comment
   // @ts-ignore
-  if (!(await git({baseDir: directory}).checkIsRepo())) {
+  if (!(await insideGitDirectory(directory))) {
     throw new OutsideGitDirectoryError(`${outputToken.path(directory || cwd())} is not a Git directory`)
   }
 }
 
+/**
+ * Returns true if the given directory is inside a .git directory tree.
+ *
+ * @param directory - The directory to check.
+ * @returns True if the directory is inside a .git directory tree.
+ */
+export async function insideGitDirectory(directory?: string): Promise<boolean> {
+  // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+  // @ts-ignore
+  return git({baseDir: directory}).checkIsRepo()
+}
+
 export class GitDirectoryNotCleanError extends AbortError {}
 /**
  * If the .git directory tree is not clean (has uncommitted changes)

packages/theme/src/cli/services/dev.ts

@@ -1,5 +1,5 @@
 import {hasRequiredThemeDirectories, mountThemeFileSystem} from '../utilities/theme-fs.js'
-import {currentDirectoryConfirmed} from '../utilities/theme-ui.js'
+import {ensureDirectoryConfirmed} from '../utilities/theme-ui.js'
 import {setupDevServer} from '../utilities/theme-environment/theme-environment.js'
 import {DevServerContext, LiveReload} from '../utilities/theme-environment/types.js'
 import {isStorefrontPasswordProtected} from '../utilities/theme-environment/storefront-session.js'
@@ -38,7 +38,7 @@ export interface DevOptions {
 }
 
 export async function dev(options: DevOptions) {
-  if (!(await hasRequiredThemeDirectories(options.directory)) && !(await currentDirectoryConfirmed(options.force))) {
+  if (!(await hasRequiredThemeDirectories(options.directory)) && !(await ensureDirectoryConfirmed(options.force))) {
     return
   }
 

packages/theme/src/cli/services/pull.test.ts

@@ -3,23 +3,27 @@ import {setThemeStore} from './local-storage.js'
 import {findOrSelectTheme} from '../utilities/theme-selector.js'
 import {ensureThemeStore} from '../utilities/theme-store.js'
 import {DevelopmentThemeManager} from '../utilities/development-theme-manager.js'
-import {mountThemeFileSystem} from '../utilities/theme-fs.js'
+import {hasRequiredThemeDirectories, mountThemeFileSystem} from '../utilities/theme-fs.js'
 import {fakeThemeFileSystem} from '../utilities/theme-fs/theme-fs-mock-factory.js'
 import {downloadTheme} from '../utilities/theme-downloader.js'
+import {themeComponent, ensureDirectoryConfirmed} from '../utilities/theme-ui.js'
 import {mkTmpDir, rmdir} from '@shopify/cli-kit/node/fs'
 import {buildTheme} from '@shopify/cli-kit/node/themes/factories'
 import {ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
 import {fetchChecksums} from '@shopify/cli-kit/node/themes/api'
+import {insideGitDirectory, isClean} from '@shopify/cli-kit/node/git'
 import {test, describe, expect, vi, beforeEach} from 'vitest'
 
 vi.mock('../utilities/theme-selector.js')
 vi.mock('../utilities/theme-store.js')
 vi.mock('../utilities/theme-fs.js')
 vi.mock('../utilities/theme-downloader.js')
+vi.mock('../utilities/theme-ui.js')
 vi.mock('@shopify/cli-kit/node/context/local')
 vi.mock('@shopify/cli-kit/node/session')
 vi.mock('@shopify/cli-kit/node/themes/api')
 vi.mock('@shopify/cli-kit/node/ui')
+vi.mock('@shopify/cli-kit/node/git')
 
 const adminSession = {token: '', storeFqdn: ''}
 const path = '/my-theme'
@@ -47,6 +51,7 @@ describe('pull', () => {
     vi.mocked(ensureAuthenticatedThemes).mockResolvedValue(adminSession)
     vi.mocked(mountThemeFileSystem).mockReturnValue(localThemeFileSystem)
     vi.mocked(fetchChecksums).mockResolvedValue([])
+    vi.mocked(themeComponent).mockReturnValue([])
     findDevelopmentThemeSpy.mockClear()
     fetchDevelopmentThemeSpy.mockClear()
   })
@@ -82,6 +87,40 @@ describe('pull', () => {
     )
   })
 
+  test('should ask for confirmation if the current directory is a Git directory and is not clean', async () => {
+    // Given
+    const theme = buildTheme({id: 1, name: 'Theme', role: 'development'})!
+    vi.mocked(insideGitDirectory).mockResolvedValue(true)
+    vi.mocked(isClean).mockResolvedValue(false)
+    vi.mocked(ensureDirectoryConfirmed).mockResolvedValue(false)
+
+    // When
+    await pull({...defaultFlags, theme: theme.id.toString()})
+
+    // Then
+    expect(vi.mocked(ensureDirectoryConfirmed)).toHaveBeenCalledWith(
+      false,
+      'The current Git directory has uncommitted changes. Do you want to proceed?',
+    )
+  })
+
+  test('should not ask for confirmation if --force flag is provided', async () => {
+    // Given
+    const theme = buildTheme({id: 1, name: 'Theme', role: 'development'})!
+    vi.mocked(findOrSelectTheme).mockResolvedValue(theme)
+    vi.mocked(fetchDevelopmentThemeSpy).mockResolvedValue(undefined)
+
+    // When
+    await pull({...defaultFlags, theme: theme.id.toString(), force: true})
+
+    // Then
+    expect(vi.mocked(findOrSelectTheme)).toHaveBeenCalledOnce()
+    expect(vi.mocked(hasRequiredThemeDirectories)).not.toHaveBeenCalled()
+    expect(vi.mocked(insideGitDirectory)).not.toHaveBeenCalled()
+    expect(vi.mocked(isClean)).not.toHaveBeenCalled()
+    expect(vi.mocked(ensureDirectoryConfirmed)).not.toHaveBeenCalled()
+  })
+
   describe('isEmptyDir', () => {
     test('returns true when directory is empty', async () => {
       // Given

packages/theme/src/cli/services/pull.ts

@@ -1,6 +1,6 @@
 import {downloadTheme} from '../utilities/theme-downloader.js'
 import {hasRequiredThemeDirectories, mountThemeFileSystem} from '../utilities/theme-fs.js'
-import {currentDirectoryConfirmed, themeComponent} from '../utilities/theme-ui.js'
+import {ensureDirectoryConfirmed, themeComponent} from '../utilities/theme-ui.js'
 import {rejectGeneratedStaticAssets} from '../utilities/asset-checksum.js'
 import {showEmbeddedCLIWarning} from '../utilities/embedded-cli-warning.js'
 import {ensureThemeStore} from '../utilities/theme-store.js'
@@ -13,6 +13,7 @@ import {fetchChecksums} from '@shopify/cli-kit/node/themes/api'
 import {renderSuccess} from '@shopify/cli-kit/node/ui'
 import {glob} from '@shopify/cli-kit/node/fs'
 import {cwd} from '@shopify/cli-kit/node/path'
+import {insideGitDirectory, isClean} from '@shopify/cli-kit/node/git'
 
 interface PullOptions {
   path: string
@@ -107,6 +108,10 @@ export async function pull(flags: PullFlags): Promise<void> {
 
   const {path, nodelete, live, development, only, ignore, force} = flags
 
+  if (!(await validateDirectory(path ?? cwd(), force ?? false))) {
+    return
+  }
+
   const theme = await findOrSelectTheme(adminSession, {
     header: 'Select a theme to open',
     filter: {
@@ -132,24 +137,7 @@ export async function pull(flags: PullFlags): Promise<void> {
  * @param options - the options that modify how the theme gets downloaded
  */
 async function executePull(theme: Theme, session: AdminSession, options: PullOptions) {
-  const path = options.path
-  const force = options.force
-
-  /**
-   * If users are not forcing the `pull` command, the directory is not empty,
-   * and the directory doesn't look like a theme directory, we ask for
-   * confirmation, because the `pull` command has the destructive behavior of
-   * removing local assets that are not present remotely.
-   */
-  if (
-    !(await isEmptyDir(path)) &&
-    !(await hasRequiredThemeDirectories(path)) &&
-    !(await currentDirectoryConfirmed(force))
-  ) {
-    return
-  }
-
-  const themeFileSystem = mountThemeFileSystem(path, {filters: options})
+  const themeFileSystem = mountThemeFileSystem(options.path, {filters: options})
   const [remoteChecksums] = await Promise.all([fetchChecksums(theme.id, session), themeFileSystem.ready()])
   const themeChecksums = rejectGeneratedStaticAssets(remoteChecksums)
 
@@ -196,3 +184,46 @@ export async function isEmptyDir(path: string) {
 
   return entries.length === 0
 }
+
+/**
+ * Validates the directory before pulling the theme.
+ *
+ * @param path - The path to the directory.
+ * @param force - Whether to force the pull operation.
+ * @returns Whether the directory is valid.
+ */
+async function validateDirectory(path: string, force: boolean) {
+  if (force) return true
+
+  /**
+   * If users are not forcing the `pull` command, the directory is not empty,
+   * and the directory doesn't look like a theme directory, we ask for
+   * confirmation, because the `pull` command has the destructive behavior of
+   * removing local assets that are not present remotely.
+   */
+  if (
+    !(await isEmptyDir(path)) &&
+    !(await hasRequiredThemeDirectories(path)) &&
+    !(await ensureDirectoryConfirmed(force))
+  ) {
+    return false
+  }
+
+  /**
+   * If users are not forcing the 'pull' command, and the current directory is a
+   * Git directory and it is not clean, we ask for confirmation before proceeding.
+   */
+  const dirtyDirectory = (await insideGitDirectory(path)) && !(await isClean(path))
+
+  if (
+    dirtyDirectory &&
+    !(await ensureDirectoryConfirmed(
+      force,
+      'The current Git directory has uncommitted changes. Do you want to proceed?',
+    ))
+  ) {
+    return false
+  }
+
+  return true
+}

packages/theme/src/cli/services/push.ts

@@ -1,7 +1,7 @@
 /* eslint-disable tsdoc/syntax */
 import {hasRequiredThemeDirectories, mountThemeFileSystem} from '../utilities/theme-fs.js'
 import {uploadTheme} from '../utilities/theme-uploader.js'
-import {currentDirectoryConfirmed, themeComponent} from '../utilities/theme-ui.js'
+import {ensureDirectoryConfirmed, themeComponent} from '../utilities/theme-ui.js'
 import {ensureThemeStore} from '../utilities/theme-store.js'
 import {DevelopmentThemeManager} from '../utilities/development-theme-manager.js'
 import {findOrSelectTheme} from '../utilities/theme-selector.js'
@@ -133,7 +133,7 @@ export async function push(flags: PushFlags): Promise<void> {
   const adminSession = await ensureAuthenticatedThemes(store, flags.password)
 
   const workingDirectory = path ? resolvePath(path) : cwd()
-  if (!(await hasRequiredThemeDirectories(workingDirectory)) && !(await currentDirectoryConfirmed(force))) {
+  if (!(await hasRequiredThemeDirectories(workingDirectory)) && !(await ensureDirectoryConfirmed(force))) {
     return
   }
 

packages/theme/src/cli/utilities/theme-ui.ts

@@ -16,12 +16,15 @@ export function themesComponent(themes: Theme[]) {
   return {list: {items}}
 }
 
-export async function currentDirectoryConfirmed(force: boolean) {
+export async function ensureDirectoryConfirmed(
+  force: boolean,
+  message = "It doesn't seem like you're running this command in a theme directory.",
+) {
   if (force) {
     return true
   }
 
-  renderWarning({body: `It doesn't seem like you're running this command in a theme directory.`})
+  renderWarning({body: message})
 
   if (!process.stdout.isTTY) {
     return true