Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: preserve merge states in submodules #769

Merged
merged 4 commits into from Jan 20, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 4 additions & 5 deletions lib/gitWorkflow.js
Expand Up @@ -37,20 +37,19 @@ 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

/**
* 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)
}

/**
Expand Down
16 changes: 0 additions & 16 deletions lib/resolveGitDir.js

This file was deleted.

45 changes: 45 additions & 0 deletions lib/resolveGitRepo.js
@@ -0,0 +1,45 @@
'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(dotGit.replace(/^gitdir: /, ''))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it does not work properly because of this line. It takes path relative to node_modules or package.json (not sure), and from this going up a directory. I think it should be something like
const gitConfigDir = path.resolve(gitDir + '/' + dotGit.replace(/^gitdir: /, ''))
but probably better written

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. I tested it myself locally and the path in .git seems to be relative to the gitDir, so that would be better indeed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more thing, in my case
dotGit.replace(/^gitdir: /, '') contains new line at the end, so it tries to read from
path/path/path
MERGE_MSG
when i add .trim() after replace it reads from proper path.

With this change and above change to path it works great in my case 🥇

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those following newlines could use some trimming in other places as well, let me fix... thanks for testing!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Maybe a re-test just in case?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great now

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

const gitDir = await execGit(['rev-parse', '--show-toplevel'], options)
const superprojectWorkingTree = await execGit(
['rev-parse', '--show-superproject-working-tree'],
options
)
// A super-project working tree exists only in the context of submodules
const isSubmodule = !!superprojectWorkingTree
const gitConfigDir = await resolveGitConfigDir({ gitDir, isSubmodule })

return { gitDir: normalize(gitDir), gitConfigDir, isSubmodule }
} catch (error) {
return { error, gitDir: null }
}
}
24 changes: 8 additions & 16 deletions lib/runAll.js
Expand Up @@ -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')

Expand Down Expand Up @@ -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`
Expand Down Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions test/generateTasks.spec.js
Expand Up @@ -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)

Expand All @@ -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 = {
Expand Down
13 changes: 7 additions & 6 deletions 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(() => {
Expand All @@ -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,
Expand All @@ -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,
Expand Down
19 changes: 11 additions & 8 deletions test/resolveGitDir.spec.js → 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
})

Expand All @@ -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)
})
})
10 changes: 7 additions & 3 deletions 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
Expand Down
39 changes: 38 additions & 1 deletion test/runAll.unmocked.spec.js
Expand Up @@ -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']) => {
Expand Down Expand Up @@ -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)
})
})