From 3d6025281ebac4022a9237da1a5ecda51fc1e693 Mon Sep 17 00:00:00 2001 From: joseg Date: Mon, 29 Feb 2016 12:15:52 -0700 Subject: [PATCH 1/6] fix tree.walk() and add test --- lib/tree.js | 5 ++- test/tests/tree.js | 77 ++++++++++++++++++++++++++++++++++ test/utils/repository_setup.js | 16 +++++-- 3 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 test/tests/tree.js diff --git a/lib/tree.js b/lib/tree.js index 138099c2e..a9f00fe40 100644 --- a/lib/tree.js +++ b/lib/tree.js @@ -127,6 +127,7 @@ Tree.prototype.walk = function(blobsOnly) { var event = new events.EventEmitter(); var total = 1; + var entries = []; // This looks like a DFS, but it is a BFS because of implicit queueing in // the recursive call to `entry.getTree(bfs)` @@ -136,8 +137,8 @@ Tree.prototype.walk = function(blobsOnly) { if (error) { return event.emit("error", error); } - var entries = tree.entries(); - entries.forEach(function (entry, entryIndex) { + + tree.entries().forEach(function (entry, entryIndex) { if (!blobsOnly || entry.isFile()) { event.emit("entry", entry); entries.push(entry); diff --git a/test/tests/tree.js b/test/tests/tree.js new file mode 100644 index 000000000..9808b516e --- /dev/null +++ b/test/tests/tree.js @@ -0,0 +1,77 @@ +var assert = require("assert"); +var path = require("path"); +var local = path.join.bind(path, __dirname); +var promisify = require("promisify-node"); +var fse = promisify(require("fs-extra")); + +describe("Tree", function() { + var RepoUtils = require("../utils/repository_setup"); + + var repoPath = local("../repos/tree"); + + beforeEach(function() { + var test = this; + return RepoUtils.createRepository(repoPath) + .then(function(repo) { + test.repository = repo; + }); + }); + + after(function() { + return fse.remove(repoPath); + }); + + it("walks its entries and returns the same entries on both progress and end", + function() { + var repo = this.repository; + var progressEntries = []; + var endEntries; + + return RepoUtils.commitFileToRepo(repo, "test.txt", "") + .then(function(commit) { + return RepoUtils.commitFileToRepo(repo, "foo/bar.txt", "", commit); + }) + .then(function(commit) { + return commit.getTree(); + }) + .then(function(tree) { + assert(tree); + + return new Promise(function (resolve, reject) { + var walker = tree.walk(); + + walker.on("entry", function(entry) { + progressEntries.push(entry); + }); + walker.on("end", function(entries) { + endEntries = entries; + resolve(); + }); + walker.on("error", reject); + + walker.start(); + }); + }) + .then(function() { + assert(progressEntries.length); + assert(endEntries && endEntries.length); + + assert.equal( + progressEntries.length, endEntries.length, + "Different number of progress entries and end entries" + ); + + function getEntryPath(entry) { + return entry.path(); + } + + var progressFilePaths = progressEntries.map(getEntryPath).sort(); + var endFilePaths = endEntries.map(getEntryPath); + + assert.deepEqual( + progressFilePaths.sort(), endFilePaths.sort(), + "progress entries do not match end entries" + ); + }); + }); +}); diff --git a/test/utils/repository_setup.js b/test/utils/repository_setup.js index 26384bd91..d48c04650 100644 --- a/test/utils/repository_setup.js +++ b/test/utils/repository_setup.js @@ -18,18 +18,28 @@ var RepositorySetup = { }, commitFileToRepo: - function commitFileToRepo(repository, fileName, fileContent) { + function commitFileToRepo(repository, fileName, fileContent, parentCommit) { var repoWorkDir = repository.workdir(); var signature = NodeGit.Signature.create("Foo bar", "foo@bar.com", 123456789, 60); - return fse.writeFile(path.join(repoWorkDir, fileName), fileContent) + var filePath = path.join(repoWorkDir, fileName); + var parents = []; + if (parentCommit) { + parents.push(parentCommit); + } + + // fse.ensure allows us to write files inside new folders + return fse.ensureFile(filePath) + .then(function() { + return fse.writeFile(filePath, fileContent); + }) .then(function() { return RepositorySetup.addFileToIndex(repository, fileName); }) .then(function(oid) { return repository.createCommit("HEAD", signature, signature, - "initial commit", oid, []); + "initial commit", oid, parents); }) .then(function(commitOid) { return repository.getCommit(commitOid); From 123e2db21e2a7c6247c09966fd4b936692a930bd Mon Sep 17 00:00:00 2001 From: joseg Date: Mon, 29 Feb 2016 12:50:47 -0700 Subject: [PATCH 2/6] make sure both `entry` event and `end` event entries are what we expect --- test/tests/tree.js | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/test/tests/tree.js b/test/tests/tree.js index 9808b516e..164a54447 100644 --- a/test/tests/tree.js +++ b/test/tests/tree.js @@ -24,12 +24,15 @@ describe("Tree", function() { it("walks its entries and returns the same entries on both progress and end", function() { var repo = this.repository; + var file1 = "test.txt"; + var file2 = "foo/bar.txt"; + var expectedPaths = [file1, file2]; var progressEntries = []; var endEntries; - return RepoUtils.commitFileToRepo(repo, "test.txt", "") + return RepoUtils.commitFileToRepo(repo, file1, "") .then(function(commit) { - return RepoUtils.commitFileToRepo(repo, "foo/bar.txt", "", commit); + return RepoUtils.commitFileToRepo(repo, file2, "", commit); }) .then(function(commit) { return commit.getTree(); @@ -56,21 +59,21 @@ describe("Tree", function() { assert(progressEntries.length); assert(endEntries && endEntries.length); - assert.equal( - progressEntries.length, endEntries.length, - "Different number of progress entries and end entries" - ); - function getEntryPath(entry) { return entry.path(); } - var progressFilePaths = progressEntries.map(getEntryPath).sort(); + var progressFilePaths = progressEntries.map(getEntryPath); var endFilePaths = endEntries.map(getEntryPath); assert.deepEqual( - progressFilePaths.sort(), endFilePaths.sort(), - "progress entries do not match end entries" + expectedPaths, progressFilePaths, + "progress entry paths do not match expected paths" + ); + + assert.deepEqual( + expectedPaths, endFilePaths, + "end entry paths do not match expected paths" ); }); }); From 952b6b21098676fe4e8d43ba662e6ab24c24d7f0 Mon Sep 17 00:00:00 2001 From: joseg Date: Mon, 29 Feb 2016 13:09:48 -0700 Subject: [PATCH 3/6] make sure test file name is correct on all OSs --- test/tests/tree.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/tests/tree.js b/test/tests/tree.js index 164a54447..630ba9cca 100644 --- a/test/tests/tree.js +++ b/test/tests/tree.js @@ -26,7 +26,8 @@ describe("Tree", function() { var repo = this.repository; var file1 = "test.txt"; var file2 = "foo/bar.txt"; - var expectedPaths = [file1, file2]; + // index.addByPath doesn't like \s so normalize only for the expected paths + var expectedPaths = [file1, path.normalize(file2)]; var progressEntries = []; var endEntries; From 99149c8ce2eefacdd521253f39edc1735ad1c406 Mon Sep 17 00:00:00 2001 From: John Haley Date: Tue, 1 Mar 2016 09:57:43 -0700 Subject: [PATCH 4/6] Use `Set` in `Tree#walk` for storing entries --- lib/tree.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/tree.js b/lib/tree.js index a9f00fe40..845bd0400 100644 --- a/lib/tree.js +++ b/lib/tree.js @@ -127,7 +127,7 @@ Tree.prototype.walk = function(blobsOnly) { var event = new events.EventEmitter(); var total = 1; - var entries = []; + var entries = new Set(); // This looks like a DFS, but it is a BFS because of implicit queueing in // the recursive call to `entry.getTree(bfs)` @@ -139,9 +139,9 @@ Tree.prototype.walk = function(blobsOnly) { } tree.entries().forEach(function (entry, entryIndex) { - if (!blobsOnly || entry.isFile()) { + if (!blobsOnly || entry.isFile() && !entries.has(entry)) { event.emit("entry", entry); - entries.push(entry); + entries.add(entry); } if (entry.isTree()) { @@ -151,7 +151,7 @@ Tree.prototype.walk = function(blobsOnly) { }); if (total === 0) { - event.emit("end", entries); + event.emit("end", Array.from(entries)); } } From 14b2c601e390fbd587dfd4aa69fafec610fae4bf Mon Sep 17 00:00:00 2001 From: John Haley Date: Tue, 1 Mar 2016 10:05:11 -0700 Subject: [PATCH 5/6] Fix linter --- .jshintrc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.jshintrc b/.jshintrc index 17b577845..9f0a2639b 100644 --- a/.jshintrc +++ b/.jshintrc @@ -18,7 +18,8 @@ "maxlen": 80, "node": true, "predef": [ - "Promise" + "Promise", + "Set" ], "proto": true, "quotmark": "double", From 0e64add66055f7510a0b0e428abbe4fb4f8919d4 Mon Sep 17 00:00:00 2001 From: John Haley Date: Tue, 1 Mar 2016 10:36:23 -0700 Subject: [PATCH 6/6] Fix tests Neither `[v for (v of set)]` nor `Array.from` work in Node 0.12 so we have to maintain our own list of entries instead of re-iterating through them to create the final array result. --- lib/tree.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/tree.js b/lib/tree.js index 845bd0400..c85a925bc 100644 --- a/lib/tree.js +++ b/lib/tree.js @@ -128,6 +128,7 @@ Tree.prototype.walk = function(blobsOnly) { var total = 1; var entries = new Set(); + var finalEntires = []; // This looks like a DFS, but it is a BFS because of implicit queueing in // the recursive call to `entry.getTree(bfs)` @@ -142,6 +143,10 @@ Tree.prototype.walk = function(blobsOnly) { if (!blobsOnly || entry.isFile() && !entries.has(entry)) { event.emit("entry", entry); entries.add(entry); + + // Node 0.12 doesn't support either [v for (v of entries)] nor + // Array.from so we'll just maintain our own list. + finalEntires.push(entry); } if (entry.isTree()) { @@ -151,7 +156,7 @@ Tree.prototype.walk = function(blobsOnly) { }); if (total === 0) { - event.emit("end", Array.from(entries)); + event.emit("end", finalEntires); } }