Async Git Repository class #9213

thedaniel · 2015-10-19T15:41:08Z

This is an experimental spike of an alternate GitRepository class that uses nodegit rather than git-utils. We would love to speed up the responsiveness of various aspects of Atom (tree-view, etc) by doing more Git operations asynchronously, and it would also be nice to get out of the business of maintaining our own libgit2 wrapper.

Here are a few notes about the reasoning behind some decisions here:

It follows the existing GitRepository API closely, changing everything sync to Promise-based async.
- The assumption is that having a close mapping between the sync and aysnc APIs will make it less painful to refactor client packages.
- I'm not 100% sure we should keep this design, so it's a tradeoff between design quality and incurred labor. Ideally we'll keep this private and experimental all the way until Atom 2.0 so that we can change it as we need to.
The class is written in ES6, because Coffeescript, while lovely, is the past, and the community seems to have consensus on transpilers like babel.
- This includes an .eslintrc, because I've found it extremely useful.
- The spec is still in coffee because I thought it might be easier to just modify it as I go to make sure I had the same coverage on both classes, though at the moment I wish I'd run it through a coffee -> js converter first.
I haven't done a lot of ES6 or constructed a Promise-based API for anything serious before, so I'm sure to misuse some APIs.
We'll need to keep the sync GitRepository class around for backwards compat, so I haven't decided quite how to integrate this with the current provider system.
- Perhaps we add a private method on the sync repo like GitRepository.async that returns the Promise for an async repo, so that internal callers can use async without changing anything in the GitRepositoryProvider?
Documentation is sparse because I wasn't sure this was a good path to follow until today. I'll flesh it out more as I go.

cc @atom/core @orderedlist

Determine a good test case (even if manual / subjective) for git-related UI responsiveness
- many simultaneous changes, check tree-view update?
- simulate degraded disk performance / network drive?
cf GitRepository getDirectoryStatus mishandles project root directory #9240

nathansobo · 2015-10-19T17:59:32Z

What's the plan for transitioning to this API? I was thinking our existing synchronous GitRepository could have an async property that points to this new version. We can then encourage users of today's APIs to use the async version like this:

let repository = someMethodReturningARepository().async

When we hit Atom 2.0 and transition to the repository always being async, we can retain the async property and just point it to self, such that repository.async === repository returns true.

However, I'm not sure how this all meshes with your idea of keeping this API private for a while. In my mind, we should start transitioning the community to any async APIs we feel confident about. We might not want to make every method on the class public, but I'd like to deprecate existing synchronous functionality as soon as we can.

thedaniel · 2015-10-19T18:02:04Z

I was thinking our existing synchronous GitRepository could have an async property that points to this new version.

Yep, I agree with this, it's suggested in the bullet points above.

However, I'm not sure how this all meshes with your idea of keeping this API private for a while. In my mind, we should start transitioning the community to any async APIs we feel confident about.

This is not necessarily a conflict with how I feel, I'm just not expecting we'll feel confident about the APIs for a while.

nathansobo · 2015-10-19T18:02:42Z

spec/git-repository-async-spec.coffee

+
+      onSuccess = jasmine.createSpy('onSuccess')
+
+      waitsForPromise ->


If you also write the specs in ES6, async/await syntax could really clean up these async tests...

it('does something async', function () { waitsForPromise(async function() { expect(await repo.getPath()).toBe('/some/path') }) })

I think that's ES7+ only

Yes, which we have access to 😄

nathansobo · 2015-10-19T18:10:54Z

⚡ Thanks for moving us in the right direction on this.

nathansobo · 2015-10-19T18:13:00Z

it's suggested in the bullet points above.

Sorry, I didn't comprehend it initially for some reason. Of course async will have to be a promise, and when we transition all our repository-returning APIs to be asynchronous, we'll have to have an async property on the returned promise that references the original promise.

smashwilson · 2015-10-19T18:17:03Z

Oh hey, this will be excellent for packages that integrate with git. 👍 😁

alexandernst · 2015-10-19T21:21:58Z

❤️ ES6 instead of ☕script!

thedaniel · 2015-10-20T13:48:58Z

Thought: it seems like a good place to start using this in prod & packages is to proxy all the did-change-status events from the sync implementation to the async implementation. At this moment I'm just writing tests that assert the events fire correctly in async-land at all and calling through from sync to async in a few methods (e.g. getPathStatus(path)) just so the side-effects happen, but ultimately we could remove anything solely concerned with generating events out of the sync implementation entirely and just re-emit the ones generated by the async implementation.

philipgiuliani · 2015-10-21T20:05:24Z

src/git-repository-async.js

