From 36121e3bf87f895e7c0846e8c92af1efe2f02f14 Mon Sep 17 00:00:00 2001 From: joshaber Date: Tue, 29 Mar 2016 22:59:24 -0400 Subject: [PATCH 1/9] First pass at the git work queue. --- spec/git-work-queue-spec.js | 68 +++++++++++++++++++++++++++++++++++++ src/git-repository-async.js | 6 +++- src/git-work-queue.js | 61 +++++++++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 spec/git-work-queue-spec.js create mode 100644 src/git-work-queue.js diff --git a/spec/git-work-queue-spec.js b/spec/git-work-queue-spec.js new file mode 100644 index 000000000..18ce63f0b --- /dev/null +++ b/spec/git-work-queue-spec.js @@ -0,0 +1,68 @@ +/** @babel */ + +import GitWorkQueue from '../src/git-work-queue' + +import {it} from './async-spec-helpers' + +fdescribe('GitWorkQueue', () => { + let queue + + beforeEach(() => { + queue = new GitWorkQueue() + }) + + describe('.enqueue', () => { + it('calls the enqueued function', async () => { + let called = false + await queue.enqueue(() => { + called = true + return Promise.resolve() + }) + expect(called).toBe(true) + }) + + it('forwards values from the inner promise', async () => { + const result = await queue.enqueue(() => Promise.resolve(42)) + expect(result).toBe(42) + }) + + it('forwards errors from the inner promise', async () => { + let threw = false + try { + await queue.enqueue(() => Promise.reject(new Error('down with the sickness'))) + } catch (e) { + threw = true + } + expect(threw).toBe(true) + }) + + it('continues to dequeue work after a promise has been rejected', async () => { + try { + await queue.enqueue(() => Promise.reject(new Error('down with the sickness'))) + } catch (e) {} + + const result = await queue.enqueue(() => Promise.resolve(42)) + expect(result).toBe(42) + }) + + it('queues up work', async () => { + let resolve = null + queue.enqueue(() => { + return new Promise((resolve_, reject) => { + resolve = resolve_ + }) + }) + + expect(queue.getQueueDepth()).toBe(0) + + queue.enqueue(() => { + return new Promise((resolve, reject) => {}) + }) + + expect(queue.getQueueDepth()).toBe(1) + resolve() + + waitsFor(() => queue.getQueueDepth() === 0) + }) + }) +}) diff --git a/src/git-repository-async.js b/src/git-repository-async.js index 17d293c14..c7f05e68d 100644 --- a/src/git-repository-async.js +++ b/src/git-repository-async.js @@ -3,6 +3,7 @@ import fs from 'fs-plus' import path from 'path' import Git from 'nodegit' +import GitWorkQueue from './git-work-queue' import {Emitter, CompositeDisposable, Disposable} from 'event-kit' const modifiedStatusFlags = Git.Status.STATUS.WT_MODIFIED | Git.Status.STATUS.INDEX_MODIFIED | Git.Status.STATUS.WT_DELETED | Git.Status.STATUS.INDEX_DELETED | Git.Status.STATUS.WT_TYPECHANGE | Git.Status.STATUS.INDEX_TYPECHANGE @@ -38,8 +39,10 @@ export default class GitRepositoryAsync { } constructor (_path, options = {}) { - Git.enableThreadSafety() + // We'll serialize our access manually. + Git.disableThreadSafety() + this.workQueue = new GitWorkQueue() this.emitter = new Emitter() this.subscriptions = new CompositeDisposable() this.pathStatusCache = {} @@ -81,6 +84,7 @@ export default class GitRepositoryAsync { this.emitter.dispose() this.emitter = null } + if (this.subscriptions) { this.subscriptions.dispose() this.subscriptions = null diff --git a/src/git-work-queue.js b/src/git-work-queue.js new file mode 100644 index 000000000..1d69e654a --- /dev/null +++ b/src/git-work-queue.js @@ -0,0 +1,61 @@ +/** @babel */ + +// A queue used to manage git work. +export default class GitWorkQueue { + constructor () { + this.queue = [] + this.working = false + } + + // Enqueue the given function. The function must return a {Promise} when + // called. + enqueue (fn) { + let resolve = null + let reject = null + const wrapperPromise = new Promise((resolve_, reject_) => { + resolve = resolve_ + reject = reject_ + }) + + this.queue.push(this.wrapFunction(fn, resolve, reject)) + + this.startNext() + + return wrapperPromise + } + + wrapFunction (fn, resolve, reject) { + return () => { + const promise = fn() + promise + .then(result => { + resolve(result) + this.taskDidComplete() + }, error => { + reject(error) + this.taskDidComplete() + }) + } + } + + taskDidComplete () { + this.working = false + + this.startNext() + } + + shouldStartNext () { + return !this.working && this.queue.length > 0 + } + + startNext () { + if (!this.shouldStartNext()) return + + this.working = true + + const fn = this.queue.shift() + fn() + } + + getQueueDepth () { return this.queue.length } +} From f1516f7de4a2a1bee64804ea0319c3d06fd6df3a Mon Sep 17 00:00:00 2001 From: joshaber Date: Tue, 29 Mar 2016 23:54:39 -0400 Subject: [PATCH 2/9] First pass at using the work queue. --- spec/git-work-queue-spec.js | 4 +- src/git-repository-async.js | 128 ++++++++++++++++++++---------------- src/git-work-queue.js | 10 +-- 3 files changed, 79 insertions(+), 63 deletions(-) diff --git a/spec/git-work-queue-spec.js b/spec/git-work-queue-spec.js index 18ce63f0b..9d8e8cea0 100644 --- a/spec/git-work-queue-spec.js +++ b/spec/git-work-queue-spec.js @@ -55,9 +55,7 @@ fdescribe('GitWorkQueue', () => { expect(queue.getQueueDepth()).toBe(0) - queue.enqueue(() => { - return new Promise((resolve, reject) => {}) - }) + queue.enqueue(() => new Promise((resolve, reject) => {})) expect(queue.getQueueDepth()).toBe(1) resolve() diff --git a/src/git-repository-async.js b/src/git-repository-async.js index c7f05e68d..f7c4f1b5b 100644 --- a/src/git-repository-async.js +++ b/src/git-repository-async.js @@ -264,10 +264,12 @@ export default class GitRepositoryAsync { // Public: Returns a {Promise} which resolves to whether the given branch // exists. hasBranch (branch) { - return this.getRepo() - .then(repo => repo.getBranch(branch)) - .then(branch => branch != null) - .catch(_ => false) + return this.workQueue.enqueue(() => { + return this.getRepo() + .then(repo => repo.getBranch(branch)) + .then(branch => branch != null) + .catch(_ => false) + }) } // Public: Retrieves a shortened version of the HEAD reference value. @@ -281,9 +283,11 @@ export default class GitRepositoryAsync { // // Returns a {Promise} which resolves to a {String}. getShortHead (_path) { - return this.getRepo(_path) - .then(repo => repo.getCurrentBranch()) - .then(branch => branch.shorthand()) + return this.workQueue.enqueue(() => { + return this.getRepo(_path) + .then(repo => repo.getCurrentBranch()) + .then(branch => branch.shorthand()) + }) } // Public: Is the given path a submodule in the repository? @@ -315,16 +319,18 @@ export default class GitRepositoryAsync { // * `ahead` The {Number} of commits ahead. // * `behind` The {Number} of commits behind. getAheadBehindCount (reference, _path) { - return this.getRepo(_path) - .then(repo => Promise.all([repo, repo.getBranch(reference)])) - .then(([repo, local]) => { - const upstream = Git.Branch.upstream(local) - return Promise.all([repo, local, upstream]) - }) - .then(([repo, local, upstream]) => { - return Git.Graph.aheadBehind(repo, local.target(), upstream.target()) - }) - .catch(_ => ({ahead: 0, behind: 0})) + return this.workQueue.enqueue(() => { + return this.getRepo(_path) + .then(repo => Promise.all([repo, repo.getBranch(reference)])) + .then(([repo, local]) => { + const upstream = Git.Branch.upstream(local) + return Promise.all([repo, local, upstream]) + }) + .then(([repo, local, upstream]) => { + return Git.Graph.aheadBehind(repo, local.target(), upstream.target()) + }) + .catch(_ => ({ahead: 0, behind: 0})) + }) } // Public: Get the cached ahead/behind commit counts for the current branch's @@ -356,10 +362,12 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to the {String} git configuration value // specified by the key. getConfigValue (key, _path) { - return this.getRepo(_path) - .then(repo => repo.configSnapshot()) - .then(config => config.getStringBuf(key)) - .catch(_ => null) + return this.workQueue.enqueue(() => { + return this.getRepo(_path) + .then(repo => repo.configSnapshot()) + .then(config => config.getStringBuf(key)) + .catch(_ => null) + }) } // Public: Get the URL for the 'origin' remote. @@ -382,9 +390,11 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to a {String} branch name such as // `refs/remotes/origin/master`. getUpstreamBranch (_path) { - return this.getRepo(_path) - .then(repo => repo.getCurrentBranch()) - .then(branch => Git.Branch.upstream(branch)) + return this.workQueue.enqueue(() => { + return this.getRepo(_path) + .then(repo => repo.getCurrentBranch()) + .then(branch => Git.Branch.upstream(branch)) + }) } // Public: Gets all the local and remote references. @@ -397,23 +407,25 @@ export default class GitRepositoryAsync { // * `remotes` An {Array} of remote reference names. // * `tags` An {Array} of tag reference names. getReferences (_path) { - return this.getRepo(_path) - .then(repo => repo.getReferences(Git.Reference.TYPE.LISTALL)) - .then(refs => { - const heads = [] - const remotes = [] - const tags = [] - for (const ref of refs) { - if (ref.isTag()) { - tags.push(ref.name()) - } else if (ref.isRemote()) { - remotes.push(ref.name()) - } else if (ref.isBranch()) { - heads.push(ref.name()) + return this.workQueue.enqueue(() => { + return this.getRepo(_path) + .then(repo => repo.getReferences(Git.Reference.TYPE.LISTALL)) + .then(refs => { + const heads = [] + const remotes = [] + const tags = [] + for (const ref of refs) { + if (ref.isTag()) { + tags.push(ref.name()) + } else if (ref.isRemote()) { + remotes.push(ref.name()) + } else if (ref.isBranch()) { + heads.push(ref.name()) + } } - } - return {heads, remotes, tags} - }) + return {heads, remotes, tags} + }) + }) } // Public: Get the SHA for the given reference. @@ -425,9 +437,11 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to the current {String} SHA for the // given reference. getReferenceTarget (reference, _path) { - return this.getRepo(_path) - .then(repo => Git.Reference.nameToId(repo, reference)) - .then(oid => oid.tostrS()) + return this.workQueue.enqueue(() => { + return this.getRepo(_path) + .then(repo => Git.Reference.nameToId(repo, reference)) + .then(oid => oid.tostrS()) + }) } // Reading Status @@ -505,8 +519,8 @@ export default class GitRepositoryAsync { // status bit for the path. refreshStatusForPath (_path) { let relativePath - return Promise.all([this.getRepo(), this.getWorkingDirectory()]) - .then(([repo, wd]) => { + return this.getWorkingDirectory() + .then(wd => { relativePath = this.relativize(_path, wd) return this._getStatus([relativePath]) }) @@ -1081,18 +1095,20 @@ export default class GitRepositoryAsync { // // Returns a {Promise} which resolves to an {Array} of {NodeGit.StatusFile} // statuses for the paths. - _getStatus (paths, repo) { - return this.getRepo() - .then(repo => { - const opts = { - flags: Git.Status.OPT.INCLUDE_UNTRACKED | Git.Status.OPT.RECURSE_UNTRACKED_DIRS - } + _getStatus (paths) { + return this.workQueue.enqueue(() => { + return this.getRepo() + .then(repo => { + const opts = { + flags: Git.Status.OPT.INCLUDE_UNTRACKED | Git.Status.OPT.RECURSE_UNTRACKED_DIRS + } - if (paths) { - opts.pathspec = paths - } + if (paths) { + opts.pathspec = paths + } - return repo.getStatusExt(opts) - }) + return repo.getStatusExt(opts) + }) + }) } } diff --git a/src/git-work-queue.js b/src/git-work-queue.js index 1d69e654a..6e8c2af67 100644 --- a/src/git-work-queue.js +++ b/src/git-work-queue.js @@ -19,7 +19,9 @@ export default class GitWorkQueue { this.queue.push(this.wrapFunction(fn, resolve, reject)) - this.startNext() + if (this.shouldStartNext()) { + this.startNext() + } return wrapperPromise } @@ -41,7 +43,9 @@ export default class GitWorkQueue { taskDidComplete () { this.working = false - this.startNext() + if (this.shouldStartNext()) { + this.startNext() + } } shouldStartNext () { @@ -49,8 +53,6 @@ export default class GitWorkQueue { } startNext () { - if (!this.shouldStartNext()) return - this.working = true const fn = this.queue.shift() From c0e9fde59066d1eb9ded8f2a9e3b2745daa6b140 Mon Sep 17 00:00:00 2001 From: joshaber Date: Wed, 30 Mar 2016 11:17:58 -0400 Subject: [PATCH 3/9] Re-organize to prevent recursive work queueing. --- src/git-repository-async.js | 204 +++++++++++++++++++++--------------- 1 file changed, 117 insertions(+), 87 deletions(-) diff --git a/src/git-repository-async.js b/src/git-repository-async.js index f7c4f1b5b..29788da08 100644 --- a/src/git-repository-async.js +++ b/src/git-repository-async.js @@ -297,14 +297,18 @@ export default class GitRepositoryAsync { // Returns a {Promise} that resolves true if the given path is a submodule in // the repository. isSubmodule (_path) { - return this.getRepo() - .then(repo => repo.openIndex()) - .then(index => Promise.all([index, this.relativizeToWorkingDirectory(_path)])) - .then(([index, relativePath]) => { - const entry = index.getByPath(relativePath) - if (!entry) return false + return this.relativizeToWorkingDirectory(_path) + .then(relativePath => { + return this.workQueue.enqueue(() => { + return this.getRepo() + .then(repo => repo.openIndex()) + .then(index => { + const entry = index.getByPath(relativePath) + if (!entry) return false - return entry.mode === submoduleMode + return entry.mode === submoduleMode + }) + }) }) } @@ -478,12 +482,17 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to a {Boolean} that's true if the `path` // is ignored. isPathIgnored (_path) { - return Promise.all([this.getRepo(), this.getWorkingDirectory()]) - .then(([repo, wd]) => { - const relativePath = this.relativize(_path, wd) - return Git.Ignore.pathIsIgnored(repo, relativePath) + return this.getWorkingDirectory() + .then(wd => { + return this.workQueue.enqueue(() => { + return this.getRepo() + .then(repo => { + const relativePath = this.relativize(_path, wd) + return Git.Ignore.pathIsIgnored(repo, relativePath) + }) + .then(ignored => Boolean(ignored)) + }) }) - .then(ignored => Boolean(ignored)) } // Get the status of a directory in the repository's working directory. @@ -627,34 +636,39 @@ export default class GitRepositoryAsync { // * `added` The {Number} of added lines. // * `deleted` The {Number} of deleted lines. getDiffStats (_path) { - return this.getRepo(_path) - .then(repo => Promise.all([repo, repo.getHeadCommit()])) - .then(([repo, headCommit]) => Promise.all([repo, headCommit.getTree(), this.getWorkingDirectory(_path)])) - .then(([repo, tree, wd]) => { - const options = new Git.DiffOptions() - options.contextLines = 0 - options.flags = Git.Diff.OPTION.DISABLE_PATHSPEC_MATCH - options.pathspec = this.relativize(_path, wd) - if (process.platform === 'win32') { - // Ignore eol of line differences on windows so that files checked in - // as LF don't report every line modified when the text contains CRLF - // endings. - options.flags |= Git.Diff.OPTION.IGNORE_WHITESPACE_EOL - } - return Git.Diff.treeToWorkdir(repo, tree, options) - }) - .then(diff => this._getDiffLines(diff)) - .then(lines => { - const stats = {added: 0, deleted: 0} - for (const line of lines) { - const origin = line.origin() - if (origin === Git.Diff.LINE.ADDITION) { - stats.added++ - } else if (origin === Git.Diff.LINE.DELETION) { - stats.deleted++ - } - } - return stats + return this.getWorkingDirectory(_path) + .then(wd => { + return this.workQueue.enqueue(() => { + return this.getRepo(_path) + .then(repo => Promise.all([repo, repo.getHeadCommit()])) + .then(([repo, headCommit]) => Promise.all([repo, headCommit.getTree()])) + .then(([repo, tree]) => { + const options = new Git.DiffOptions() + options.contextLines = 0 + options.flags = Git.Diff.OPTION.DISABLE_PATHSPEC_MATCH + options.pathspec = this.relativize(_path, wd) + if (process.platform === 'win32') { + // Ignore eol of line differences on windows so that files checked in + // as LF don't report every line modified when the text contains CRLF + // endings. + options.flags |= Git.Diff.OPTION.IGNORE_WHITESPACE_EOL + } + return Git.Diff.treeToWorkdir(repo, tree, options) + }) + .then(diff => this._getDiffLines(diff)) + .then(lines => { + const stats = {added: 0, deleted: 0} + for (const line of lines) { + const origin = line.origin() + if (origin === Git.Diff.LINE.ADDITION) { + stats.added++ + } else if (origin === Git.Diff.LINE.DELETION) { + stats.deleted++ + } + } + return stats + }) + }) }) } @@ -670,24 +684,29 @@ export default class GitRepositoryAsync { // * `oldLines` The {Number} of lines in the old hunk. // * `newLines` The {Number} of lines in the new hunk getLineDiffs (_path, text) { - let relativePath = null - return Promise.all([this.getRepo(_path), this.getWorkingDirectory(_path)]) - .then(([repo, wd]) => { - relativePath = this.relativize(_path, wd) - return repo.getHeadCommit() - }) - .then(commit => commit.getEntry(relativePath)) - .then(entry => entry.getBlob()) - .then(blob => { - const options = new Git.DiffOptions() - options.contextLines = 0 - if (process.platform === 'win32') { - // Ignore eol of line differences on windows so that files checked in - // as LF don't report every line modified when the text contains CRLF - // endings. - options.flags = Git.Diff.OPTION.IGNORE_WHITESPACE_EOL - } - return this._diffBlobToBuffer(blob, text, options) + return this.getWorkingDirectory(_path) + .then(wd => { + let relativePath = null + return this.workQueue.enqueue(() => { + return this.getRepo(_path) + .then(repo => { + relativePath = this.relativize(_path, wd) + return repo.getHeadCommit() + }) + .then(commit => commit.getEntry(relativePath)) + .then(entry => entry.getBlob()) + .then(blob => { + const options = new Git.DiffOptions() + options.contextLines = 0 + if (process.platform === 'win32') { + // Ignore eol of line differences on windows so that files checked in + // as LF don't report every line modified when the text contains CRLF + // endings. + options.flags = Git.Diff.OPTION.IGNORE_WHITESPACE_EOL + } + return this._diffBlobToBuffer(blob, text, options) + }) + }) }) } @@ -709,14 +728,19 @@ export default class GitRepositoryAsync { // Returns a {Promise} that resolves or rejects depending on whether the // method was successful. checkoutHead (_path) { - return Promise.all([this.getRepo(_path), this.getWorkingDirectory(_path)]) - .then(([repo, wd]) => { - const checkoutOptions = new Git.CheckoutOptions() - checkoutOptions.paths = [this.relativize(_path, wd)] - checkoutOptions.checkoutStrategy = Git.Checkout.STRATEGY.FORCE | Git.Checkout.STRATEGY.DISABLE_PATHSPEC_MATCH - return Git.Checkout.head(repo, checkoutOptions) + return this.getWorkingDirectory(_path) + .then(wd => { + return this.workQueue.enqueue(() => { + return this.getRepo(_path) + .then(repo => { + const checkoutOptions = new Git.CheckoutOptions() + checkoutOptions.paths = [this.relativize(_path, wd)] + checkoutOptions.checkoutStrategy = Git.Checkout.STRATEGY.FORCE | Git.Checkout.STRATEGY.DISABLE_PATHSPEC_MATCH + return Git.Checkout.head(repo, checkoutOptions) + }) + .then(() => this.refreshStatusForPath(_path)) + }) }) - .then(() => this.refreshStatusForPath(_path)) } // Public: Checks out a branch in your repository. @@ -727,17 +751,19 @@ export default class GitRepositoryAsync { // // Returns a {Promise} that resolves if the method was successful. checkoutReference (reference, create) { - return this.getRepo() - .then(repo => repo.checkoutBranch(reference)) - .catch(error => { - if (create) { - return this._createBranch(reference) - .then(_ => this.checkoutReference(reference, false)) - } else { - throw error - } - }) - .then(_ => null) + return this.workQueue.enqueue(() => { + return this.getRepo() + .then(repo => repo.checkoutBranch(reference)) + }) + .catch(error => { + if (create) { + return this._createBranch(reference) + .then(_ => this.checkoutReference(reference, false)) + } else { + throw error + } + }) + .then(_ => null) } // Private @@ -763,9 +789,11 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to a {NodeGit.Ref} reference to the // created branch. _createBranch (name) { - return this.getRepo() - .then(repo => Promise.all([repo, repo.getHeadCommit()])) - .then(([repo, commit]) => repo.createBranch(name, commit)) + return this.workQueue.enqueue(() => { + return this.getRepo() + .then(repo => Promise.all([repo, repo.getHeadCommit()])) + .then(([repo, commit]) => repo.createBranch(name, commit)) + }) } // Get all the hunks in the diff. @@ -822,14 +850,16 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to a {boolean} indicating whether the // branch name changed. _refreshBranch () { - return this.getRepo() - .then(repo => repo.getCurrentBranch()) - .then(ref => ref.name()) - .then(branchName => { - const changed = branchName !== this.branch - this.branch = branchName - return changed - }) + return this.workQueue.enqueue(() => { + return this.getRepo() + .then(repo => repo.getCurrentBranch()) + .then(ref => ref.name()) + .then(branchName => { + const changed = branchName !== this.branch + this.branch = branchName + return changed + }) + }) } // Refresh the cached ahead/behind count with the given branch. From 6ba2f6d4b8da38b60399fd4a7d14df109dd3e41a Mon Sep 17 00:00:00 2001 From: joshaber Date: Wed, 30 Mar 2016 11:23:54 -0400 Subject: [PATCH 4/9] Pull refresh outside the work function. Otherwise we deadlock lolololol --- src/git-repository-async.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/git-repository-async.js b/src/git-repository-async.js index 29788da08..86db4a574 100644 --- a/src/git-repository-async.js +++ b/src/git-repository-async.js @@ -738,9 +738,9 @@ export default class GitRepositoryAsync { checkoutOptions.checkoutStrategy = Git.Checkout.STRATEGY.FORCE | Git.Checkout.STRATEGY.DISABLE_PATHSPEC_MATCH return Git.Checkout.head(repo, checkoutOptions) }) - .then(() => this.refreshStatusForPath(_path)) }) }) + .then(() => this.refreshStatusForPath(_path)) } // Public: Checks out a branch in your repository. From f028c779b16aa9a5986502f35d7e1941ca0abd88 Mon Sep 17 00:00:00 2001 From: joshaber Date: Wed, 30 Mar 2016 11:44:24 -0400 Subject: [PATCH 5/9] Treat it more like a pool. --- spec/git-work-queue-spec.js | 2 +- src/git-work-queue.js | 32 +++++++++++++------------------- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/spec/git-work-queue-spec.js b/spec/git-work-queue-spec.js index 9d8e8cea0..cd38ddd72 100644 --- a/spec/git-work-queue-spec.js +++ b/spec/git-work-queue-spec.js @@ -8,7 +8,7 @@ fdescribe('GitWorkQueue', () => { let queue beforeEach(() => { - queue = new GitWorkQueue() + queue = new GitWorkQueue([{}]) }) describe('.enqueue', () => { diff --git a/src/git-work-queue.js b/src/git-work-queue.js index 6e8c2af67..d73c7153a 100644 --- a/src/git-work-queue.js +++ b/src/git-work-queue.js @@ -2,9 +2,10 @@ // A queue used to manage git work. export default class GitWorkQueue { - constructor () { + constructor (pool) { + this.pool = pool + this.queue = [] - this.working = false } // Enqueue the given function. The function must return a {Promise} when @@ -19,41 +20,34 @@ export default class GitWorkQueue { this.queue.push(this.wrapFunction(fn, resolve, reject)) - if (this.shouldStartNext()) { - this.startNext() - } + this.startNextIfAble() return wrapperPromise } wrapFunction (fn, resolve, reject) { return () => { - const promise = fn() + const repo = this.pool.shift() + const promise = fn(repo) promise .then(result => { resolve(result) - this.taskDidComplete() + this.taskDidComplete(repo) }, error => { reject(error) - this.taskDidComplete() + this.taskDidComplete(repo) }) } } - taskDidComplete () { - this.working = false + taskDidComplete (repo) { + this.pool.push(repo) - if (this.shouldStartNext()) { - this.startNext() - } + this.startNextIfAble() } - shouldStartNext () { - return !this.working && this.queue.length > 0 - } - - startNext () { - this.working = true + startNextIfAble () { + if (!this.pool.length || !this.queue.length) return const fn = this.queue.shift() fn() From e701fcc2926d261e102279c3cdf0589d039ece39 Mon Sep 17 00:00:00 2001 From: joshaber Date: Wed, 30 Mar 2016 11:47:05 -0400 Subject: [PATCH 6/9] Rename work queue to resource pool. --- ...rk-queue-spec.js => resource-pool-spec.js} | 6 +-- src/git-repository-async.js | 37 ++++++++++--------- src/{git-work-queue.js => resource-pool.js} | 4 +- 3 files changed, 24 insertions(+), 23 deletions(-) rename spec/{git-work-queue-spec.js => resource-pool-spec.js} (92%) rename src/{git-work-queue.js => resource-pool.js} (93%) diff --git a/spec/git-work-queue-spec.js b/spec/resource-pool-spec.js similarity index 92% rename from spec/git-work-queue-spec.js rename to spec/resource-pool-spec.js index cd38ddd72..3a5d79bcd 100644 --- a/spec/git-work-queue-spec.js +++ b/spec/resource-pool-spec.js @@ -1,14 +1,14 @@ /** @babel */ -import GitWorkQueue from '../src/git-work-queue' +import ResourcePool from '../src/resource-pool' import {it} from './async-spec-helpers' -fdescribe('GitWorkQueue', () => { +fdescribe('ResourcePool', () => { let queue beforeEach(() => { - queue = new GitWorkQueue([{}]) + queue = new ResourcePool([{}]) }) describe('.enqueue', () => { diff --git a/src/git-repository-async.js b/src/git-repository-async.js index 86db4a574..f700ab049 100644 --- a/src/git-repository-async.js +++ b/src/git-repository-async.js @@ -3,7 +3,7 @@ import fs from 'fs-plus' import path from 'path' import Git from 'nodegit' -import GitWorkQueue from './git-work-queue' +import ResourcePool from './resource-pool' import {Emitter, CompositeDisposable, Disposable} from 'event-kit' const modifiedStatusFlags = Git.Status.STATUS.WT_MODIFIED | Git.Status.STATUS.INDEX_MODIFIED | Git.Status.STATUS.WT_DELETED | Git.Status.STATUS.INDEX_DELETED | Git.Status.STATUS.WT_TYPECHANGE | Git.Status.STATUS.INDEX_TYPECHANGE @@ -42,7 +42,6 @@ export default class GitRepositoryAsync { // We'll serialize our access manually. Git.disableThreadSafety() - this.workQueue = new GitWorkQueue() this.emitter = new Emitter() this.subscriptions = new CompositeDisposable() this.pathStatusCache = {} @@ -53,6 +52,8 @@ export default class GitRepositoryAsync { this._openExactPath = options.openExactPath || false this.repoPromise = this.openRepository() + this.repoPool = new ResourcePool([this.repoPromise]) + this.isCaseInsensitive = fs.isCaseInsensitive() this.upstream = {} this.submodules = {} @@ -264,7 +265,7 @@ export default class GitRepositoryAsync { // Public: Returns a {Promise} which resolves to whether the given branch // exists. hasBranch (branch) { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo() .then(repo => repo.getBranch(branch)) .then(branch => branch != null) @@ -283,7 +284,7 @@ export default class GitRepositoryAsync { // // Returns a {Promise} which resolves to a {String}. getShortHead (_path) { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo(_path) .then(repo => repo.getCurrentBranch()) .then(branch => branch.shorthand()) @@ -299,7 +300,7 @@ export default class GitRepositoryAsync { isSubmodule (_path) { return this.relativizeToWorkingDirectory(_path) .then(relativePath => { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo() .then(repo => repo.openIndex()) .then(index => { @@ -323,7 +324,7 @@ export default class GitRepositoryAsync { // * `ahead` The {Number} of commits ahead. // * `behind` The {Number} of commits behind. getAheadBehindCount (reference, _path) { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo(_path) .then(repo => Promise.all([repo, repo.getBranch(reference)])) .then(([repo, local]) => { @@ -366,7 +367,7 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to the {String} git configuration value // specified by the key. getConfigValue (key, _path) { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo(_path) .then(repo => repo.configSnapshot()) .then(config => config.getStringBuf(key)) @@ -394,7 +395,7 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to a {String} branch name such as // `refs/remotes/origin/master`. getUpstreamBranch (_path) { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo(_path) .then(repo => repo.getCurrentBranch()) .then(branch => Git.Branch.upstream(branch)) @@ -411,7 +412,7 @@ export default class GitRepositoryAsync { // * `remotes` An {Array} of remote reference names. // * `tags` An {Array} of tag reference names. getReferences (_path) { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo(_path) .then(repo => repo.getReferences(Git.Reference.TYPE.LISTALL)) .then(refs => { @@ -441,7 +442,7 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to the current {String} SHA for the // given reference. getReferenceTarget (reference, _path) { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo(_path) .then(repo => Git.Reference.nameToId(repo, reference)) .then(oid => oid.tostrS()) @@ -484,7 +485,7 @@ export default class GitRepositoryAsync { isPathIgnored (_path) { return this.getWorkingDirectory() .then(wd => { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo() .then(repo => { const relativePath = this.relativize(_path, wd) @@ -638,7 +639,7 @@ export default class GitRepositoryAsync { getDiffStats (_path) { return this.getWorkingDirectory(_path) .then(wd => { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo(_path) .then(repo => Promise.all([repo, repo.getHeadCommit()])) .then(([repo, headCommit]) => Promise.all([repo, headCommit.getTree()])) @@ -687,7 +688,7 @@ export default class GitRepositoryAsync { return this.getWorkingDirectory(_path) .then(wd => { let relativePath = null - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo(_path) .then(repo => { relativePath = this.relativize(_path, wd) @@ -730,7 +731,7 @@ export default class GitRepositoryAsync { checkoutHead (_path) { return this.getWorkingDirectory(_path) .then(wd => { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo(_path) .then(repo => { const checkoutOptions = new Git.CheckoutOptions() @@ -751,7 +752,7 @@ export default class GitRepositoryAsync { // // Returns a {Promise} that resolves if the method was successful. checkoutReference (reference, create) { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo() .then(repo => repo.checkoutBranch(reference)) }) @@ -789,7 +790,7 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to a {NodeGit.Ref} reference to the // created branch. _createBranch (name) { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo() .then(repo => Promise.all([repo, repo.getHeadCommit()])) .then(([repo, commit]) => repo.createBranch(name, commit)) @@ -850,7 +851,7 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to a {boolean} indicating whether the // branch name changed. _refreshBranch () { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo() .then(repo => repo.getCurrentBranch()) .then(ref => ref.name()) @@ -1126,7 +1127,7 @@ export default class GitRepositoryAsync { // Returns a {Promise} which resolves to an {Array} of {NodeGit.StatusFile} // statuses for the paths. _getStatus (paths) { - return this.workQueue.enqueue(() => { + return this.repoPool.enqueue(() => { return this.getRepo() .then(repo => { const opts = { diff --git a/src/git-work-queue.js b/src/resource-pool.js similarity index 93% rename from src/git-work-queue.js rename to src/resource-pool.js index d73c7153a..2a40b4304 100644 --- a/src/git-work-queue.js +++ b/src/resource-pool.js @@ -1,7 +1,7 @@ /** @babel */ -// A queue used to manage git work. -export default class GitWorkQueue { +// Manages a pool of some resource. +export default class ResourcePool { constructor (pool) { this.pool = pool From dea119ef3ed1934a4c8055237d74d38fc99aa583 Mon Sep 17 00:00:00 2001 From: joshaber Date: Wed, 30 Mar 2016 11:50:05 -0400 Subject: [PATCH 7/9] Less repo-centric naming. --- src/resource-pool.js | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/resource-pool.js b/src/resource-pool.js index 2a40b4304..ae7cb71d0 100644 --- a/src/resource-pool.js +++ b/src/resource-pool.js @@ -8,8 +8,8 @@ export default class ResourcePool { this.queue = [] } - // Enqueue the given function. The function must return a {Promise} when - // called. + // Enqueue the given function. The function will be given an object from the + // pool. The function must return a {Promise}. enqueue (fn) { let resolve = null let reject = null @@ -20,37 +20,37 @@ export default class ResourcePool { this.queue.push(this.wrapFunction(fn, resolve, reject)) - this.startNextIfAble() + this.dequeueIfAble() return wrapperPromise } wrapFunction (fn, resolve, reject) { - return () => { - const repo = this.pool.shift() - const promise = fn(repo) + return (resource) => { + const promise = fn(resource) promise .then(result => { resolve(result) - this.taskDidComplete(repo) + this.taskDidComplete(resource) }, error => { reject(error) - this.taskDidComplete(repo) + this.taskDidComplete(resource) }) } } - taskDidComplete (repo) { - this.pool.push(repo) + taskDidComplete (resource) { + this.pool.push(resource) - this.startNextIfAble() + this.dequeueIfAble() } - startNextIfAble () { + dequeueIfAble () { if (!this.pool.length || !this.queue.length) return const fn = this.queue.shift() - fn() + const resource = this.pool.shift() + fn(resource) } getQueueDepth () { return this.queue.length } From f59a86b2b9360a5219685335f49712e7767caaf3 Mon Sep 17 00:00:00 2001 From: joshaber Date: Wed, 30 Mar 2016 11:51:49 -0400 Subject: [PATCH 8/9] Note that we're not using this yet. --- src/git-repository-async.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/git-repository-async.js b/src/git-repository-async.js index f700ab049..e45c1e47e 100644 --- a/src/git-repository-async.js +++ b/src/git-repository-async.js @@ -52,6 +52,9 @@ export default class GitRepositoryAsync { this._openExactPath = options.openExactPath || false this.repoPromise = this.openRepository() + // NB: We don't currently _use_ the pooled object. But by giving it one + // thing, we're really just serializing all the work. Down the road, we + // could open multiple connections to the repository. this.repoPool = new ResourcePool([this.repoPromise]) this.isCaseInsensitive = fs.isCaseInsensitive() From f19d3a2bce88d03ebf3fe7604b57a25a815f8d8e Mon Sep 17 00:00:00 2001 From: joshaber Date: Wed, 30 Mar 2016 11:59:35 -0400 Subject: [PATCH 9/9] Unfocus. --- spec/resource-pool-spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/resource-pool-spec.js b/spec/resource-pool-spec.js index 3a5d79bcd..27893360a 100644 --- a/spec/resource-pool-spec.js +++ b/spec/resource-pool-spec.js @@ -4,7 +4,7 @@ import ResourcePool from '../src/resource-pool' import {it} from './async-spec-helpers' -fdescribe('ResourcePool', () => { +describe('ResourcePool', () => { let queue beforeEach(() => {