Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add async implementation of GitRepositoryProvider.repositoryForDirectory #16558

Merged
merged 3 commits into from Feb 21, 2019

Conversation

Projects
None yet
6 participants
@lgeiger
Copy link
Contributor

lgeiger commented Jan 14, 2018

Description of the Change

This PR converts scr/git-repository-provider.coffee to JS and converts the synchronous filesystem calls inside GitRepositoryProvider.repositoryForDirectory to async.
As far as I know this doesn't make it fully async since Repository.open still does some synchronous calls, but it's a starting point for future optimisations.

Alternate Designs

I'd prefer to fully remove GitRepositoryProvider.repositoryForDirectorySync to reduce the amount of code but this would be a breaking change for any packages that might be using this method or atom.project.addPath().

atom.project.addPath() is the only place where repositoryForDirectorySync is currently used inside Atom:

repo = provider.repositoryForDirectorySync(directory)

I think adding a optional async implementation of atom.project.addPath() in a future PR would be the easiest thing to do.

A quick look through the community packages reveals that it's not really used there either

Cick here to see the full output of searching through all community packages

> grep -r "repositoryForDirectorySync" packages/
packages//svn/lib/svn-repository-provider.coffee:    Promise.resolve(@repositoryForDirectorySync(directory))
packages//svn/lib/svn-repository-provider.coffee:  repositoryForDirectorySync: (directory) ->
packages//atom-hg/lib/hg-repository-provider.coffee:      Promise.resolve(@repositoryForDirectorySync(directory))
packages//atom-hg/lib/hg-repository-provider.coffee:    repositoryForDirectorySync: (directory) ->
grep: packages//hipsum/node_modules/.bin/hipster-ipsum: No such file or directory
grep: packages//dynapp-atom/dynapp-atom: No such file or directory
grep: packages//docker/docker: Too many levels of symbolic links
grep: packages//parcel/parcel: Too many levels of symbolic links
packages//atom-tsd/typings/atom/atom.d.ts:        repositoryForDirectorySync(directory? : Pathwatcher.Directory) : any;
grep: packages//run-file/node_modules/.bin/csonc: No such file or directory
grep: packages//quilt-completions/quilt-completions: No such file or directory
packages//atom-yeoman/typingsTemp/atom/atom.d.ts:        repositoryForDirectorySync(directory? : Pathwatcher.Directory) : any;
grep: packages//epitools/.#amin.c: No such file or directory
packages//omnisharp-atom/typingsTemp/atom/atom.d.ts:        repositoryForDirectorySync(directory? : GitRepository) : GitRepository;
grep: packages//web-view/web-view: Too many levels of symbolic links
grep: packages//orionsoft/atom: No such file or directory
packages//nuclide/pkg/nuclide-hg-repository/lib/HgRepositoryProvider.js:    return Promise.resolve(this.repositoryForDirectorySync(directory));
packages//nuclide/pkg/nuclide-hg-repository/lib/HgRepositoryProvider.js:  repositoryForDirectorySync(directory: Directory): ?HgRepositoryClient {
packages//nuclide/pkg/nuclide-hg-repository/lib/HgRepositoryProvider.js:    return trackTiming('hg-repository.repositoryForDirectorySync', () => {
packages//nuclide/pkg/nuclide-remote-connection/lib/RemoteConnection.js:  // ::repositoryForDirectorySync, so we need the repo information to already be
grep: packages//tree-view-click/.eslintrc: No such file or directory
packages//json-schema/typingsTemp/atom/atom.d.ts:        repositoryForDirectorySync(directory? : GitRepository) : GitRepository;

Why Should This Be In Core?

It's already in Core

Benefits

This removes synchronous IO from Atom Core which should help when using Atom together with network attached file systems.

Possible Drawbacks

This adds similar/duplicate code to git-repository-provider.js.

Verification Process

Unit tests.

Applicable Issues

#1766, #12159 (comment)

lgeiger added some commits Jan 13, 2018

Add tests for repositoryForDirectorySync
Since `repositoryForDirectory` and `repositoryForDirectorySync` don't
share the same implementation anymore.
@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Jan 16, 2018

  • svn uses its own repositoryForDirectorySync implementation
  • atom-hg uses its own repositoryForDirectorySync implementation
  • nuclide-hg-repository uses its own repositoryForDirectorySync implementation

All of your other results appear to be errors from grep or are from unofficial TypeScript bindings. So it looks like Atom core is the only place that uses repositoryForDirectorySync unless I missed something.

@lgeiger

This comment has been minimized.

Copy link
Contributor Author

lgeiger commented Jan 16, 2018

So it looks like Atom core is the only place that uses repositoryForDirectorySync unless I missed something.

Yes, it's only used by atom.project.addPath() but I fear making it use the async version may be a breaking change for some use cases.

@t9md

This comment has been minimized.

Copy link
Contributor

t9md commented Jan 18, 2018

One example, making it async affects my project-folder pkg.
(I'm not particularly opposing change, just sharing sample case which affects)

https://github.com/t9md/atom-project-folder/blob/af22038737ca6f427a429f9cd10e6042152d9bf2/spec/project-folder-spec.js#L22

Maybe popular project-manager be affected too?

@lee-dohm lee-dohm merged commit d07d854 into atom:master Feb 21, 2019

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lgeiger lgeiger deleted the lgeiger:async-git branch Feb 21, 2019

@lgeiger

This comment has been minimized.

Copy link
Contributor Author

lgeiger commented Feb 21, 2019

Thanks for merging, I totally forgot about this PR 😄

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Feb 21, 2019

You're welcome! And thank you very much for the quality submission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.