From e95c89fc64b26e003ab417ded5b5b0c5dde3d028 Mon Sep 17 00:00:00 2001
From: Lucas Holmquist <lholmqui@redhat.com>
Date: Fri, 19 Jul 2019 11:25:10 -0400
Subject: [PATCH 1/7] squash: adding some comments

squash: add more comments

squash: add more comments
---
 bin/cmd.js | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/bin/cmd.js b/bin/cmd.js
index 9d81059..47f95a0 100755
--- a/bin/cmd.js
+++ b/bin/cmd.js
@@ -35,19 +35,28 @@ const shortHand = { h: ['--help']
 const parsed = nopt(knownOpts, shortHand)
 const usage = require('help')()
 
+// The --help or -h flag was used
 if (parsed.help) {
   return usage()
 }
 
+// The --version or -v flag was used
 if (parsed.version) {
   console.log('core-validate-commit', 'v' + require('../package').version)
   return
 }
 
+// any arguments after a --flag will be in the remain array
 const args = parsed.argv.remain
+
+// The argument should be the commit you are validating
+// If there is no args,  then use the HEAD commit
 if (!args.length)
   args.push('HEAD')
 
+// Given a commit hash, load it
+// If a URL is passed in, then load the commit remotely
+// If not, then do a git show
 function load(sha, cb) {
   const parsed = url.parse(sha)
   if (parsed.protocol) {
@@ -60,6 +69,7 @@ function load(sha, cb) {
   })
 }
 
+// Load the commit from a URL
 function loadPatch(uri, cb) {
   let h = http
   if (~uri.protocol.indexOf('https')) {
@@ -85,26 +95,36 @@ function loadPatch(uri, cb) {
   }).on('error', cb)
 }
 
+// Create a new Validator
 const v = new Validator(parsed)
 
+// The --list-subsytems or --ls flag was used
 if (parsed['list-subsystems']) {
   utils.describeSubsystem(subsystem.defaults.subsystems.sort())
   return
 }
 
+// The --list or -l flag was used
 if (parsed.list) {
+  // Get the list of Rule names
+  // There is nothing here that says we need to have created that validator first,
+  // unless at some point we don't count disabled things
+  // But we should probably just get the rules from the rules in ./lib/rules
+  // Then this function can move up to the top
   const ruleNames = Array.from(v.rules.keys())
+  // Find the length of the longest Rule names
   const max = ruleNames.reduce((m, item) => {
     if (item.length > m) m = item.length
     return m
   }, 0)
-
+  // Loop through and output the rules
   for (const rule of v.rules.values()) {
     utils.describeRule(rule, max)
   }
   return
 }
 
+// The --tap or -t flag was used
 if (parsed.tap) {
   const tap = new Tap()
   tap.pipe(process.stdout)
@@ -138,6 +158,7 @@ if (parsed.tap) {
   run()
 
 } else {
+  // no --flags used,  defaults to --validate-metadata
   v.on('commit', (c) => {
     pretty(c.commit, c.messages, v)
     run()

From d5dc539840a23f5354cf4c19cd3a33272bef54d9 Mon Sep 17 00:00:00 2001
From: Lucas Holmquist <lholmqui@redhat.com>
Date: Tue, 23 Jul 2019 14:55:22 -0400
Subject: [PATCH 2/7] squash: move the list-subsytem command up

* This moves the --list-subsystems command up before the creation of the Validator, since it doesn't need that to happen
---
 bin/cmd.js | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/bin/cmd.js b/bin/cmd.js
index 47f95a0..ad66d1e 100755
--- a/bin/cmd.js
+++ b/bin/cmd.js
@@ -46,6 +46,12 @@ if (parsed.version) {
   return
 }
 
+// The --list-subsytems or --ls flag was used
+if (parsed['list-subsystems']) {
+  utils.describeSubsystem(subsystem.defaults.subsystems.sort())
+  return
+}
+
 // any arguments after a --flag will be in the remain array
 const args = parsed.argv.remain
 
@@ -98,12 +104,6 @@ function loadPatch(uri, cb) {
 // Create a new Validator
 const v = new Validator(parsed)
 
-// The --list-subsytems or --ls flag was used
-if (parsed['list-subsystems']) {
-  utils.describeSubsystem(subsystem.defaults.subsystems.sort())
-  return
-}
-
 // The --list or -l flag was used
 if (parsed.list) {
   // Get the list of Rule names

From 2254e6590c111569e8cf7c2c79374a22e1e47efd Mon Sep 17 00:00:00 2001
From: Lucas Holmquist <lholmqui@redhat.com>
Date: Tue, 23 Jul 2019 14:53:40 -0400
Subject: [PATCH 3/7] squash: moving the load and loadPatch functions util

* This moves the function that loads the commit, from either a 'git show' or from a url, into the util module
* This is for cleaning up the main cmd.js file which should only really have logic related to the commands
---
 bin/cmd.js   | 53 ++--------------------------------------------------
 lib/utils.js | 45 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 51 deletions(-)

diff --git a/bin/cmd.js b/bin/cmd.js
index ad66d1e..98505f6 100755
--- a/bin/cmd.js
+++ b/bin/cmd.js
@@ -2,11 +2,7 @@
 
 'use strict'
 
-const exec = require('child_process').exec
 const fs = require('fs')
-const http = require('http')
-const https = require('https')
-const url = require('url')
 const nopt = require('nopt')
 const path = require('path')
 const pretty = require('../lib/format-pretty')
@@ -60,57 +56,12 @@ const args = parsed.argv.remain
 if (!args.length)
   args.push('HEAD')
 
-// Given a commit hash, load it
-// If a URL is passed in, then load the commit remotely
-// If not, then do a git show
-function load(sha, cb) {
-  const parsed = url.parse(sha)
-  if (parsed.protocol) {
-    return loadPatch(parsed, cb)
-  }
-
-  exec(`git show --quiet --format=medium ${sha}`, (err, stdout, stderr) => {
-    if (err) return cb(err)
-    cb(null, stdout.trim())
-  })
-}
-
-// Load the commit from a URL
-function loadPatch(uri, cb) {
-  let h = http
-  if (~uri.protocol.indexOf('https')) {
-    h = https
-  }
-  uri.headers = {
-    'user-agent': 'core-validate-commit'
-  }
-  h.get(uri, (res) => {
-    let buf = ''
-    res.on('data', (chunk) => {
-      buf += chunk
-    })
-
-    res.on('end', () => {
-      try {
-        const out = JSON.parse(buf)
-        cb(null, out)
-      } catch (err) {
-        cb(err)
-      }
-    })
-  }).on('error', cb)
-}
-
 // Create a new Validator
 const v = new Validator(parsed)
 
 // The --list or -l flag was used
 if (parsed.list) {
   // Get the list of Rule names
-  // There is nothing here that says we need to have created that validator first,
-  // unless at some point we don't count disabled things
-  // But we should probably just get the rules from the rules in ./lib/rules
-  // Then this function can move up to the top
   const ruleNames = Array.from(v.rules.keys())
   // Find the length of the longest Rule names
   const max = ruleNames.reduce((m, item) => {
@@ -148,7 +99,7 @@ if (parsed.tap) {
   function run() {
     if (!args.length) return
     const sha = args.shift()
-    load(sha, (err, data) => {
+    utils.load(sha, (err, data) => {
       if (err) throw err
       v.lint(data)
       run()
@@ -170,7 +121,7 @@ if (parsed.tap) {
       return
     }
     const sha = args.shift()
-    load(sha, (err, data) => {
+    utils.load(sha, (err, data) => {
       if (err) throw err
       v.lint(data)
     })
diff --git a/lib/utils.js b/lib/utils.js
index 49dd75d..3d7d2ed 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -1,5 +1,9 @@
 'use strict'
 
+const exec = require('child_process').exec
+const http = require('http')
+const https = require('https')
+const url = require('url')
 const chalk = require('chalk')
 const CHECK = chalk.green('✔')
 const X = chalk.red('✖')
@@ -57,3 +61,44 @@ exports.describeSubsystem = function describeSubsystem(subsystems, max = 20) {
     }
   }
 }
+
+// Given a commit hash, load it
+// If a URL is passed in, then load the commit remotely
+// If not, then do a git show
+exports.load = function load(sha, cb) {
+  const parsed = url.parse(sha)
+  if (parsed.protocol) {
+    return exports.loadPatch(parsed, cb)
+  }
+
+  exec(`git show --quiet --format=medium ${sha}`, (err, stdout, stderr) => {
+    if (err) return cb(err)
+    cb(null, stdout.trim())
+  })
+}
+
+// Load the commit from a URL
+exports.loadPatch = function loadPatch(uri, cb) {
+  let h = http
+  if (~uri.protocol.indexOf('https')) {
+    h = https
+  }
+  uri.headers = {
+    'user-agent': 'core-validate-commit'
+  }
+  h.get(uri, (res) => {
+    let buf = ''
+    res.on('data', (chunk) => {
+      buf += chunk
+    })
+
+    res.on('end', () => {
+      try {
+        const out = JSON.parse(buf)
+        cb(null, out)
+      } catch (err) {
+        cb(err)
+      }
+    })
+  }).on('error', cb)
+}

From b4949d0af3e0e481b2a84dddaeb04612f8711075 Mon Sep 17 00:00:00 2001
From: Lucas Holmquist <lholmqui@redhat.com>
Date: Thu, 25 Jul 2019 11:40:23 -0400
Subject: [PATCH 4/7] squash: Adding tests in for load and loadPatch

* This also adds the proxyquire and nock modules as dev dependencies
---
 package.json       |   2 +
 test/utils-test.js | 104 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)
 create mode 100644 test/utils-test.js

diff --git a/package.json b/package.json
index f888be8..a4a1565 100644
--- a/package.json
+++ b/package.json
@@ -18,6 +18,8 @@
   "devDependencies": {
     "check-pkg": "^2.0.2",
     "lintit": "^6.0.1",
+    "nock": "~10.0.6",
+    "proxyquire": "^2.1.1",
     "tap": "^12.6.1"
   },
   "files": [
diff --git a/test/utils-test.js b/test/utils-test.js
new file mode 100644
index 0000000..fc112e9
--- /dev/null
+++ b/test/utils-test.js
@@ -0,0 +1,104 @@
+'use strict'
+
+const { test } = require('tap')
+const proxyquire = require('proxyquire')
+const nock = require('nock')
+
+const commitHash = 'a12b34c56'
+
+test('Test util functions', (t) => {
+  t.test('test sha load function', (tt) => {
+    const commitMessage = 'Commit Message'
+
+    const utils = proxyquire('../lib/utils', {
+      'child_process': {
+        exec: (cmd, cb) => {
+          tt.equals(cmd,
+                    `git show --quiet --format=medium ${commitHash}`,
+                    'cmd should be equal')
+          cb(null, commitMessage)
+        }
+      }
+    })
+
+    utils.load(commitHash, (err, stdout, stderr) => {
+      tt.equals(err, null, 'Should not be an error')
+      tt.equals(commitMessage, stdout, 'should have the commit message')
+      tt.end()
+    })
+  })
+
+  t.test('test sha load function with error', (tt) => {
+    const utils = proxyquire('../lib/utils', {
+      'child_process': {
+        exec: (cmd, cb) => {
+          cb('Error', null)
+        }
+      }
+    })
+
+    utils.load(commitHash, (err, stdout, stderr) => {
+      tt.equals(err, 'Error', 'should have the error message')
+      tt.end()
+    })
+  })
+
+  t.test('test load patch function using http', (tt) => {
+    const commitUrl = `http://api.github.com/repos/nodejs/${commitHash}`
+    const util = require('../lib/utils')
+
+    nock('http://api.github.com', {
+      reqheaders: {
+        'user-agent': 'core-validate-commit'
+      }
+    })
+      .get(`/repos/nodejs/${commitHash}`)
+      .reply(200, {commit: 'message'})
+
+    util.load(commitUrl, (err, commitMessage) => {
+      tt.equals(err, null, 'Should not be an error')
+      tt.pass()
+      tt.end()
+    })
+  })
+
+  t.test('test load patch function using https', (tt) => {
+    const commitUrl = `https://api.github.com/repos/nodejs/${commitHash}`
+    const util = require('../lib/utils')
+
+    nock('https://api.github.com', {
+      reqheaders: {
+        'user-agent': 'core-validate-commit'
+      }
+    })
+      .get(`/repos/nodejs/${commitHash}`)
+      .reply(200, {commit: 'message'})
+
+    util.load(commitUrl, (err, commitMessage) => {
+      tt.equals(err, null, 'Should not be an error')
+      tt.pass()
+      tt.end()
+    })
+  })
+
+  t.test('test load patch function - catch parse error', (tt) => {
+    const commitUrl = `http://api.github.com/repos/nodejs/${commitHash}`
+    const util = require('../lib/utils')
+
+    nock('http://api.github.com', {
+      reqheaders: {
+        'user-agent': 'core-validate-commit'
+      }
+    })
+      .get(`/repos/nodejs/${commitHash}`)
+      .reply(200, '{commit: \'message\'}')
+
+    util.load(commitUrl, (err, commitMessage) => {
+      tt.true(err)
+      tt.pass()
+      tt.end()
+    })
+  })
+
+  t.end()
+})

From 5d8f5caff22e555df6887b68edb77f88e9cb750c Mon Sep 17 00:00:00 2001
From: Lucas Holmquist <lholmqui@redhat.com>
Date: Tue, 23 Jul 2019 15:26:53 -0400
Subject: [PATCH 5/7] squash: moving validate-metadata

* move validate-metadata logic to utils for easier testing
---
 bin/cmd.js   | 21 ++-------------------
 lib/utils.js | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/bin/cmd.js b/bin/cmd.js
index 98505f6..866a3e8 100755
--- a/bin/cmd.js
+++ b/bin/cmd.js
@@ -5,7 +5,6 @@
 const fs = require('fs')
 const nopt = require('nopt')
 const path = require('path')
-const pretty = require('../lib/format-pretty')
 const formatTap = require('../lib/format-tap')
 const Validator = require('../lib')
 const Tap = require('../lib/tap')
@@ -110,22 +109,6 @@ if (parsed.tap) {
 
 } else {
   // no --flags used,  defaults to --validate-metadata
-  v.on('commit', (c) => {
-    pretty(c.commit, c.messages, v)
-    run()
-  })
-
-  function run() {
-    if (!args.length) {
-      process.exitCode = v.errors
-      return
-    }
-    const sha = args.shift()
-    utils.load(sha, (err, data) => {
-      if (err) throw err
-      v.lint(data)
-    })
-  }
-
-  run()
+  utils.validateMetadata(v, args)
+  return
 }
diff --git a/lib/utils.js b/lib/utils.js
index 3d7d2ed..b5a7bd8 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -4,6 +4,7 @@ const exec = require('child_process').exec
 const http = require('http')
 const https = require('https')
 const url = require('url')
+const pretty = require('../lib/format-pretty')
 const chalk = require('chalk')
 const CHECK = chalk.green('✔')
 const X = chalk.red('✖')
@@ -102,3 +103,24 @@ exports.loadPatch = function loadPatch(uri, cb) {
     })
   }).on('error', cb)
 }
+
+exports.validateMetadata = function(validator, args) {
+  validator.on('commit', (c) => {
+    pretty(c.commit, c.messages, validator)
+    run()
+  })
+
+  function run() {
+    if (!args.length) {
+      process.exitCode = validator.errors
+      return
+    }
+    const sha = args.shift()
+    exports.load(sha, (err, data) => {
+      if (err) throw err
+      validator.lint(data)
+    })
+  }
+
+  run()
+}

From b31953f312e26aec6c952d752a1afd343d768bbd Mon Sep 17 00:00:00 2001
From: Lucas Holmquist <lholmqui@redhat.com>
Date: Thu, 25 Jul 2019 12:24:29 -0400
Subject: [PATCH 6/7] squash: move the tap logic to util

* This moves the tap logic out of cmd.js and into a separate function in utils
---
 bin/cmd.js   | 36 ++----------------------------------
 lib/utils.js | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/bin/cmd.js b/bin/cmd.js
index 866a3e8..45da9ce 100755
--- a/bin/cmd.js
+++ b/bin/cmd.js
@@ -2,12 +2,9 @@
 
 'use strict'
 
-const fs = require('fs')
 const nopt = require('nopt')
 const path = require('path')
-const formatTap = require('../lib/format-tap')
 const Validator = require('../lib')
-const Tap = require('../lib/tap')
 const utils = require('../lib/utils')
 const subsystem = require('../lib/rules/subsystem')
 const knownOpts = { help: Boolean
@@ -76,37 +73,8 @@ if (parsed.list) {
 
 // The --tap or -t flag was used
 if (parsed.tap) {
-  const tap = new Tap()
-  tap.pipe(process.stdout)
-  if (parsed.out) tap.pipe(fs.createWriteStream(parsed.out))
-  let count = 0
-  let total = args.length
-
-  v.on('commit', (c) => {
-    count++
-    const test = tap.test(c.commit.sha)
-    formatTap(test, c.commit, c.messages, v)
-    if (count === total) {
-      setImmediate(() => {
-        tap.end()
-        if (tap.status === 'fail')
-          process.exitCode = 1
-      })
-    }
-  })
-
-  function run() {
-    if (!args.length) return
-    const sha = args.shift()
-    utils.load(sha, (err, data) => {
-      if (err) throw err
-      v.lint(data)
-      run()
-    })
-  }
-
-  run()
-
+  utils.parseTap(v, parsed, args)
+  return
 } else {
   // no --flags used,  defaults to --validate-metadata
   utils.validateMetadata(v, args)
diff --git a/lib/utils.js b/lib/utils.js
index b5a7bd8..12124f7 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -1,9 +1,12 @@
 'use strict'
 
+const fs = require('fs')
 const exec = require('child_process').exec
 const http = require('http')
 const https = require('https')
 const url = require('url')
+const formatTap = require('../lib/format-tap')
+const Tap = require('../lib/tap')
 const pretty = require('../lib/format-pretty')
 const chalk = require('chalk')
 const CHECK = chalk.green('✔')
@@ -124,3 +127,36 @@ exports.validateMetadata = function(validator, args) {
 
   run()
 }
+
+exports.parseTap = function(validator, parsed, args) {
+  const tap = new Tap()
+  tap.pipe(process.stdout)
+  if (parsed.out) tap.pipe(fs.createWriteStream(parsed.out))
+  let count = 0
+  let total = args.length
+
+  validator.on('commit', (c) => {
+    count++
+    const test = tap.test(c.commit.sha)
+    formatTap(test, c.commit, c.messages, validator)
+    if (count === total) {
+      setImmediate(() => {
+        tap.end()
+        if (tap.status === 'fail')
+          process.exitCode = 1
+      })
+    }
+  })
+
+  function run() {
+    if (!args.length) return
+    const sha = args.shift()
+    exports.load(sha, (err, data) => {
+      if (err) throw err
+      validator.lint(data)
+      run()
+    })
+  }
+
+  run()
+}

From 28f321ed73a9db89bc073d1250450aee166e7417 Mon Sep 17 00:00:00 2001
From: Lucas Holmquist <lholmqui@redhat.com>
Date: Mon, 29 Jul 2019 10:17:07 -0400
Subject: [PATCH 7/7] squash: add callback function

* This adds a callback function to both the validate-metadata and tap functions.
* it also moves the process.exitCode assignment back into the cmd.js instead of the in the actual util module function
* This will make testing those indiviual functions eaiser.
* Another added benefit is if at some point a programmable API was to be created, we just want to return a code instead of assigning it to the process.exitCode
---
 bin/cmd.js   | 12 ++++++++----
 lib/utils.js | 14 ++++++++------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/bin/cmd.js b/bin/cmd.js
index 45da9ce..cce8cb1 100755
--- a/bin/cmd.js
+++ b/bin/cmd.js
@@ -73,10 +73,14 @@ if (parsed.list) {
 
 // The --tap or -t flag was used
 if (parsed.tap) {
-  utils.parseTap(v, parsed, args)
-  return
+  utils.parseTap(v, parsed, args, (code) => {
+    process.exitCode = code
+    return
+  })
 } else {
   // no --flags used,  defaults to --validate-metadata
-  utils.validateMetadata(v, args)
-  return
+  utils.validateMetadata(v, args, (code) => {
+    process.exitCode = code
+    return
+  })
 }
diff --git a/lib/utils.js b/lib/utils.js
index 12124f7..a2e2c44 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -107,7 +107,7 @@ exports.loadPatch = function loadPatch(uri, cb) {
   }).on('error', cb)
 }
 
-exports.validateMetadata = function(validator, args) {
+exports.validateMetadata = function(validator, args, cb) {
   validator.on('commit', (c) => {
     pretty(c.commit, c.messages, validator)
     run()
@@ -115,8 +115,7 @@ exports.validateMetadata = function(validator, args) {
 
   function run() {
     if (!args.length) {
-      process.exitCode = validator.errors
-      return
+      return cb(validator.errors)
     }
     const sha = args.shift()
     exports.load(sha, (err, data) => {
@@ -128,7 +127,7 @@ exports.validateMetadata = function(validator, args) {
   run()
 }
 
-exports.parseTap = function(validator, parsed, args) {
+exports.parseTap = function(validator, parsed, args, cb) {
   const tap = new Tap()
   tap.pipe(process.stdout)
   if (parsed.out) tap.pipe(fs.createWriteStream(parsed.out))
@@ -142,8 +141,11 @@ exports.parseTap = function(validator, parsed, args) {
     if (count === total) {
       setImmediate(() => {
         tap.end()
-        if (tap.status === 'fail')
-          process.exitCode = 1
+        if (tap.status === 'fail') {
+          return cb(1)
+        }
+
+        return cb(0)
       })
     }
   })