fix: -d usage with -t different than HEAD

CONTRIBUTING.md

@@ -207,5 +207,5 @@ To test SGD as a Salesforce CLI plugin from a pending pull request:
 1. uninstall the previous version you may have `sfdx plugins:uninstall sfdx-git-delta`
 2. clone the repository
 3. checkout the branch to test
-3. run `sfdx plugins:link` from that local repository
-4. test the plugin!
+4. run `yarn pack`, followed by `sfdx plugins:link`, from that local repository
+5. test the plugin!

README.md

@@ -259,8 +259,19 @@ sfdx sgd:source:delta --to "HEAD" --from "HEAD^" --output changed-sources/ --gen
 In addition to the `package` and `destructiveChanges` folders, the `sfdx sgd:source:delta` command will also produce a copy of the added/changed files in the ouput folder.
 
 _Content of the output folder when using the --generate-delta option, with the same scenario as above:_
+
 ![delta-source](/img/example_generateDelta.png)
 
+> ⚠️ the `--generate-delta (-d)` can only be used when `--to (-t)` value is set to "HEAD" or to the "HEAD commit SHA".
+> If you need to use it with `--to (-t)` pointing to another commit than "HEAD", just checkout that commit first and then use `--generate-delta (-d)`. Exemple:
+>
+> ```sh
+> # move HEAD to past commit we are interested in
+> $ git checkout <not-HEAD-commit-sha>
+> # You can omit --to, it will take "HEAD" as default value
+> $ sfdx sgd:source:delta --from "HEAD^" --output changed-sources/ --generate-delta
+> ```
+
 ### Exclude some metadata only from destructiveChanges.xml:
 
 The `--ignore [-i]` parameter allows you to specify an [ignore file](https://git-scm.com/docs/gitignore) used to filter the

__tests__/integration/main.test.js

@@ -1,6 +1,9 @@
 'use strict'
 const app = require('../../src/main')
-jest.mock('child_process')
+const gc = require('../../src/utils/gitConstants')
+
+const child_process = require('child_process')
+jest.mock('child_process', () => ({ spawnSync: jest.fn() }))
 jest.mock('fs-extra')
 jest.mock('fs')
 jest.mock('git-state')
@@ -17,6 +20,7 @@ describe(`test if the appli`, () => {
     fsMocked.__setMockFiles({
       output: '',
     })
+    child_process.spawnSync.mockImplementation(() => ({ stdout: '' }))
   })
 
   test('can execute with simple parameters and no diff', () => {
@@ -143,4 +147,24 @@ describe(`test if the appli`, () => {
       })
     }).toThrow()
   })
+
+  test('throw errors when "-t" and "-d" are set', () => {
+    const notHeadSHA = 'test'
+    /*const child_process = require('child_process')
+    jest.mock('child_process', () => ({ spawnSync: jest.fn() }))*/
+    child_process.spawnSync
+      .mockReturnValueOnce({ stdout: Buffer.from('HEAD', gc.UTF8_ENCODING) })
+      .mockReturnValueOnce({
+        stdout: Buffer.from(notHeadSHA, gc.UTF8_ENCODING),
+      })
+    expect(() => {
+      app({
+        output: 'output',
+        repo: '',
+        to: notHeadSHA,
+        generateDelta: true,
+        apiVersion: '46',
+      })
+    }).toThrow()
+  })
 })

__tests__/unit/lib/utils/repoSetup.test.js

@@ -1,25 +1,69 @@
 'use strict'
-const repoSetup = require('../../../../src/utils/repoSetup')
+const RepoSetup = require('../../../../src/utils/repoSetup')
+const gc = require('../../../../src/utils/gitConstants')
 const child_process = require('child_process')
 jest.mock('child_process', () => ({ spawnSync: jest.fn() }))
 jest.mock('fs-extra')
 jest.mock('fs')
 
 describe(`test if repoSetup`, () => {
+  test('say "to" equal "HEAD"', () => {
+    const config = { repo: '.', to: 'HEAD' }
+    child_process.spawnSync.mockImplementation(() => ({
+      stdout: '',
+    }))
+    const repoSetup = new RepoSetup(config)
+    const toEqualHead = repoSetup.isToEqualHead()
+
+    expect(toEqualHead).toBe(true)
+    expect(child_process.spawnSync).not.toHaveBeenCalled()
+  })
+
+  test('say when "to" do not equals "HEAD"', () => {
+    const config = { repo: '.', to: 'not HEAD' }
+    child_process.spawnSync
+      .mockReturnValueOnce({ stdout: Buffer.from('HEAD', gc.UTF8_ENCODING) })
+      .mockReturnValueOnce({
+        stdout: Buffer.from('not HEAD', gc.UTF8_ENCODING),
+      })
+    const repoSetup = new RepoSetup(config)
+    const toEqualHead = repoSetup.isToEqualHead()
+
+    expect(toEqualHead).toBe(false)
+    expect(child_process.spawnSync).toHaveBeenCalled()
+  })
+
   test('can set config.from if not defined', () => {
     const config = { repo: '.' }
     child_process.spawnSync.mockImplementation(() => ({
       stdout: '',
     }))
-    repoSetup(config)
-    expect(config.from).not.toBeUndefined()
+    const repoSetup = new RepoSetup(config)
+    const firsSha = repoSetup.computeFromRef()
+
+    expect(firsSha).not.toBeUndefined()
+    expect(child_process.spawnSync).toHaveBeenCalled()
+  })
+
+  test('can set config.from if defined', () => {
+    const config = { repo: '.', from: 'HEAD~1' }
+    child_process.spawnSync.mockImplementation(() => ({
+      stdout: '',
+    }))
+    const repoSetup = new RepoSetup(config)
+    const firsSha = repoSetup.computeFromRef()
+
+    expect(firsSha).not.toBeUndefined()
+    expect(child_process.spawnSync).not.toHaveBeenCalled()
   })
+
   test('can set core.quotepath to off', () => {
-    const config = { repo: '.', from: 'HEAD' }
+    const config = { repo: '.', from: 'HEAD~1' }
     child_process.spawnSync.mockImplementation(() => ({
       stdout: '',
     }))
-    repoSetup(config)
-    expect(config.from).not.toBeUndefined()
+    const repoSetup = new RepoSetup(config)
+    repoSetup.repoConfiguration()
+    expect(child_process.spawnSync).toHaveBeenCalled()
   })
 })

