Repository: okonet/lint-staged
Pull request #769 (2020-01-20 18:07:29)

fix: preserve merge states in submodules
This PR fixes the behaviour of lint-staged during merge conflicts in git submodules. The issue was that in a git submodule, the `.git` directory is actually a file containing a reference to the actual directory (that is located inside the parent repo's `.git` directory).

The fix includes detecting whether the current repo is a submodule by running `git rev-parse --show-superproject-working-tree`, which will only output a path when run inside a submodule. When this is true, the true `.git` location is read from the submodule's file. This resolved path is then passed to the main `gitWorkflow` instead of it assuming that the `.git` directory is always at the root of the repo/submodule.

Fixes https://github.com/okonet/lint-staged/issues/768

Thanks to @armata007 for the bug report.

codecov[bot]: # [Codecov](https://codecov.io/gh/okonet/lint-staged/pull/769?src=pr&el=h1) Report
> Merging [#769](https://codecov.io/gh/okonet/lint-staged/pull/769?src=pr&el=desc) into [master](https://codecov.io/gh/okonet/lint-staged/commit/072924fce037c726112c00a6207afa92e238a267?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/okonet/lint-staged/pull/769/graphs/tree.svg?width=650&token=LSoEgsjk4O&height=150&src=pr)](https://codecov.io/gh/okonet/lint-staged/pull/769?src=pr&el=tree)

```diff
@@          Coverage Diff          @@
##           master   #769   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines         429    439   +10     
  Branches       97     99    +2     
=====================================
+ Hits          429    439   +10
```


| [Impacted Files](https://codecov.io/gh/okonet/lint-staged/pull/769?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [lib/runAll.js](https://codecov.io/gh/okonet/lint-staged/pull/769/diff?src=pr&el=tree#diff-bGliL3J1bkFsbC5qcw==) | `100% <100%> (ø)` | :arrow_up: |
| [lib/resolveGitRepo.js](https://codecov.io/gh/okonet/lint-staged/pull/769/diff?src=pr&el=tree#diff-bGliL3Jlc29sdmVHaXRSZXBvLmpz) | `100% <100%> (ø)` | |
| [lib/gitWorkflow.js](https://codecov.io/gh/okonet/lint-staged/pull/769/diff?src=pr&el=tree#diff-bGliL2dpdFdvcmtmbG93Lmpz) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/okonet/lint-staged/pull/769?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/okonet/lint-staged/pull/769?src=pr&el=footer). Last update [072924f...d701f73](https://codecov.io/gh/okonet/lint-staged/pull/769?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/lib/gitWorkflow.js b/lib/gitWorkflow.js
index 6fd01ae..25c9477 100644
--- a/lib/gitWorkflow.js
+++ b/lib/gitWorkflow.js
@@ -37,10 +37,9 @@ const handleError = (error, ctx) => {
 }
 
 class GitWorkflow {
-  constructor({ allowEmpty, gitDir, stagedFileChunks }) {
+  constructor({ allowEmpty, gitConfigDir, gitDir, stagedFileChunks }) {
     this.execGit = (args, options = {}) => execGit(args, { ...options, cwd: gitDir })
     this.unstagedDiff = null
-    this.gitDir = gitDir
     this.allowEmpty = allowEmpty
     this.stagedFileChunks = stagedFileChunks
 
@@ -48,9 +47,9 @@ class GitWorkflow {
      * These three files hold state about an ongoing git merge
      * Resolve paths during constructor
      */
-    this.mergeHeadFilename = path.resolve(this.gitDir, '.git', MERGE_HEAD)
-    this.mergeModeFilename = path.resolve(this.gitDir, '.git', MERGE_MODE)
-    this.mergeMsgFilename = path.resolve(this.gitDir, '.git', MERGE_MSG)
+    this.mergeHeadFilename = path.resolve(gitConfigDir, MERGE_HEAD)
+    this.mergeModeFilename = path.resolve(gitConfigDir, MERGE_MODE)
+    this.mergeMsgFilename = path.resolve(gitConfigDir, MERGE_MSG)
   }
 
   /**
@@ -196,7 +195,7 @@ class GitWorkflow {
     try {
       const backupStash = await this.getBackupStash(ctx)
       const output = await this.execGit(['show', '--format=%b', `${backupStash}^3`])
-      const untrackedDiff = output.replace(/^\n*/, '') // remove empty lines from start of output
+      const untrackedDiff = typeof output === 'string' && output.trim() // remove empty lines from start of output
       if (!untrackedDiff) return
       await this.execGit([...gitApplyArgs], { input: `${untrackedDiff}\n` })
     } catch (err) {} // eslint-disable-line no-empty
diff --git a/lib/resolveGitDir.js b/lib/resolveGitDir.js
deleted file mode 100644
index cf04c4c..0000000
--- a/lib/resolveGitDir.js
+++ /dev/null
@@ -1,16 +0,0 @@
-'use strict'
-
-const execGit = require('./execGit')
-const normalize = require('normalize-path')
-
-module.exports = async function resolveGitDir(options = {}) {
-  try {
-    // git cli uses GIT_DIR to fast track its response however it might be set to a different path
-    // depending on where the caller initiated this from, hence clear GIT_DIR
-    delete process.env.GIT_DIR
-    const gitDir = await execGit(['rev-parse', '--show-toplevel'], options)
-    return normalize(gitDir)
-  } catch (error) {
-    return null
-  }
-}
diff --git a/lib/resolveGitRepo.js b/lib/resolveGitRepo.js
new file mode 100644
index 0000000..5ee5d41
--- /dev/null
+++ b/lib/resolveGitRepo.js
@@ -0,0 +1,48 @@
+'use strict'
+
+const normalize = require('normalize-path')
+const path = require('path')
+
+const execGit = require('./execGit')
+const { readBufferFromFile } = require('./file')
+
+/**
+ * Resolve path to the .git directory, with special handling for
+ * submodules
+ */
+const resolveGitConfigDir = async ({ gitDir, isSubmodule }) => {
+  const defaultDir = path.resolve(gitDir, '.git')
+  if (!isSubmodule) return normalize(defaultDir)
+
+  const buffer = await readBufferFromFile(defaultDir)
+  const dotGit = buffer.toString()
+  const gitConfigDir = path.resolve(gitDir, dotGit.replace(/^gitdir: /, '').trim())
+  return normalize(gitConfigDir)
+}
+
+/**
+ * Resolve git directory and possible submodule paths
+ */
+module.exports = async function resolveGitRepo(options = {}) {
+  try {
+    // git cli uses GIT_DIR to fast track its response however it might be set to a different path
+    // depending on where the caller initiated this from, hence clear GIT_DIR
+    delete process.env.GIT_DIR
+
+    // The git repo root directory; this points to the root of a submodule instead of the parent
+    const gitDir = await execGit(['rev-parse', '--show-toplevel'], options)
+
+    // A super-project working tree exists only in submodules; poinst to the parent root
+    const superprojectWorkingTree = await execGit(
+      ['rev-parse', '--show-superproject-working-tree'],
+      options
+    )
+
+    const isSubmodule = !!superprojectWorkingTree
+    const gitConfigDir = await resolveGitConfigDir({ gitDir, isSubmodule })
+
+    return { gitDir: normalize(gitDir), gitConfigDir, isSubmodule }
+  } catch (error) {
+    return { error, gitDir: null }
+  }
+}
diff --git a/lib/runAll.js b/lib/runAll.js
index 3203fc1..10ce9a2 100644
--- a/lib/runAll.js
+++ b/lib/runAll.js
@@ -11,7 +11,7 @@ const generateTasks = require('./generateTasks')
 const getStagedFiles = require('./getStagedFiles')
 const GitWorkflow = require('./gitWorkflow')
 const makeCmdTasks = require('./makeCmdTasks')
-const resolveGitDir = require('./resolveGitDir')
+const resolveGitRepo = require('./resolveGitRepo')
 
 const debugLog = require('debug')('lint-staged:run')
 
@@ -47,27 +47,19 @@ module.exports = async function runAll(
 ) {
   debugLog('Running all linter scripts')
 
-  const gitDir = await resolveGitDir({ cwd })
-
-  if (!gitDir) {
-    throw new Error('Current directory is not a git directory!')
-  }
-
+  const { gitDir, gitConfigDir, isSubmodule } = await resolveGitRepo({ cwd })
+  if (!gitDir) throw new Error('Current directory is not a git directory!')
   debugLog('Resolved git directory to be `%s`', gitDir)
+  debugLog('Resolved git config directory to be `%s`', gitConfigDir)
+  if (isSubmodule) debugLog('Current git directory is a submodule')
 
   const files = await getStagedFiles({ cwd: gitDir })
-
-  if (!files) {
-    throw new Error('Unable to get staged files!')
-  }
-
+  if (!files) throw new Error('Unable to get staged files!')
   debugLog('Loaded list of staged files in git:\n%O', files)
 
   const stagedFileChunks = chunkFiles({ files, gitDir, maxArgLength, relative })
   const chunkCount = stagedFileChunks.length
-  if (chunkCount > 1) {
-    debugLog(`Chunked staged files into ${chunkCount} part`, chunkCount)
-  }
+  if (chunkCount > 1) debugLog(`Chunked staged files into ${chunkCount} part`, chunkCount)
 
   // lint-staged 10 will automatically add modifications to index
   // Warn user when their command includes `git add`
@@ -146,7 +138,7 @@ module.exports = async function runAll(
     return 'No tasks to run.'
   }
 
-  const git = new GitWorkflow({ allowEmpty, gitDir, stagedFileChunks })
+  const git = new GitWorkflow({ allowEmpty, gitConfigDir, gitDir, stagedFileChunks })
 
   // Running git reset or dropping the backup stash should be skipped
   // when there are git errors NOT related to applying unstaged modifications.


Test patch:
diff --git a/test/generateTasks.spec.js b/test/generateTasks.spec.js
index 6a6d5c7..08c5acf 100644
--- a/test/generateTasks.spec.js
+++ b/test/generateTasks.spec.js
@@ -3,7 +3,7 @@ import normalize from 'normalize-path'
 import path from 'path'
 
 import generateTasks from '../lib/generateTasks'
-import resolveGitDir from '../lib/resolveGitDir'
+import resolveGitRepo from '../lib/resolveGitRepo'
 
 const normalizePath = path => normalize(path)
 
@@ -28,9 +28,9 @@ const files = [
 ]
 
 // Mocks get hoisted
-jest.mock('../lib/resolveGitDir.js')
+jest.mock('../lib/resolveGitRepo.js')
 const gitDir = path.join(os.tmpdir(), 'tmp-lint-staged')
-resolveGitDir.mockResolvedValue(gitDir)
+resolveGitRepo.mockResolvedValue({ gitDir })
 const cwd = gitDir
 
 const config = {
diff --git a/test/index2.spec.js b/test/index2.spec.js
index 99d2a1d..7ae02a1 100644
--- a/test/index2.spec.js
+++ b/test/index2.spec.js
@@ -1,14 +1,15 @@
 import Listr from 'listr'
+import makeConsoleMock from 'consolemock'
 import path from 'path'
 
-// silence console from Jest output
-console.log = jest.fn(() => {})
-console.error = jest.fn(() => {})
-
 jest.mock('listr')
+jest.mock('../lib/resolveGitRepo')
 
 // eslint-disable-next-line import/first
 import lintStaged from '../lib/index'
+import resolveGitRepo from '../lib/resolveGitRepo'
+
+resolveGitRepo.mockImplementation(async () => ({ gitDir: 'foo', gitConfigDir: 'bar' }))
 
 describe('lintStaged', () => {
   afterEach(() => {
@@ -19,7 +20,7 @@ describe('lintStaged', () => {
     expect.assertions(1)
     await lintStaged(
       { configPath: path.join(__dirname, '__mocks__', 'my-config.json'), quiet: true },
-      console
+      makeConsoleMock()
     )
     expect(Listr.mock.calls[0][1]).toEqual({
       dateFormat: false,
@@ -35,7 +36,7 @@ describe('lintStaged', () => {
         configPath: path.join(__dirname, '__mocks__', 'my-config.json'),
         debug: true
       },
-      console
+      makeConsoleMock()
     )
     expect(Listr.mock.calls[0][1]).toEqual({
       dateFormat: false,
diff --git a/test/resolveGitDir.spec.js b/test/resolveGitRepo.spec.js
similarity index 64%
rename from test/resolveGitDir.spec.js
rename to test/resolveGitRepo.spec.js
index ad09654..11666bd 100644
--- a/test/resolveGitDir.spec.js
+++ b/test/resolveGitRepo.spec.js
@@ -1,24 +1,26 @@
 import normalize from 'normalize-path'
 import path from 'path'
 
-import resolveGitDir from '../lib/resolveGitDir'
+import resolveGitRepo from '../lib/resolveGitRepo'
 
 /**
- * resolveGitDir runs execa, so the mock needs to be disabled for these tests
+ * resolveGitRepo runs execa, so the mock needs to be disabled for these tests
  */
 jest.unmock('execa')
 
-describe('resolveGitDir', () => {
+describe('resolveGitRepo', () => {
   it('should resolve to current working dir when .git is in the same dir', async () => {
-    const expected = normalize(process.cwd())
-    expect(await resolveGitDir()).toEqual(expected)
+    const cwd = normalize(process.cwd())
+    const { gitDir } = await resolveGitRepo()
+    expect(gitDir).toEqual(cwd)
   })
 
   it('should resolve to the parent dir when .git is in the parent dir', async () => {
     const expected = normalize(path.dirname(__dirname))
     const processCwdBkp = process.cwd
     process.cwd = () => __dirname
-    expect(await resolveGitDir()).toEqual(expected)
+    const { gitDir } = await resolveGitRepo()
+    expect(gitDir).toEqual(expected)
     process.cwd = processCwdBkp
   })
 
@@ -27,12 +29,13 @@ describe('resolveGitDir', () => {
     const processCwdBkp = process.cwd
     process.cwd = () => __dirname
     process.env.GIT_DIR = 'wrong/path/.git' // refer to https://github.com/DonJayamanne/gitHistoryVSCode/issues/233#issuecomment-375769718
-    expect(await resolveGitDir()).toEqual(expected)
+    const { gitDir } = await resolveGitRepo()
+    expect(gitDir).toEqual(expected)
     process.cwd = processCwdBkp
   })
 
   it('should return null when not in a git directory', async () => {
-    const gitDir = await resolveGitDir({ cwd: '/' }) // assume root is not a git directory
+    const { gitDir } = await resolveGitRepo({ cwd: '/' }) // assume root is not a git directory
     expect(gitDir).toEqual(null)
   })
 })
diff --git a/test/runAll.spec.js b/test/runAll.spec.js
index c5d32b9..7a89aeb 100644
--- a/test/runAll.spec.js
+++ b/test/runAll.spec.js
@@ -1,16 +1,20 @@
 import makeConsoleMock from 'consolemock'
 import execa from 'execa'
 import normalize from 'normalize-path'
+import path from 'path'
 
-import resolveGitDir from '../lib/resolveGitDir'
+import resolveGitRepo from '../lib/resolveGitRepo'
 import getStagedFiles from '../lib/getStagedFiles'
 import runAll from '../lib/runAll'
 
-jest.mock('../lib/resolveGitDir')
+jest.mock('../lib/resolveGitRepo')
 jest.mock('../lib/getStagedFiles')
 jest.mock('../lib/gitWorkflow')
 
-resolveGitDir.mockImplementation(async () => normalize(process.cwd()))
+resolveGitRepo.mockImplementation(async () => {
+  const cwd = process.cwd()
+  return { gitConfigDir: normalize(path.resolve(cwd, '.git')), gitDir: normalize(cwd) }
+})
 getStagedFiles.mockImplementation(async () => [])
 
 const globalConsoleTemp = console
diff --git a/test/runAll.unmocked.spec.js b/test/runAll.unmocked.spec.js
index eb73049..ea197ff 100644
--- a/test/runAll.unmocked.spec.js
+++ b/test/runAll.unmocked.spec.js
@@ -66,7 +66,7 @@ const writeFile = async (filename, content, dir = cwd) =>
   fs.writeFile(path.resolve(dir, filename), content)
 
 // Wrap execGit to always pass `gitOps`
-const execGit = async args => execGitBase(args, { cwd })
+const execGit = async (args, options = {}) => execGitBase(args, { cwd, ...options })
 
 // Execute runAll before git commit to emulate lint-staged
 const gitCommit = async (options, args = ['-m test']) => {
@@ -242,7 +242,7 @@ describe('runAll', () => {
 
     // Latest commit contains pretty file
     // `git show` strips empty line from here here
-    expect(await execGit(['show', 'HEAD:test.js'])).toEqual(testJsFilePretty.replace(/\n$/, ''))
+    expect(await execGit(['show', 'HEAD:test.js'])).toEqual(testJsFilePretty.trim())
 
     // Since edit was not staged, the file is still modified
     const status = await execGit(['status'])
@@ -269,7 +269,7 @@ describe('runAll', () => {
 
     // Latest commit contains pretty file
     // `git show` strips empty line from here here
-    expect(await execGit(['show', 'HEAD:test.js'])).toEqual(testJsFilePretty.replace(/\n$/, ''))
+    expect(await execGit(['show', 'HEAD:test.js'])).toEqual(testJsFilePretty.trim())
 
     // Nothing is staged
     const status = await execGit(['status'])
@@ -346,7 +346,7 @@ describe('runAll', () => {
 
     // Latest commit contains pretty file
     // `git show` strips empty line from here here
-    expect(await execGit(['show', 'HEAD:test.js'])).toEqual(testJsFilePretty.replace(/\n$/, ''))
+    expect(await execGit(['show', 'HEAD:test.js'])).toEqual(testJsFilePretty.trim())
 
     // Nothing is staged
     expect(await execGit(['status'])).toMatch('nothing to commit, working tree clean')
@@ -735,4 +735,41 @@ describe('runAll', () => {
     expect(await execGit(['diff', '-1'])).toEqual('')
     expect(await readFile('test.js')).toEqual(testJsFilePretty)
   })
+
+  it('should handle git submodules', async () => {
+    // create a new repo for the git submodule to a temp path
+    let submoduleDir = path.resolve(cwd, 'submodule-temp')
+    await fs.ensureDir(submoduleDir)
+    await execGit('init', { cwd: submoduleDir })
+    await execGit(['config', 'user.name', '"test"'], { cwd: submoduleDir })
+    await execGit(['config', 'user.email', '"test@test.com"'], { cwd: submoduleDir })
+    await appendFile('README.md', '# Test\n', submoduleDir)
+    await execGit(['add', 'README.md'], { cwd: submoduleDir })
+    await execGit(['commit', '-m initial commit'], { cwd: submoduleDir })
+
+    // Add the newly-created repo as a submodule in a new path.
+    // This simulates adding it from a remote
+    await execGit(['submodule', 'add', '--force', './submodule-temp', './submodule'])
+    submoduleDir = path.resolve(cwd, 'submodule')
+    // Set these again for Windows git in CI
+    await execGit(['config', 'user.name', '"test"'], { cwd: submoduleDir })
+    await execGit(['config', 'user.email', '"test@test.com"'], { cwd: submoduleDir })
+
+    // Stage pretty file
+    await appendFile('test.js', testJsFilePretty, submoduleDir)
+    await execGit(['add', 'test.js'], { cwd: submoduleDir })
+
+    // Run lint-staged with `prettier --list-different` and commit pretty file
+    await runAll({
+      config: { '*.js': 'prettier --list-different' },
+      cwd: submoduleDir,
+      quiet: true
+    })
+    await execGit(['commit', '-m test'], { cwd: submoduleDir })
+
+    // Nothing is wrong, so a new commit is created
+    expect(await execGit(['rev-list', '--count', 'HEAD'], { cwd: submoduleDir })).toEqual('2')
+    expect(await execGit(['log', '-1', '--pretty=%B'], { cwd: submoduleDir })).toMatch('test')
+    expect(await readFile('test.js', submoduleDir)).toEqual(testJsFilePretty)
+  })
 })