[ci-visibility] Better git commands

packages/dd-trace/src/plugins/util/exec.js

@@ -1,8 +1,8 @@
 const cp = require('child_process')
 
-const sanitizedExec = (cmd, options = {}) => {
+const sanitizedExec = (cmd, flags, options = { stdio: 'pipe' }) => {
   try {
-    return cp.execSync(cmd, options).toString().replace(/(\r\n|\n|\r)/gm, '')
+    return cp.execFileSync(cmd, flags, options).toString().replace(/(\r\n|\n|\r)/gm, '')
   } catch (e) {
     return ''
   }

packages/dd-trace/src/plugins/util/git.js

@@ -1,6 +1,7 @@
-const { execSync } = require('child_process')
+const { execFileSync } = require('child_process')
 const os = require('os')
 const path = require('path')
+const fs = require('fs')
 
 const log = require('../../log')
 const { sanitizedExec } = require('./exec')
@@ -21,26 +22,35 @@ const {
 
 const GIT_REV_LIST_MAX_BUFFER = 8 * 1024 * 1024 // 8MB
 
+function isDirectory (path) {
+  try {
+    const stats = fs.statSync(path)
+    return stats.isDirectory()
+  } catch (e) {
+    return false
+  }
+}
+
 function isShallowRepository () {
-  return sanitizedExec('git rev-parse --is-shallow-repository', { stdio: 'pipe' }) === 'true'
+  return sanitizedExec('git', ['rev-parse', '--is-shallow-repository']) === 'true'
 }
 
 function unshallowRepository () {
   try {
-    execSync('git config remote.origin.partialclonefilter "blob:none"', { stdio: 'pipe' })
-    execSync('git fetch --shallow-since="1 month ago" --update-shallow --refetch', { stdio: 'pipe' })
+    sanitizedExec('git', ['config', 'remote.origin.partialclonefilter', '"blob:none"'])
+    sanitizedExec('git', ['fetch', '--shallow-since="1 month ago"', '--update-shallow', '--refetch'])
   } catch (err) {
     log.error(err)
   }
 }
 
 function getRepositoryUrl () {
-  return sanitizedExec('git config --get remote.origin.url', { stdio: 'pipe' })
+  return sanitizedExec('git', ['config', '--get', 'remote.origin.url'])
 }
 
 function getLatestCommits () {
   try {
-    return execSync('git log --format=%H -n 1000 --since="1 month ago"', { stdio: 'pipe' })
+    return execFileSync('git', ['log', '--format=%H', '-n 1000', '--since="1 month ago"'], { stdio: 'pipe' })
       .toString()
       .split('\n')
       .filter(commit => commit)
@@ -51,15 +61,21 @@ function getLatestCommits () {
 }
 
 function getCommitsToUpload (commitsToExclude) {
-  let gitCommandToGetCommitsToUpload =
-    'git rev-list --objects --no-object-names --filter=blob:none --since="1 month ago" HEAD'
-
-  commitsToExclude.forEach(commit => {
-    gitCommandToGetCommitsToUpload = `${gitCommandToGetCommitsToUpload} ^${commit}`
-  })
+  const commitsToExcludeString = commitsToExclude.map(commit => `^${commit}`)
 
   try {
-    return execSync(gitCommandToGetCommitsToUpload, { stdio: 'pipe', maxBuffer: GIT_REV_LIST_MAX_BUFFER })
+    return execFileSync(
+      'git',
+      [
+        'rev-list',
+        '--objects',
+        '--no-object-names',
+        '--filter=blob:none',
+        '--since="1 month ago"',
+        'HEAD',
+        ...commitsToExcludeString
+      ],
+      { stdio: 'pipe', maxBuffer: GIT_REV_LIST_MAX_BUFFER })
       .toString()
       .split('\n')
       .filter(commit => commit)
@@ -72,21 +88,32 @@ function getCommitsToUpload (commitsToExclude) {
 function generatePackFilesForCommits (commitsToUpload) {
   const tmpFolder = os.tmpdir()
 
+  if (!isDirectory(tmpFolder)) {
+    log.error(new Error('Provided path to generate packfiles is not a directory'))
+    return []
+  }
+
   const randomPrefix = String(Math.floor(Math.random() * 10000))
   const temporaryPath = path.join(tmpFolder, randomPrefix)
   const cwdPath = path.join(process.cwd(), randomPrefix)
 
   // Generates pack files to upload and
   // returns the ordered list of packfiles' paths
   function execGitPackObjects (targetPath) {
-    return execSync(
-      `git pack-objects --compression=9 --max-pack-size=3m ${targetPath}`,
-      { input: commitsToUpload.join('\n') }
+    return execFileSync(
+      'git',
+      [
+        'pack-objects',
+        '--compression=9',
+        '--max-pack-size=3m',
+        targetPath
+      ],
+      { stdio: 'pipe', input: commitsToUpload.join('\n') }
     ).toString().split('\n').filter(commit => commit).map(commit => `${targetPath}-${commit}.pack`)
   }
 
   try {
-    return execGitPackObjects(temporaryPath, commitsToUpload)
+    return execGitPackObjects(temporaryPath)
   } catch (err) {
     log.error(err)
     /**
@@ -102,7 +129,7 @@ function generatePackFilesForCommits (commitsToUpload) {
      * TODO: fix issue and remove workaround.
      */
     try {
-      return execGitPackObjects(cwdPath, commitsToUpload)
+      return execGitPackObjects(cwdPath)
     } catch (err) {
       log.error(err)
     }
@@ -133,23 +160,23 @@ function getGitMetadata (ciMetadata) {
     committerName,
     committerEmail,
     committerDate
-  ] = sanitizedExec('git show -s --format=%an,%ae,%aI,%cn,%ce,%cI', { stdio: 'pipe' }).split(',')
+  ] = sanitizedExec('git', ['show', '-s', '--format=%an,%ae,%aI,%cn,%ce,%cI']).split(',')
 
   return {
     [GIT_REPOSITORY_URL]:
-      repositoryUrl || sanitizedExec('git ls-remote --get-url', { stdio: 'pipe' }),
+      repositoryUrl || sanitizedExec('git', ['ls-remote', '--get-url']),
     [GIT_COMMIT_MESSAGE]:
-      commitMessage || sanitizedExec('git show -s --format=%s', { stdio: 'pipe' }),
+      commitMessage || sanitizedExec('git', ['show', '-s', '--format=%s']),
     [GIT_COMMIT_AUTHOR_DATE]: authorDate,
     [GIT_COMMIT_AUTHOR_NAME]: ciAuthorName || authorName,
     [GIT_COMMIT_AUTHOR_EMAIL]: ciAuthorEmail || authorEmail,
     [GIT_COMMIT_COMMITTER_DATE]: committerDate,
     [GIT_COMMIT_COMMITTER_NAME]: committerName,
     [GIT_COMMIT_COMMITTER_EMAIL]: committerEmail,
-    [GIT_BRANCH]: branch || sanitizedExec('git rev-parse --abbrev-ref HEAD', { stdio: 'pipe' }),
-    [GIT_COMMIT_SHA]: commitSHA || sanitizedExec('git rev-parse HEAD', { stdio: 'pipe' }),
+    [GIT_BRANCH]: branch || sanitizedExec('git', ['rev-parse', '--abbrev-ref', 'HEAD']),
+    [GIT_COMMIT_SHA]: commitSHA || sanitizedExec('git', ['rev-parse', 'HEAD']),
     [GIT_TAG]: tag,
-    [CI_WORKSPACE_PATH]: ciWorkspacePath || sanitizedExec('git rev-parse --show-toplevel', { stdio: 'pipe' })
+    [CI_WORKSPACE_PATH]: ciWorkspacePath || sanitizedExec('git', ['rev-parse', '--show-toplevel'])
   }
 }
 

packages/dd-trace/test/plugins/util/git.spec.js

@@ -68,12 +68,12 @@ describe('git', () => {
       }
     )
     expect(metadata[GIT_REPOSITORY_URL]).not.to.equal('ciRepositoryUrl')
-    expect(sanitizedExecStub).to.have.been.calledWith('git ls-remote --get-url', { stdio: 'pipe' })
-    expect(sanitizedExecStub).to.have.been.calledWith('git show -s --format=%an,%ae,%aI,%cn,%ce,%cI', { stdio: 'pipe' })
-    expect(sanitizedExecStub).not.to.have.been.calledWith('git show -s --format=%s', { stdio: 'pipe' })
-    expect(sanitizedExecStub).not.to.have.been.calledWith('git rev-parse HEAD', { stdio: 'pipe' })
-    expect(sanitizedExecStub).not.to.have.been.calledWith('git rev-parse --abbrev-ref HEAD', { stdio: 'pipe' })
-    expect(sanitizedExecStub).not.to.have.been.calledWith('git rev-parse --show-toplevel', { stdio: 'pipe' })
+    expect(sanitizedExecStub).to.have.been.calledWith('git', ['ls-remote', '--get-url'])
+    expect(sanitizedExecStub).to.have.been.calledWith('git', ['show', '-s', '--format=%an,%ae,%aI,%cn,%ce,%cI'])
+    expect(sanitizedExecStub).not.to.have.been.calledWith('git', ['show', '-s', '--format=%s'])
+    expect(sanitizedExecStub).not.to.have.been.calledWith('git', ['rev-parse', 'HEAD'])
+    expect(sanitizedExecStub).not.to.have.been.calledWith('git', ['rev-parse', '--abbrev-ref', 'HEAD'])
+    expect(sanitizedExecStub).not.to.have.been.calledWith('git', ['rev-parse', '--show-toplevel'])
   })
   it('does not crash if git is not available', () => {
     sanitizedExecStub.returns('')
@@ -121,12 +121,12 @@ describe('git', () => {
       [GIT_COMMIT_COMMITTER_NAME]: 'git committer',
       [CI_WORKSPACE_PATH]: 'ciWorkspacePath'
     })
-    expect(sanitizedExecStub).to.have.been.calledWith('git ls-remote --get-url', { stdio: 'pipe' })
-    expect(sanitizedExecStub).to.have.been.calledWith('git show -s --format=%s', { stdio: 'pipe' })
-    expect(sanitizedExecStub).to.have.been.calledWith('git show -s --format=%an,%ae,%aI,%cn,%ce,%cI', { stdio: 'pipe' })
-    expect(sanitizedExecStub).to.have.been.calledWith('git rev-parse HEAD', { stdio: 'pipe' })
-    expect(sanitizedExecStub).to.have.been.calledWith('git rev-parse --abbrev-ref HEAD', { stdio: 'pipe' })
-    expect(sanitizedExecStub).to.have.been.calledWith('git rev-parse --show-toplevel', { stdio: 'pipe' })
+    expect(sanitizedExecStub).to.have.been.calledWith('git', ['ls-remote', '--get-url'])
+    expect(sanitizedExecStub).to.have.been.calledWith('git', ['show', '-s', '--format=%s'])
+    expect(sanitizedExecStub).to.have.been.calledWith('git', ['show', '-s', '--format=%an,%ae,%aI,%cn,%ce,%cI'])
+    expect(sanitizedExecStub).to.have.been.calledWith('git', ['rev-parse', 'HEAD'])
+    expect(sanitizedExecStub).to.have.been.calledWith('git', ['rev-parse', '--abbrev-ref', 'HEAD'])
+    expect(sanitizedExecStub).to.have.been.calledWith('git', ['rev-parse', '--show-toplevel'])
   })
 })
 
@@ -155,7 +155,7 @@ describe('getCommitsToUpload', () => {
     const { getCommitsToUpload } = proxyquire('../../../src/plugins/util/git',
       {
         'child_process': {
-          'execSync': (_, ...rest) => execSync(`head -c ${GIT_REV_LIST_MAX_BUFFER * 2} /dev/zero`, ...rest)
+          'execFileSync': (_, ...rest) => execSync(`head -c ${GIT_REV_LIST_MAX_BUFFER * 2} /dev/zero`, ...rest)
         },
         '../../log': {
           error: logErrorSpy
@@ -169,44 +169,61 @@ describe('getCommitsToUpload', () => {
 })
 
 describe('generatePackFilesForCommits', () => {
+  let tmpdirStub
   beforeEach(() => {
     sinon.stub(Math, 'random').returns('0.1234')
-    sinon.stub(os, 'tmpdir').returns('tmp')
+    tmpdirStub = sinon.stub(os, 'tmpdir').returns('/tmp')
     sinon.stub(process, 'cwd').returns('cwd')
   })
   afterEach(() => {
     sinon.restore()
   })
   it('creates pack files in temporary path', () => {
-    const execSyncSpy = sinon.stub().returns(['commitSHA'])
+    const execFileSyncSpy = sinon.stub().returns(['commitSHA'])
 
     const { generatePackFilesForCommits } = proxyquire('../../../src/plugins/util/git',
       {
         'child_process': {
-          'execSync': execSyncSpy
+          'execFileSync': execFileSyncSpy
         }
       }
     )
 
-    const temporaryPath = path.join('tmp', '1234')
+    const temporaryPath = path.join('/tmp', '1234')
     const packFilesToUpload = generatePackFilesForCommits(['commitSHA'])
     expect(packFilesToUpload).to.eql([`${temporaryPath}-commitSHA.pack`])
   })
 
   it('creates pack files in cwd if the temporary path fails', () => {
-    const execSyncSpy = sinon.stub().onCall(0).throws().onCall(1).returns(['commitSHA'])
+    const execFileSyncSpy = sinon.stub().onCall(0).throws().onCall(1).returns(['commitSHA'])
 
     const cwdPath = path.join('cwd', '1234')
 
     const { generatePackFilesForCommits } = proxyquire('../../../src/plugins/util/git',
       {
         'child_process': {
-          'execSync': execSyncSpy
+          'execFileSync': execFileSyncSpy
         }
       }
     )
 
     const packFilesToUpload = generatePackFilesForCommits(['commitSHA'])
     expect(packFilesToUpload).to.eql([`${cwdPath}-commitSHA.pack`])
   })
+
+  it('does not work if tmpdir does not return a folder', () => {
+    tmpdirStub.restore()
+    sinon.stub(os, 'tmpdir').returns('; echo hey')
+    const execFileSyncSpy = sinon.stub().onCall(0).throws().onCall(1).returns(['commitSHA'])
+
+    const { generatePackFilesForCommits } = proxyquire('../../../src/plugins/util/git',
+      {
+        'child_process': {
+          'execFileSync': execFileSyncSpy
+        }
+      }
+    )
+    const packFilesToUpload = generatePackFilesForCommits(['commitSHA'])
+    expect(packFilesToUpload).to.eql([])
+  })
 })