src/main.js

@@ -3,7 +3,7 @@ const PackageConstructor = require('./utils/packageConstructor')
 const TypeHandlerFactory = require('./service/typeHandlerFactory')
 const { sanitizePath } = require('./utils/childProcessUtils')
 const metadataManager = require('./metadata/metadataManager')
-const repoSetup = require('./utils/repoSetup')
+const RepoSetup = require('./utils/repoSetup')
 const repoGitDiff = require('./utils/repoGitDiff')
 
 const fs = require('fs')
@@ -15,7 +15,7 @@ const DESTRUCTIVE_CHANGES_FILE_NAME = 'destructiveChanges'
 const PACKAGE_FILE_NAME = 'package'
 const XML_FILE_EXTENSION = 'xml'
 
-const checkConfig = config => {
+const checkConfig = (config, repoSetup) => {
   const errors = []
   if (typeof config.to !== 'string') {
     errors.push(`to ${config.to} is not a sha`)
@@ -34,27 +34,36 @@ const checkConfig = config => {
     errors.push(`${config.repo} is not a git repository`)
   }
 
+  if (!repoSetup.isToEqualHead() && config.generateDelta) {
+    errors.push(
+      `--generate-delta (-d) parameter cannot be used when --to (-t) parameter is not equivalent to HEAD`
+    )
+  }
+
   return errors
 }
 
-const sanitizeConfig = config => {
+const sanitizeConfig = (config, repoSetup) => {
   config.apiVersion = parseInt(config.apiVersion)
-  repoSetup(config)
   config.repo = config.repo ? sanitizePath(config.repo) : config.repo
   config.output = sanitizePath(config.output)
   config.ignore = config.ignore ? sanitizePath(config.ignore) : config.ignore
   config.ignoreDestructive = config.ignoreDestructive
     ? sanitizePath(config.ignoreDestructive)
     : config.ignoreDestructive
+  config.from = repoSetup.computeFromRef()
 }
 
 module.exports = config => {
-  sanitizeConfig(config)
-  const inputError = checkConfig(config)
+  const repoSetup = new RepoSetup(config)
+  sanitizeConfig(config, repoSetup)
+  const inputError = checkConfig(config, repoSetup)
   if (inputError.length > 0) {
     throw new Error(inputError)
   }
 
+  repoSetup.repoConfiguration()
+
   const metadata = metadataManager.getDefinition(
     'directoryName',
     config.apiVersion

src/utils/repoSetup.js

@@ -2,19 +2,58 @@
 const childProcess = require('child_process')
 const gc = require('./gitConstants')
 
-const revlistParams = ['rev-list', '--max-parents=0', 'HEAD']
+const HEAD = 'HEAD'
+
+const revparseParams = ['rev-parse']
+const revlistParams = ['rev-list', '--max-parents=0', HEAD]
 const gitConfig = ['config', 'core.quotepath', 'off']
 
-module.exports = config => {
-  childProcess.spawnSync('git', gitConfig, {
-    cwd: config.repo,
-  }).stdout
-  if (!config.from) {
-    const firstCommitSHARaw = childProcess.spawnSync('git', revlistParams, {
-      cwd: config.repo,
-    }).stdout
-    const firstCommitSHA = Buffer.from(firstCommitSHARaw)
-
-    config.from = firstCommitSHA.toString(gc.UTF8_ENCODING).trim()
+const _bufToStr = buf => {
+  return Buffer.from(buf).toString(gc.UTF8_ENCODING).trim()
+}
+
+class RepoSetup {
+  constructor(config) {
+    this.config = config
+    this.config.generateDelta
+  }
+
+  isToEqualHead() {
+    if (this.config.to === HEAD) {
+      return true
+    }
+    const headSHA = _bufToStr(
+      childProcess.spawnSync('git', [...revparseParams, HEAD], {
+        cwd: this.config.repo,
+      }).stdout
+    )
+
+    const toSHA = _bufToStr(
+      childProcess.spawnSync('git', [...revparseParams, this.config.to], {
+        cwd: this.config.repo,
+      }).stdout
+    )
+
+    return toSHA === headSHA
+  }
+
+  repoConfiguration() {
+    childProcess.spawnSync('git', gitConfig, {
+      cwd: this.config.repo,
+    })
+  }
+
+  computeFromRef() {
+    let firstCommitSHA = this.config.from
+    if (!firstCommitSHA) {
+      firstCommitSHA = _bufToStr(
+        childProcess.spawnSync('git', revlistParams, {
+          cwd: this.config.repo,
+        }).stdout
+      )
+    }
+    return firstCommitSHA
   }
 }
+
+module.exports = RepoSetup