diff --git a/lib/resolve/resolvers/GitFsResolver.js b/lib/resolve/resolvers/GitFsResolver.js index 5cd60dee..1cd11c43 100644 --- a/lib/resolve/resolvers/GitFsResolver.js +++ b/lib/resolve/resolvers/GitFsResolver.js @@ -19,19 +19,7 @@ mout.object.mixIn(GitFsResolver, GitResolver); // ----------------- -GitFsResolver.prototype._resolveSelf = function () { - return this._findResolution() - .then(this._readJson.bind(this, this._source)) - .then(this._copy.bind(this)) - .then(this._checkout.bind(this)) - .then(this._cleanup.bind(this)); -}; - -// ----------------- - -GitFsResolver.prototype._copy = function (meta) { - var ignore = meta.ignore; - +GitFsResolver.prototype._copy = function () { // Copy folder permissions return Q.nfcall(fs.stat, this._source) .then(function (stat) { @@ -39,10 +27,7 @@ GitFsResolver.prototype._copy = function (meta) { }.bind(this)) // Copy folder contents .then(function () { - return Q.nfcall(ncp, this._source, this._tempDir, { - // Don't copy ignored files - filter: ignore && ignore.length ? this._createIgnoreFilter(ignore) : null - }); + return Q.nfcall(ncp, this._source, this._tempDir); }.bind(this)); }; @@ -50,8 +35,12 @@ GitFsResolver.prototype._copy = function (meta) { GitFsResolver.prototype._checkout = function () { var resolution = this._resolution; - // Checkout resolution - return cmd('git', ['checkout', '-f', resolution.tag || resolution.branch || resolution.commit], { cwd: this._tempDir }) + // The checkout process could be similar to the GitRemoteResolver by prepending file:// to the source + // But from my performance measures, it's faster to copy the folder and just checkout in there + + // Copy files to the temporary directory first + return this._copy() + .then(cmd.bind(cmd, 'git', ['checkout', '-f', resolution.tag || resolution.branch || resolution.commit], { cwd: this._tempDir })) // Cleanup unstagged files .then(cmd.bind(cmd, 'git', ['clean', '-f', '-d'], { cwd: this._tempDir })); }; @@ -67,7 +56,10 @@ GitFsResolver.fetchRefs = function (source) { return cmd('git', ['show-ref', '--tags', '--heads'], { cwd : source }) .then(function (stdout) { // Make them an array - var refs = stdout.toString().trim().split('\n'); + var refs = stdout.toString() + .trim() // Trim trailing and leading spaces + .replace(/[\t ]+/g, ' ') // Standardize spaces (some git versions make tabs, other spaces) + .split(/\r?\n/); // Split lines into an array this._refs = this._refs || {}; return this._refs[source] = refs; diff --git a/lib/resolve/resolvers/GitRemoteResolver.js b/lib/resolve/resolvers/GitRemoteResolver.js index d5d896ce..f3041c43 100644 --- a/lib/resolve/resolvers/GitRemoteResolver.js +++ b/lib/resolve/resolvers/GitRemoteResolver.js @@ -40,7 +40,10 @@ GitRemoteResolver.fetchRefs = function (source) { return cmd('git', ['ls-remote', '--tags', '--heads', source]) .then(function (stdout) { // Make them an array - var refs = stdout.toString().trim().split('\n'); + var refs = stdout.toString() + .trim() // Trim trailing and leading spaces + .replace(/[\t ]+/g, ' ') // Standardize spaces (some git versions make tabs, other spaces) + .split(/\r?\n/); // Split lines into an array this._refs = this._refs || {}; return this._refs[source] = refs; diff --git a/lib/resolve/resolvers/GitResolver.js b/lib/resolve/resolvers/GitResolver.js index 99e1def0..2357a9ce 100644 --- a/lib/resolve/resolvers/GitResolver.js +++ b/lib/resolve/resolvers/GitResolver.js @@ -23,8 +23,13 @@ util.inherits(GitResolver, Resolver); GitResolver.prototype._resolveSelf = function () { return this._findResolution() - .then(this._checkout.bind(this)) - .then(this._cleanup.bind(this)); + .then(function () { + return this._checkout() + // Always run cleanup after checkout to ensure that .git is removed! + // If it's not removed, problems might arrise when the "tmp" module attemps + // to delete the temporary folder + .fin(this._cleanup.bind(this)); + }.bind(this)); }; GitResolver.prototype.hasNew = function (canonicalPkg) { diff --git a/test/resolve/resolvers/gitFsResolver.js b/test/resolve/resolvers/gitFsResolver.js index 0b2ba152..dd4add10 100644 --- a/test/resolve/resolvers/gitFsResolver.js +++ b/test/resolve/resolvers/gitFsResolver.js @@ -1,7 +1,8 @@ -var path = require('path'); -var fs = require('fs'); var expect = require('expect.js'); var path = require('path'); +var fs = require('fs'); +var path = require('path'); +var cmd = require('../../../lib/util/cmd'); var GitFsResolver = require('../../../lib/resolve/resolvers/GitFsResolver'); describe('GitFsResolver', function () { @@ -90,10 +91,21 @@ describe('GitFsResolver', function () { }); it('should remove any untracked files and directories', function (next) { - var resolver = new GitFsResolver(testPackage, { target: '7339c38f5874129504b83650fbb2d850394573e9' }); + var resolver = new GitFsResolver(testPackage, { target: '7339c38f5874129504b83650fbb2d850394573e9' }), + file = path.join(testPackage, 'new-file'), + dir = path.join(testPackage, 'new-dir'); - fs.writeFileSync(path.join(testPackage, 'new-file'), 'foo'); - fs.mkdir(path.join(testPackage, 'new-dir')); + fs.writeFileSync(file, 'foo'); + fs.mkdir(dir); + + function cleanup(err) { + fs.unlinkSync(file); + fs.rmdirSync(dir); + + if (err) { + throw err; + } + } resolver.resolve() .then(function (dir) { @@ -103,17 +115,61 @@ describe('GitFsResolver', function () { expect(files).to.not.contain('new-file'); expect(files).to.not.contain('new-dir'); + + cleanup(); + next(); + }) + .then(null, cleanup) + .done(); + }); + + it('should leave the original repository untouched', function (next) { + // Switch to master + cmd('git', ['checkout', 'master'], { cwd: testPackage }) + // Resolve to some-branch + .then(function () { + var resolver = new GitFsResolver(testPackage, { target: 'some-branch' }); + return resolver.resolve(); + }) + // Check if the original branch is still the master one + .then(function () { + return cmd('git', ['branch', '--color=never'], { cwd: testPackage }) + .then(function (stdout) { + expect(stdout).to.contain('* master'); + }); + }) + // Check if git status is empty + .then(function () { + return cmd('git', ['status', '--porcelain'], { cwd: testPackage }) + .then(function (stdout) { + stdout = stdout.trim(); + expect(stdout).to.equal(''); + next(); + }); + }) + .done(); + }); + + it('should copy source folder permissions', function (next) { + var mode0755; + + // Change testPackage dir to 0755 + fs.chmodSync(testPackage, 0755); + mode0755 = fs.statSync(testPackage).mode; + + var resolver = new GitFsResolver(testPackage, { target: 'some-branch' }); + + resolver.resolve() + .then(function (dir) { + // Check if temporary dir is 0755 instead of default 0777 & ~process.umask() + var stat = fs.statSync(dir); + expect(stat.mode).to.equal(mode0755); next(); }) .done(); }); }); - describe('._copy', function () { - it('should copy files from the source to the temporary directory'); - it('should not copy over the files specified in the ignore list'); - }); - describe('#fetchRefs', function () { afterEach(cleanInternalResolverCache); diff --git a/test/resolve/resolvers/gitRemoteResolver.js b/test/resolve/resolvers/gitRemoteResolver.js index 6cf8ac44..afdd34ff 100644 --- a/test/resolve/resolvers/gitRemoteResolver.js +++ b/test/resolve/resolvers/gitRemoteResolver.js @@ -96,16 +96,16 @@ describe('GitRemoteResolver', function () { GitRemoteResolver.fetchRefs('file://' + testPackage) .then(function (refs) { expect(refs).to.eql([ - 'f99467d1069892ea639b6a3d2afdbff6ac62f44e\trefs/heads/master', - '8b03dbbe20e0bc4f1fae2811ea0063121eb1b155\trefs/heads/some-branch', - '122ac45fd22671a23cf77055a32d06d5a7baedd0\trefs/tags/0.0.1', - '19b3a35cc7fded9a8a60d5b8fc0d18eb4940c476\trefs/tags/0.0.1^{}', - '34dd75a11e686be862844996392e96e9457c7467\trefs/tags/0.0.2', - 'ddc6ea571c49c1ab8bb213fda18efdfe2bc8dd00\trefs/tags/0.0.2^{}', - '92327598500f115d09ab14f16cde23718fc87658\trefs/tags/0.1.0', - 'b273e321ebc69381be2780668a22e28bec9e2b07\trefs/tags/0.1.0^{}', - '192bc846a342eb8ae62bb1a54d1394959e6fcd92\trefs/tags/0.1.1', - 'f99467d1069892ea639b6a3d2afdbff6ac62f44e\trefs/tags/0.1.1^{}' + 'f99467d1069892ea639b6a3d2afdbff6ac62f44e refs/heads/master', + '8b03dbbe20e0bc4f1fae2811ea0063121eb1b155 refs/heads/some-branch', + '122ac45fd22671a23cf77055a32d06d5a7baedd0 refs/tags/0.0.1', + '19b3a35cc7fded9a8a60d5b8fc0d18eb4940c476 refs/tags/0.0.1^{}', + '34dd75a11e686be862844996392e96e9457c7467 refs/tags/0.0.2', + 'ddc6ea571c49c1ab8bb213fda18efdfe2bc8dd00 refs/tags/0.0.2^{}', + '92327598500f115d09ab14f16cde23718fc87658 refs/tags/0.1.0', + 'b273e321ebc69381be2780668a22e28bec9e2b07 refs/tags/0.1.0^{}', + '192bc846a342eb8ae62bb1a54d1394959e6fcd92 refs/tags/0.1.1', + 'f99467d1069892ea639b6a3d2afdbff6ac62f44e refs/tags/0.1.1^{}' ]); next(); }) @@ -128,15 +128,15 @@ describe('GitRemoteResolver', function () { }) .then(function (refs) { expect(refs).to.eql([ - '8b03dbbe20e0bc4f1fae2811ea0063121eb1b155\trefs/heads/some-branch', - '122ac45fd22671a23cf77055a32d06d5a7baedd0\trefs/tags/0.0.1', - '19b3a35cc7fded9a8a60d5b8fc0d18eb4940c476\trefs/tags/0.0.1^{}', - '34dd75a11e686be862844996392e96e9457c7467\trefs/tags/0.0.2', - 'ddc6ea571c49c1ab8bb213fda18efdfe2bc8dd00\trefs/tags/0.0.2^{}', - '92327598500f115d09ab14f16cde23718fc87658\trefs/tags/0.1.0', - 'b273e321ebc69381be2780668a22e28bec9e2b07\trefs/tags/0.1.0^{}', - '192bc846a342eb8ae62bb1a54d1394959e6fcd92\trefs/tags/0.1.1', - 'f99467d1069892ea639b6a3d2afdbff6ac62f44e\trefs/tags/0.1.1^{}' + '8b03dbbe20e0bc4f1fae2811ea0063121eb1b155 refs/heads/some-branch', + '122ac45fd22671a23cf77055a32d06d5a7baedd0 refs/tags/0.0.1', + '19b3a35cc7fded9a8a60d5b8fc0d18eb4940c476 refs/tags/0.0.1^{}', + '34dd75a11e686be862844996392e96e9457c7467 refs/tags/0.0.2', + 'ddc6ea571c49c1ab8bb213fda18efdfe2bc8dd00 refs/tags/0.0.2^{}', + '92327598500f115d09ab14f16cde23718fc87658 refs/tags/0.1.0', + 'b273e321ebc69381be2780668a22e28bec9e2b07 refs/tags/0.1.0^{}', + '192bc846a342eb8ae62bb1a54d1394959e6fcd92 refs/tags/0.1.1', + 'f99467d1069892ea639b6a3d2afdbff6ac62f44e refs/tags/0.1.1^{}' ]); next(); }) diff --git a/test/resolve/resolvers/gitResolver.js b/test/resolve/resolvers/gitResolver.js index f8052155..162599f5 100644 --- a/test/resolve/resolvers/gitResolver.js +++ b/test/resolve/resolvers/gitResolver.js @@ -601,6 +601,7 @@ describe('GitResolver', function () { }); afterEach(function (next) { + cleanInternalResolverCache(); // Need to chmodr before removing..at least on windows // because .git has some read only files chmodr(tempDir, 0777, function () { @@ -640,6 +641,36 @@ describe('GitResolver', function () { }) .done(); }); + + it('should sill run even if _checkout fails for some reason', function (next) { + var resolver = new GitResolver('foo'), + called = false; + + GitResolver.fetchRefs = function () { + return Q.resolve([ + 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/master' + ]); + }; + + resolver._tempDir = tempDir; + resolver._checkout = function () { + return Q.reject(new Error('Some error')); + }; + + resolver._cleanup = function () { + called = true; + return GitResolver.prototype._cleanup.apply(this, arguments); + }; + + resolver.resolve() + .then(function () { + next(new Error('Should have failed')); + }, function () { + expect(called).to.be(true); + next(); + }) + .done(); + }); }); describe('._savePkgMeta', function () { diff --git a/test/test.js b/test/test.js index 3abc9256..2ee58545 100644 --- a/test/test.js +++ b/test/test.js @@ -49,7 +49,6 @@ if (process.argv[1] && !/mocha/.test(process.argv[1])) { //testGitFsResolver(); //testGitRemoteResolverNoTags(); } else { - // Cleanup the uncaughtException added by the tmp module // It messes with the mocha uncaughtException event to caught errors process.removeAllListeners('uncaughtException');