some tweaks to multiple issue support (#12)

- rename the long opts to '--{add,remove}-issues'
- use dashdash custom type to parse user-supplied value sooner
- clean up (debatable I suppose :) the option/arg handling to determine
  the action being performed
- drop unused optional "ISSUE" arg to `grr -D [ISSUE]`
- allow extra issues initially via `grr MAIN-ISSUE [EXTRA-ISSUES ...]`
- show titles of extra issues in regular grr output
- cache issue data for extra issues in .git/config (and add a TODO for
  how these can be used to not have to re-fetch extra issue data for
  every run)

TODO.txt

@@ -16,6 +16,14 @@
 - --commit
 - `grr -D` to check that all commits have been pushed to Gerrit, else require
   a -f
+- `extraIssues` improvement to not have to fetch issue info from JIRA/GH every
+  time. Currently the `extraIssues` issueTitle is being cached locally in
+  ".git/config", so the `grrUpdateOrCreate` pipeline doesn't always *need*
+  to go fetch issue data for them. Instead:
+    1. `stepParseExtraIssues` could be changed to just `ensureExtraIssueData`
+       (similar to `ensureIssueId` just above it for the main issue).
+    2. We would then need to ensure that `grr --update` would re-fetch issue
+       data for `extraIssues`.
 - support an option (in config and per branch) to push to a remote GH branch
   as well for every `grr`. Then have some of the GH branch utilities
   (nicer full diff, seeing individual commits as they were made, ability to

lib/cli.js

@@ -22,6 +22,31 @@ var grr = require('./grr');
 var pkg = require('../package.json');
 
 
+// ---- custom option type (from dashdash/examples/)
+
+function parseCommaSepStringNoEmpties(option, optstr, arg) {
+    // JSSTYLED
+    return arg.trim().split(/\s*,\s*/g)
+        .filter(function (part) { return part; });
+}
+
+dashdash.addOptionType({
+    name: 'commaSepString',
+    takesArg: true,
+    helpArg: 'STRING',
+    parseArg: parseCommaSepStringNoEmpties
+});
+
+dashdash.addOptionType({
+    name: 'arrayOfCommaSepString',
+    takesArg: true,
+    helpArg: 'STRING',
+    parseArg: parseCommaSepStringNoEmpties,
+    array: true,
+    arrayFlatten: true
+});
+
+
 // ---- globals and constants
 
 var log = bunyan.createLogger({
@@ -59,7 +84,8 @@ var options = [
         names: ['parenthetical', 'p'],
         type: 'string',
         help: 'Add a parenthetical comment to the commit message. Typically '
-            + 'used for followup commits on an issue already commited to.'
+            + 'used for followup commits on an issue already commited to.',
+        helpArg: 'STR'
     },
     {
         names: ['update', 'u'],
@@ -69,14 +95,17 @@ var options = [
         'default': false
     },
     {
-        names: ['add-extra', 'a'],
-        type: 'string',
-        help: 'Add the given comma-separated list of issues to the commit.'
+        names: ['add-issues', 'a'],
+        type: 'arrayOfCommaSepString',
+        help: 'Add the given comma-separated list of issues to the commit.',
+        helpArg: 'ISSUES'
     },
     {
-        names: ['remove-extra', 'r'],
-        type: 'string',
-        help: 'Remove the given comma-separated list of issues from the commit.'
+        names: ['remove-issues', 'r'],
+        type: 'arrayOfCommaSepString',
+        // JSSTYLED
+        help: 'Remove the given comma-separated list of issues from the commit.',
+        helpArg: 'ISSUES'
     },
     // TODO
     //{
@@ -117,9 +146,10 @@ function usage() {
         'Gerrit at <https://cr.joyent.us>.',
         '',
         'Usage:',
-        '    grr [<options>] [<issue>]  # create or update a CR',
-        '    grr -D [<issue>]           # delete feature branch, switch to master',
-        '    grr -L                     # list outstanding CRs for this repo',
+        '    grr [<options>] [<issue> [<extra-issues>]]',
+        '                     # start or update a CR',
+        '    grr -D           # delete feature branch, switch to master',
+        '    grr -L           # list outstanding CRs for this repo',
         '',
         'Options:',
         help,
@@ -128,9 +158,9 @@ function usage() {
         'GitHub issue (the number, account/repo#num, or full URL).',
         '',
         'Examples:',
-        '    grr TOOLS-123    # Start or update a branch and CR for this ticket',
+        '    grr TOOLS-123    # Start or update a branch and CR for this issue',
         //'    grr --commit TOOLS-123    # ... also commit uncommited changes',
-        '    grr              # Update the current CR (using the cached ticket)',
+        '    grr              # Update the current CR (using the cached issue)',
 	'    grr -a TOOLS-124 # Add an extra issue to the current CR',
 	'    grr -r TOOLS-124 # Remove an extra issue from the current CR',
         '    grr -D           # All done. Del the branch and switch back to master'
@@ -160,15 +190,26 @@ function mainFinish(err) {
     }
 }
 
+function fatal(errmsg) {
+    console.error('grr: error: %s', errmsg);
+    process.exit(1);
+}
+
 
 // ---- mainline
 
 function main(argv) {
+    // 'actions' is plural because coming "submit" work might allow multiple.
+    var actions = [];
+    var issueArg;
+    var addExtraIssueArgs;
+    var removeExtraIssueArgs;
+    var opts;
+
     try {
-        var opts = parser.parse(process.argv);
+        opts = parser.parse(process.argv);
     } catch (e) {
-        console.error('grr: error: %s', e.message);
-        process.exit(1);
+        fatal(e.message);
     }
 
     if (opts.help) {
@@ -185,39 +226,70 @@ function main(argv) {
         log.src = true;
     }
 
-    var issueArg;
-    if (opts._args.length) {
-        issueArg = opts._args.shift();
-    }
-    if (opts._args.length) {
-        console.error('grr: error: extraneous arguments: %s', opts._args);
-        process.exit(1);
+    if (opts._args.length > 0 || opts.update || opts.add_issues ||
+        opts.remove_issues)
+    {
+        actions.push('update-or-create');
+        if (opts._args.length > 0) {
+            issueArg = opts._args.shift();
+        }
+        addExtraIssueArgs = opts._args.slice();
+        if (opts.add_issues) {
+            addExtraIssueArgs = addExtraIssueArgs.concat(opts.add_issues);
+        }
+        if (opts.remove_issues) {
+            removeExtraIssueArgs = opts.remove_issues;
+        }
     }
 
     if (opts['delete']) {
-        if (opts.add_extra || opts.remove_extra) {
-            console.error('extra issues specified when deleting');
-            process.exit(1);
+        if (actions.length) {
+            fatal('cannot delete (-D) when creating/updating a CR');
         }
+        actions.push('delete');
+    }
+
+    if (opts.list) {
+        if (actions.length) {
+            fatal('cannot list (-L) when deleting (-D) or '
+                + 'creating/updating a CR');
+        }
+        actions.push('list');
+    }
+
+    if (actions.length === 0) {
+        actions.push('update-or-create');
+    }
+
+    // Only one action at a time for now.
+    assert.equal(actions.length, 1,
+        'incorrect number of actions: ' + JSON.stringify(actions));
 
-        grr.grrDelete({
-            log: log,
-            issueArg: issueArg,
-            dryRun: opts.dry_run
-        }, mainFinish);
-    } else if (opts['list']) {
-        grr.grrList({log: log}, mainFinish);
-    } else {
-        grr.grrUpdateOrCreate({
-            log: log,
-            issueArg: issueArg,
-            parenthetical: opts.parenthetical,
-            addExtraIssuesArg: opts.add_extra,
-            removeExtraIssuesArg: opts.remove_extra,
-            dryRun: opts.dry_run,
-            commit: opts.commit,
-            update: opts.update
-        }, mainFinish);
+    switch (actions[0]) {
+        case 'list':
+            grr.grrList({log: log}, mainFinish);
+            break;
+        case 'delete':
+            grr.grrDelete({
+                log: log,
+                dryRun: opts.dry_run
+            }, mainFinish);
+            break;
+        case 'update-or-create':
+            grr.grrUpdateOrCreate({
+                log: log,
+                issueArg: issueArg,
+                parenthetical: opts.parenthetical,
+                addExtraIssueArgs: addExtraIssueArgs,
+                removeExtraIssueArgs: removeExtraIssueArgs,
+                dryRun: opts.dry_run,
+                commit: opts.commit,
+                update: opts.update
+            }, mainFinish);
+            break;
+        default:
+            fatal('what action is this? ' + actions[0]);
+            break;
     }
 }
 

lib/common.js

@@ -16,14 +16,6 @@ var VError = require('verror').VError;
 
 // ---- support stuff
 
-/// A split with sensible semantics for forEach().
-function fsplit(str, separator) {
-    return str.split(separator).filter(function (s) {
-        if (s !== '')
-            return s;
-    });
-}
-
 function objCopy(obj, target) {
     assert.object(obj, 'obj');
     assert.optionalObject(obj, 'target');
@@ -122,7 +114,6 @@ function forkExecWaitAndLog(opts, cb) {
 //---- exports
 
 module.exports = {
-    fsplit: fsplit,
     objCopy: objCopy,
     deepObjCopy: deepObjCopy,
     tildeSync: tildeSync,