From e704733743bb56ffce548e91ad91745d4d425cc0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andre=CC=81=20Cruz?= <andremiguelcruz@msn.com>
Date: Fri, 7 Jun 2013 13:05:07 +0100
Subject: [PATCH] Read, use and update persistent resolutions.

Add --save-resolutions to deal with this.
Also:
  - Adjust logging when using compact
  - Minor code adjustments.
---
 README.md                               |  11 +--
 lib/commands/install.js                 |   3 +-
 lib/commands/update.js                  |   3 +-
 lib/core/Manager.js                     | 123 ++++++++++++++++--------
 lib/core/Project.js                     |  51 ++++------
 lib/core/resolvers/FsResolver.js        |   8 +-
 lib/core/resolvers/GitRemoteResolver.js |   2 +-
 lib/core/resolvers/UrlResolver.js       |   4 +-
 lib/renderers/StandardRenderer.js       |  10 +-
 templates/json/help-install.json        |   5 +
 templates/json/help-update.json         |   5 +
 templates/std/conflict-resolved.std     |   3 +-
 templates/std/conflict.std              |   3 +
 templates/std/help-generic.std          |   2 +-
 templates/std/help.std                  |   4 +-
 15 files changed, 146 insertions(+), 91 deletions(-)

diff --git a/README.md b/README.md
index fd2fe2d2..194777d0 100644
--- a/README.md
+++ b/README.md
@@ -126,15 +126,16 @@ If `config` is not passed, the default one will be used.
 
 `Manager#setProduction(production)`: Manager
 
-Enable/disable production (read of devDependencies).
+Enable/disable `production` (read of devDependencies).
 
 `Manager#getResolutions()`: Object
 
 Get the current resolutions object.
 
-`Manager#setResolutions(resolutions)`: Manager
+`Manager#setResolutions(resolutions, save)`: Manager
 
-Set the resolutions to be used on conflicts.
+Set the `resolutions` to be used on conflicts.
+If `save` is true, the `resolutions` object will be updated.
 
 `Manager#configure(targets, resolved, installed)`: Manager
 
@@ -144,9 +145,7 @@ Configures the manager with `targets` and `installed`:
 - `resolved`: object of resolved packages (keys are names and values the reconstructed decomposed endpoints)
 - `installed`: object of currently installed packages (keys are names and values the package metas)
 
-`targets` and `resolved` decomposed endpoints may contain a dependants key to specify dependants.
-Those will be used when presenting choices to the user on conflicts.
-
+`targets` and `resolved` decomposed endpoints should contain `dependency` and `dependants` keys correctly set.
 If the Manager is already working, the promise is immediately rejected.
 
 `Manager#resolve()`: Promise