+    return new GitRepositoryAsync(path, options)
+  }
+
+  constructor (path, options) {


💄 🎨 What do you think about removing the spaces in front of the arguments? https://github.com/airbnb/javascript#18.3 Just about style. Really good work! Love the ES6 ❤️

Good catch - this currently conforms to JS Standard Style (a very presumptuous name, isn't it?) rather than AirBnB style and I'm happy with Standard.

Thanks for the resource. Didn't knew about it! Looks of course also good 👍 Just important that there is one style over the whole repository 👍 👏

thedaniel · 2015-10-23T10:31:34Z

Thanks for the feedback @steelbrain!

steelbrain · 2015-10-23T11:21:51Z

src/git-repository-async.js

+
+  getPath () {
+    return this.repoPromise.then((repo) => {
+      return Promise.resolve(repo.path().replace(/\/$/, ''))


You might want to replace most of these Promise.resolves here with just values.

This will break a lot of things, and depends on atom/atom#9213

ghost · 2015-10-23T16:04:04Z

You guys really hate placing semicolons don't you? :D

SeanJM · 2015-10-23T16:43:40Z

By not adding semi colons, are you adding overhead to the JIT?

On Oct 23, 2015, at 12:04 PM, Alex Cruz notifications@github.com wrote:

You guys really hate placing semicolons don't you? :D

—
Reply to this email directly or view it on GitHub #9213 (comment).

ghost · 2015-10-23T17:11:22Z

@SeanJM I don't know why github doesn't use them in their code, is this because they have most of their code in Ruby or Coffescript doesn't allow them?

SeanJM · 2015-10-23T17:27:55Z

https://www.quora.com/Are-there-performance-differences-between-using-semi-colons-and-not-using-semi-colons-in-JavaScript https://www.quora.com/Are-there-performance-differences-between-using-semi-colons-and-not-using-semi-colons-in-JavaScript
According to this the answer is no.

On Oct 23, 2015, at 1:11 PM, Alex Cruz notifications@github.com wrote:

@SeanJM https://github.com/SeanJM I don't know why github doesn't use them in their code, is this because they have most of their code in Ruby?

—
Reply to this email directly or view it on GitHub #9213 (comment).

thomasjo · 2015-10-26T13:02:40Z

src/git-repository-async.js

+    this._gitUtilsRepo = GitUtils.open(path) // TODO remove after porting ::relativize
+    this.repoPromise = Git.Repository.open(path)
+
+    var {project, refreshOnWindowFocus} = options


This should be let (or const), doesn't look like we want or need hoisting here.

…sync-repo-relativize-symlinks

Support symlinked repo roots when relativizing

joshaber · 2016-01-15T21:17:07Z

Async Git Repository class

kevinsawicki · 2016-01-15T21:26:00Z

Awesome work here 👍

johnhaley81 · 2016-01-15T21:27:44Z

w00t!

martindale · 2016-03-17T22:40:56Z

Does this address #2456?

winstliu · 2016-03-17T22:58:57Z

@martindale it would be great if you could download the 1.7.0 beta and find out :).

martindale · 2016-03-17T23:45:26Z

Subjectively, the problem appears to be worse with 1.7.0 beta. Atom now completely hangs when launching it inside of a remote mount, and eventually requests direction on whether to close or keep waiting.

yannickcr · 2016-03-18T10:30:34Z

Same problem here, Atom 1.6.0 or 1.7.0 beta now hangs up when trying to open a big git repository inside of a remote mount making Atom unusable in this configuration 😢

bynite · 2016-03-24T10:40:16Z

Can't use Atom anymore. Probably because of this feature. I mounted a vagrant via SSHFS and Atom freezes when opening a file. Could you release a fix for this, please.

mehcode · 2016-03-24T16:51:01Z

I'm wondering why we switched to using nodegit. I realize that maintaining a wrapper around a git executable is lame but nodegit wraps libgit2 which is glacial in comparison to git. This is completely anecdotal but I've always found it far slower.

Perhaps using something like https://www.npmjs.com/package/simple-git instead?

thedaniel · 2016-03-24T17:13:59Z

@mehcode Before nodegit we were already using libgit2, with a sync api wrapper we were maintaining ourselves (https://github.com/atom/git-utils) - so we can't blame any performance regressions on libgit2 here.

mehcode · 2016-03-24T17:15:40Z

Ah. I was under the impression we were just doing shell execution.

nathansobo reviewed Oct 19, 2015
View reviewed changes

winstliu added the work-in-progress label Oct 19, 2015

philipgiuliani reviewed Oct 21, 2015
View reviewed changes

steelbrain reviewed Oct 23, 2015
View reviewed changes

thedaniel added a commit to atom/tree-view that referenced this pull request Oct 23, 2015

Return async repo instead of sync repo

dfb43df

This will break a lot of things, and depends on atom/atom#9213

thomasjo reviewed Oct 26, 2015
View reviewed changes

joshaber and others added 10 commits January 14, 2016 11:39

Merge branch 'master' into dh-async-repo

f3a6f2e

Merge branch 'master' into dh-async-repo

d6cd292

Merge branch 'master' into dh-async-repo

74d94ae

Merge remote-tracking branch 'remotes/origin/dh-async-repo' into dh-a…

411a80d

…sync-repo-relativize-symlinks

Add option for custom label to waitsForPromise

4fff622

Don't let label be null

281d6a6

Try using .getStatusExt instead of .getStatus.

f30f2c0

🔥 some lint.

fb5b3f0

Merge pull request #10352 from atom/dh-async-repo-relativize-symlinks

7d9ed01

Support symlinked repo roots when relativizing

Merge branch 'master' into dh-async-repo

7cfb8a3

joshaber added a commit that referenced this pull request Jan 15, 2016

Merge pull request #9213 from atom/dh-async-repo

4d740d3

Async Git Repository class

joshaber merged commit 4d740d3 into master Jan 15, 2016

joshaber deleted the dh-async-repo branch January 15, 2016 21:17

smashwilson mentioned this pull request Feb 23, 2016

Use nodegit if available smashwilson/merge-conflicts#205

Merged

subesokun mentioned this pull request Mar 19, 2016

Use new async Git repository class introduced in Atom 1.6 subesokun/atom-tree-view-git-status#22

Closed

dborsatto mentioned this pull request Mar 24, 2016

Atom 1.6 Frequently Freezes with Remote Project Folders (All Platforms) #11214

Closed

This was referenced Apr 19, 2016

Crashes in Atom v1.7.0-beta: Cannot read property 'onDidChangeStatus' of undefined andischerer/atom-svn#20

Open

Failed to activate the status-bar package after Atom 1.7.0 update atom/status-bar#143

Closed

maxbrunsfeld mentioned this pull request Jul 13, 2016

Remove uses of nodegit #12159

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async Git Repository class #9213

Async Git Repository class #9213

thedaniel commented Oct 19, 2015

nathansobo commented Oct 19, 2015

thedaniel commented Oct 19, 2015

nathansobo Oct 19, 2015

thedaniel Oct 30, 2015

thomasjo Oct 30, 2015

nathansobo commented Oct 19, 2015

nathansobo commented Oct 19, 2015

smashwilson commented Oct 19, 2015

alexandernst commented Oct 19, 2015

thedaniel commented Oct 20, 2015

philipgiuliani Oct 21, 2015

thedaniel Oct 21, 2015

philipgiuliani Oct 21, 2015

thedaniel commented Oct 23, 2015

steelbrain Oct 23, 2015

ghost commented Oct 23, 2015

SeanJM commented Oct 23, 2015

ghost commented Oct 23, 2015

SeanJM commented Oct 23, 2015

thomasjo Oct 26, 2015

joshaber commented Jan 15, 2016

kevinsawicki commented Jan 15, 2016

johnhaley81 commented Jan 15, 2016

martindale commented Mar 17, 2016

winstliu commented Mar 17, 2016

martindale commented Mar 17, 2016

yannickcr commented Mar 18, 2016

bynite commented Mar 24, 2016

mehcode commented Mar 24, 2016

thedaniel commented Mar 24, 2016

mehcode commented Mar 24, 2016


		onSuccess = jasmine.createSpy('onSuccess')

		waitsForPromise ->

Async Git Repository class #9213

Async Git Repository class #9213

Conversation

thedaniel commented Oct 19, 2015

nathansobo commented Oct 19, 2015

thedaniel commented Oct 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathansobo commented Oct 19, 2015

nathansobo commented Oct 19, 2015

smashwilson commented Oct 19, 2015

alexandernst commented Oct 19, 2015

thedaniel commented Oct 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedaniel commented Oct 23, 2015

Choose a reason for hiding this comment

ghost commented Oct 23, 2015

SeanJM commented Oct 23, 2015

ghost commented Oct 23, 2015

SeanJM commented Oct 23, 2015

Choose a reason for hiding this comment

joshaber commented Jan 15, 2016

kevinsawicki commented Jan 15, 2016

johnhaley81 commented Jan 15, 2016

martindale commented Mar 17, 2016

winstliu commented Mar 17, 2016

martindale commented Mar 17, 2016

yannickcr commented Mar 18, 2016

bynite commented Mar 24, 2016

mehcode commented Mar 24, 2016

thedaniel commented Mar 24, 2016

mehcode commented Mar 24, 2016