diff --git a/lib/commands/install.js b/lib/commands/install.js
index a3e71d43..eb4a5046 100644
--- a/lib/commands/install.js
+++ b/lib/commands/install.js
@@ -47,9 +47,10 @@ install.line = function (argv) {
 install.options = function (argv) {
     return cli.readOptions({
         'help': { type: Boolean, shorthand: 'h' },
+        'production': { type: Boolean, shorthand: 'p' },
         'save': { type: Boolean, shorthand: 'S' },
         'save-dev': { type: Boolean, shorthand: 'D' },
-        'production': { type: Boolean, shorthand: 'p' }
+        'save-resolutions': { type: Boolean, shorthand: 'r' }
     }, argv);
 };
 
diff --git a/lib/commands/update.js b/lib/commands/update.js
index cd6bd795..74e7c6e8 100644
--- a/lib/commands/update.js
+++ b/lib/commands/update.js
@@ -48,7 +48,8 @@ update.line = function (argv) {
 update.options = function (argv) {
     return cli.readOptions({
         'help': { type: Boolean, shorthand: 'h' },
-        'production': { type: Boolean, shorthand: 'p' }
+        'production': { type: Boolean, shorthand: 'p' },
+        'save-resolutions': { type: Boolean, shorthand: 'r' }
     }, argv);
 };
 
diff --git a/lib/core/Manager.js b/lib/core/Manager.js
index cad9ecd0..6180c811 100644
--- a/lib/core/Manager.js
+++ b/lib/core/Manager.js
@@ -30,8 +30,9 @@ Manager.prototype.getResolutions = function () {
     return this._resolutions;
 };
 
-Manager.prototype.setResolutions = function (resolutions) {
+Manager.prototype.setResolutions = function (resolutions, save) {
     this._resolutions = resolutions || {};
+    this._saveResolutions = !!save;
     return this;
 };
 
@@ -74,6 +75,7 @@ Manager.prototype.resolve = function () {
     this._fetching = {};
     this._nrFetching = 0;
     this._failed = {};
+    this._conflicted = {};
     this._hasFailed = false;
     this._deferred = Q.defer();
 
@@ -111,11 +113,7 @@ Manager.prototype.install = function () {
             var promise;
             var dest;
             var release = decEndpoint.pkgMeta._release;
-            var data = {
-                endpoint: mout.object.pick(decEndpoint, ['name', 'source', 'target']),
-                canonicalPkg: decEndpoint.canonicalPkg,
-                pkgMeta: decEndpoint.pkgMeta
-            };
+            var data = that._toData(decEndpoint);
 
             that._logger.action('install', name + (release ? '#' + release : ''), data);
 
@@ -134,11 +132,11 @@ Manager.prototype.install = function () {
         return Q.all(promises);
     })
     .then(function () {
-        // Resolve with an object where keys are names and values
-        // are the package metas
         return mout.object.map(that._dissected, function (decEndpoint) {
-            return decEndpoint.pkgMeta;
-        });
+            var data = this._toData(decEndpoint);
+            data.dependencies = mout.object.map(decEndpoint.dependencies, this._toData, this);
+            return data;
+        }, that);
     })
     .fin(function () {
         this._working = false;
@@ -340,6 +338,8 @@ Manager.prototype._failFast = function () {
 };
 
 Manager.prototype._parseDependencies = function (decEndpoint, pkgMeta, jsonKey) {
+    decEndpoint.dependencies = decEndpoint.dependencies || {};
+
     // Parse package dependencies
     mout.object.forOwn(pkgMeta[jsonKey], function (value, key) {
         var resolved;
@@ -347,6 +347,7 @@ Manager.prototype._parseDependencies = function (decEndpoint, pkgMeta, jsonKey)
         var compatible;
         var childDecEndpoint = endpointParser.json2decomposed(key, value);
 
+        decEndpoint.dependencies[key] = childDecEndpoint;
         childDecEndpoint.dependants = childDecEndpoint.dependants || [];
         childDecEndpoint.dependants.push(decEndpoint);
 
@@ -451,22 +452,40 @@ Manager.prototype._dissect = function () {
         });
     }, this);
 
-    // TODO: Remove resolutions that are not needed anymore (emitting a warn)
-    // TODO: Change the final promise result to an array of decEndpoints
-    //       with dependencies included to be able to do a npm like tree
-
     promise
     .then(function () {
+        // Look for extraneous resolutions
+        mout.object.forOwn(this._resolutions, function (resolution, name) {
+            if (this._conflicted[name]) {
+                return;
+            }
+
+            if (this._saveResolutions) {
+                this._logger.info('resolution', 'Removed unnecessary ' + name + '#' + resolution + ' resolution', {
+                    package: name,
+                    resolution: resolution,
+                    action: 'delete'
+                });
+                delete this._resolutions[name];
+            } else {
+                this._logger.warn('resolution', 'Unnecessary ' + name + '#' + resolution + ' resolution', {
+                    package: name,
+                    resolution: resolution
+                });
+            }
+        }, this);
+
         // Filter only packages that need to be installed
         this._dissected = mout.object.filter(suitables, function (decEndpoint, name) {
             var installedMeta = this._installed[name];
             return !installedMeta || installedMeta._release !== decEndpoint.pkgMeta._release;
         }, this);
 
-        // Resolve with the package metas of the dissected object
         return mout.object.map(this._dissected, function (decEndpoint) {
-            return decEndpoint.pkgMeta;
-        });
+            var data = this._toData(decEndpoint);
+            data.dependencies = mout.object.map(decEndpoint.dependencies, this._toData, this);
+            return data;
+        }, this);
     }.bind(this))
     .then(this._deferred.resolve, this._deferred.reject);
 };
@@ -474,6 +493,7 @@ Manager.prototype._dissect = function () {
 Manager.prototype._electSuitable = function (name, semvers, nonSemvers) {
     var suitable;
     var resolution;
+    var unresolvable;
     var dataPicks;
     var choices;
     var picks = [];
@@ -507,6 +527,9 @@ Manager.prototype._electSuitable = function (name, semvers, nonSemvers) {
         picks.push.apply(picks, semvers);
     }
 
+    // At this point, there's a conflict
+    this._conflicted[name] = true;
+
     // Sort picks by version/release
     picks.sort(function (pick1, pick2) {
         var version1 = pick1.pkgMeta.version;
@@ -543,23 +566,20 @@ Manager.prototype._electSuitable = function (name, semvers, nonSemvers) {
 
     // Prepare data to be sent bellow
     dataPicks = picks.map(function (pick) {
-        return {
-            endpoint: mout.object.pick(pick, ['name', 'source', 'target']),
-            pkgMeta: pick.pkgMeta,
-            canonicalPkg: pick.canonicalPkg,
-            dependants: pick.dependants.map(function (dependant) {
-                return {
-                    endpoint: mout.object.pick(dependant, ['name', 'source', 'target']),
-                    pkgMeta: dependant.pkgMeta,
-                    canonicalPkg: dependant.canonicalPkg
-                };
-            })
-        };
-    });
+        var dataPick = this._toData(pick);
+        dataPick.dependants = pick.dependants.map(this._toData.bind(this), this);
+        return dataPick;
+    }, this);
 
     // Check if there's a resolution that resolves the conflict
+    // Note that if a target is marked as unresolvable, the resolution has
+    // no effect
     resolution = this._resolutions[name];
-    if (resolution) {
+    unresolvable = mout.object.find(this._targets, function (target) {
+        return target.name === name && target.unresolvable;
+    });
+
+    if (resolution && !unresolvable) {
         if (semver.valid(resolution) != null || semver.validRange(resolution) != null) {
             suitable = mout.array.findIndex(picks, function (pick) {
                 return semver.satisfies(pick.pkgMeta.version, resolution);
@@ -570,15 +590,21 @@ Manager.prototype._electSuitable = function (name, semvers, nonSemvers) {
             });
         }
 
-        if (suitable) {
-            this._logger.conflict('resolved', 'Unable to find suitable version for ' + name, {
+        if (!suitable) {
+            this._logger.conflict('warn', 'Unsuitable resolution for ' + name + ': ' + resolution, {
                 name: name,
                 picks: dataPicks,
-                resolution: resolution,
-                suitable: dataPicks[suitable]
+                resolution: resolution
             });
-            return Q.resolve(picks[suitable]);
         }
+
+        this._logger.conflict('solved', 'Unable to find suitable version for ' + name, {
+            name: name,
+            picks: dataPicks,
+            resolution: resolution,
+            suitable: dataPicks[suitable]
+        });
+        return Q.resolve(picks[suitable]);
     }
 
     // If interactive is disabled, error out
@@ -592,25 +618,44 @@ Manager.prototype._electSuitable = function (name, semvers, nonSemvers) {
     // At this point the user needs to make a decision
     this._logger.conflict('incompatible', 'Unable to find suitable version for ' + name, {
         name: name,
-        picks: dataPicks
+        picks: dataPicks,
+        saveResolutions: this._saveResolutions
     });
 
     choices = picks.map(function (pick, index) { return index + 1; });
     return Q.nfcall(promptly.choose, 'Choice:', choices)
     .then(function (choice) {
         var pick = picks[choice - 1];
+        var resolution;
 
         // Store choice into resolutions
         if (pick.target === '*') {
-            this._resolutions[name] = pick.pkgMeta._release || '*';
+            resolution = pick.pkgMeta._release || '*';
         } else {
-            this._resolutions[name] = pick.target;
+            resolution = pick.target;
+        }
+
+        if (this._saveResolutions) {
+            this._logger.info('resolution', 'Saved ' + name + '#' + resolution + ' as the resolution', {
+                package: name,
+                resolution: resolution,
+                action: this._resolutions[name] ? 'edit' : 'add'
+            });
+            this._resolutions[name] = resolution;
         }
 
         return pick;
     }.bind(this));
 };
 
+Manager.prototype._toData = function (decEndpoint) {
+    return {
+        endpoint: mout.object.pick(decEndpoint, ['name', 'source', 'target']),
+        canonicalPkg: decEndpoint.canonicalPkg,
+        pkgMeta: decEndpoint.pkgMeta
+    };
+};
+
 Manager.prototype._getCap = function (comparators, side) {
     var matches;
     var candidate;
diff --git a/lib/core/Project.js b/lib/core/Project.js
index b5cefc6d..c25666bf 100644
--- a/lib/core/Project.js
+++ b/lib/core/Project.js
@@ -37,11 +37,15 @@ Project.prototype.install = function (endpoints, options) {
     options = options || {};
     this._production = !!options.production;
 
+    if (options.saveResolutions == null) {
+        this._saveResolutions = !!(options.save || options.saveDev);
+    } else {
+        this._saveResolutions = !!options.saveResolutions;
+    }
+
     // Analyse the project
     return this.analyse()
     .spread(function (json, tree, flattened) {
-        var promise;
-
         // Walk down the tree adding missing as targets
         that._walkTree(tree, function (node, name) {
             if (node.missing) {
@@ -55,7 +59,9 @@ Project.prototype.install = function (endpoints, options) {
         if (endpoints) {
             endpoints.forEach(function (endpoint) {
                 var decEndpoint = endpointParser.decompose(endpoint);
-                decEndpoint.specified = true;
+                // Mark as unresolvable so that a conflict for this target always require
+                // a choice
+                decEndpoint.unresolvable = true;
                 targets.push(decEndpoint);
             });
         }
@@ -70,25 +76,7 @@ Project.prototype.install = function (endpoints, options) {
         }
 
         // Bootstrap the process
-        promise = that._bootstrap(targets, resolved, flattened);
-
-        // If there are targets configured, listen to when they
-        // resolve in order to remove any associated resolution
-        // This can only be done at this step because endpoint names
-        // are not fully known before
-        if (endpoints) {
-            promise = promise.progress(function (decEndpoint) {
-                var resolutions;
-
-                if (decEndpoint.specified) {
-                    resolutions = that._manager.getResolutions();
-                    delete resolutions[decEndpoint.name];
-                    delete decEndpoint.specified;
-                }
-            });
-        }
-
-        return promise;
+        return that._bootstrap(targets, resolved, flattened);
     })
     .then(function (installed) {
         // Handle save and saveDev options
@@ -136,6 +124,7 @@ Project.prototype.update = function (names, options) {
 
     options = options || {};
     this._production = !!options.production;
+    this._saveResolutions = !!options.saveResolutions;
 
     // Analyse the project
     return this.analyse()
@@ -360,22 +349,22 @@ Project.prototype._bootstrap = function (targets, resolved, flattened) {
     // Configure the manager and kick in the resolve process
     return this._manager
     .setProduction(this._production)
-    .setResolutions(this._json.resolutions)
+    .setResolutions(this._json.resolutions, this._saveResolutions)
     .configure(targets, resolved, installed)
     .resolve()
+    // Save resolutions into the json
     .then(function () {
         var resolutions = this._manager.getResolutions();
 
-        // Update resolutions
-        if (mout.object.size(resolutions)) {
-            this._json.resolutions = resolutions;
-        } else {
+        // If empty, delete key
+        if (!mout.object.size(resolutions)) {
             delete this._json.resolutions;
+        } else {
+            this._json.resolutions = resolutions;
         }
-
-        // Install resolved ones
-        return this._manager.install();
-    }.bind(this));
+    }.bind(this))
+    // Install resolved ones
+    .then(this._manager.install.bind(this._manager));
 };
 
 Project.prototype._readJson = function () {
diff --git a/lib/core/resolvers/FsResolver.js b/lib/core/resolvers/FsResolver.js
index 0ec059b7..ca88f926 100644
--- a/lib/core/resolvers/FsResolver.js
+++ b/lib/core/resolvers/FsResolver.js
@@ -26,12 +26,12 @@ mout.object.mixIn(FsResolver, Resolver);
 
 // -----------------
 
-// TODO: should we store latest mtimes in the resolution and compare?
-//       this would be beneficial when copying big files/folders
+// TODO: Should we store latest mtimes in the resolution and compare?
+//       This would be beneficial when copying big files/folders
 
-// TODO: there's room for improvement by using streams if the source
+// TODO: There's room for improvement by using streams if the source
 //       is an archive file, by piping read stream to the zip extractor
-//       this will likely increase the complexity of code but might worth it
+//       This will likely increase the complexity of code but might worth it
 FsResolver.prototype._resolve = function () {
     return this._readJson(this._source)
     .then(this._copy.bind(this))
diff --git a/lib/core/resolvers/GitRemoteResolver.js b/lib/core/resolvers/GitRemoteResolver.js
index 3c4c1415..3f5ab76b 100644
--- a/lib/core/resolvers/GitRemoteResolver.js
+++ b/lib/core/resolvers/GitRemoteResolver.js
@@ -58,7 +58,7 @@ GitRemoteResolver.prototype._checkout = function () {
 GitRemoteResolver.fetchRefs = function (source) {
     var cache;
 
-    // TODO: normalize source because of the various available protocols?
+    // TODO: Normalize source because of the various available protocols?
     this._refs = this._refs || {};
 
     cache = this._refs[source];
diff --git a/lib/core/resolvers/UrlResolver.js b/lib/core/resolvers/UrlResolver.js
index cad44140..2ba14719 100644
--- a/lib/core/resolvers/UrlResolver.js
+++ b/lib/core/resolvers/UrlResolver.js
@@ -82,9 +82,9 @@ UrlResolver.prototype._hasNew = function (canonicalPkg, pkgMeta) {
     });
 };
 
-// TODO: there's room for improvement by using streams if the url
+// TODO: There's room for improvement by using streams if the url
 //       is an archive file, by piping read stream to the zip extractor
-//       this will likely increase the complexity of code but might worth it
+//       This will likely increase the complexity of code but might worth it
 
 UrlResolver.prototype._resolve = function () {
     // Download
diff --git a/lib/renderers/StandardRenderer.js b/lib/renderers/StandardRenderer.js
index da856214..cade18f8 100644
--- a/lib/renderers/StandardRenderer.js
+++ b/lib/renderers/StandardRenderer.js
@@ -140,7 +140,7 @@ StandardRenderer.prototype._incompatibleLog = function (log) {
     this._write(process.stdout, '\n');
 };
 
-StandardRenderer.prototype._resolvedLog = function (log) {
+StandardRenderer.prototype._solvedLog = function (log) {
     this._incompatibleLog(log);
 };
 
@@ -176,8 +176,13 @@ StandardRenderer.prototype._prefix = function (log) {
     var id = log.id;
     var idColor = this._colors[log.level] || this._colors['default'];
 
-    // If there's not enough space, print only the id
     if (this._compact) {
+        // If there's not enough space for the id, adjust it
+        // for subsequent logs
+        if (id.length > this._sizes.id) {
+            this._sizes.id = id.length += this._sizes.sumup;
+        }
+
         return mout.string.rpad(id, this._sizes.id)[idColor];
     }
 
@@ -194,6 +199,7 @@ StandardRenderer.prototype._prefix = function (log) {
 
     // Ensure at least one space between the label and the id
     if (nrSpaces < 1) {
+        // Also adjust the label size for subsequent logs
         this._sizes.label = label.length + this._sizes.sumup;
         nrSpaces = this._sizes.id + this._sizes.label - length;
     }
diff --git a/templates/json/help-install.json b/templates/json/help-install.json
index 244e3240..d197ad30 100644
--- a/templates/json/help-install.json
+++ b/templates/json/help-install.json
@@ -20,6 +20,11 @@
             "shorthand":   "-D",
             "flag":        "--save-dev",
             "description": "Save installed packages into the project's bower.json devDependencies"
+        },
+        {
+            "shorthand":   "-r",
+            "flag":        "--save-resolutions",
+            "description": "Save resolutions chosen to resolve conflicts"
         }
     ]
 }
\ No newline at end of file
diff --git a/templates/json/help-update.json b/templates/json/help-update.json
index ba3691f1..67dc4077 100644
--- a/templates/json/help-update.json
+++ b/templates/json/help-update.json
@@ -9,6 +9,11 @@
             "shorthand":   "-p",
             "flag":        "--production",
             "description": "Do not install project devDependencies"
+        },
+        {
+            "shorthand":   "-r",
+            "flag":        "--save-resolutions",
+            "description": "Save resolutions chosen to resolve conflicts"
         }
     ]
 }
\ No newline at end of file
diff --git a/templates/std/conflict-resolved.std b/templates/std/conflict-resolved.std
index a8791eab..52534cfe 100644
--- a/templates/std/conflict-resolved.std
+++ b/templates/std/conflict-resolved.std
@@ -1,8 +1,9 @@
 {{#yellow}}Please note that,{{/yellow}}
 {{#condense}}
 {{#each picks}}
-    {{#white}}{{dependants}}{{/white}} depend on {{#cyan}}{{endpoint.name}}#{{endpoint.target}}{{/cyan}}{{#if pkgMeta._release}} which resolved to {{#white}}{{pkgMeta._release}}{{/white}}{{/if}}
+    {{#if dependants}}{{#white}}{{dependants}}{{/white}}{{else}}none{{/if}} depend on {{#cyan}}{{endpoint.name}}#{{endpoint.target}}{{/cyan}}{{#if pkgMeta._release}} which resolved to {{#white}}{{pkgMeta._release}}{{/white}}{{/if}}
 {{/each}}
 {{/condense}}
 
 Resort to using {{#cyan}}{{suitable.endpoint.name}}#{{resolution}}{{/cyan}} which resolved to {{#white}}{{suitable.pkgMeta._release}}{{/white}}
+Code incompatibilities may occur.
diff --git a/templates/std/conflict.std b/templates/std/conflict.std
index 0f0f197f..9c14b9c4 100644
--- a/templates/std/conflict.std
+++ b/templates/std/conflict.std
@@ -4,3 +4,6 @@
     {{#magenta}}{{sum @index 1}}){{/magenta}} {{#cyan}}{{endpoint.name}}#{{endpoint.target}}{{/cyan}}{{#if pkgMeta._release}} which resolved to {{#white}}{{pkgMeta._release}}{{/white}}{{/if}}{{#if dependants}} and has {{#white}}{{dependants}}{{/white}} as dependants{{/if}}
 {{/each}}
 {{/condense}}
+{{#unless saveResolutions}}
+Use --save-resolutions to persist your choice to the bower.json
+{{/unless}}
\ No newline at end of file
diff --git a/templates/std/help-generic.std b/templates/std/help-generic.std
index 5cc230ae..fc39b63d 100644
--- a/templates/std/help-generic.std
+++ b/templates/std/help-generic.std
@@ -11,7 +11,7 @@ Options:
 
 {{#condense}}
 {{#each options}}
-    {{#yellow}}{{#rpad length="20"}}{{#if shorthand}}{{shorthand}}, {{/if}}{{flag}}{{/rpad}}{{/yellow}} {{description}}
+    {{#yellow}}{{#rpad length="23"}}{{#if shorthand}}{{shorthand}}, {{/if}}{{flag}}{{/rpad}}{{/yellow}} {{description}}
 {{/each}}
 {{/condense}}
 
diff --git a/templates/std/help.std b/templates/std/help.std
index ad052916..7f1127eb 100644
--- a/templates/std/help.std
+++ b/templates/std/help.std
@@ -11,7 +11,7 @@ Commands:
 
 {{#condense}}
 {{#each commands}}
-    {{#rpad length="20"}}{{@key}}{{/rpad}} {{.}}
+    {{#rpad length="23"}}{{@key}}{{/rpad}} {{.}}
 {{/each}}
 {{/condense}}
 
@@ -19,7 +19,7 @@ Options:
 
 {{#condense}}
 {{#each options}}
-    {{#yellow}}{{#rpad length="20"}}{{#if shorthand}}{{shorthand}}, {{/if}}{{flag}}{{/rpad}}{{/yellow}} {{description}}
+    {{#yellow}}{{#rpad length="23"}}{{#if shorthand}}{{shorthand}}, {{/if}}{{flag}}{{/rpad}}{{/yellow}} {{description}}
 {{/each}}
 {{/condense}}