Closed eeejay closed 5 years ago
I guess this could be a fix for issue #271
This needs tests before it can be considered.
Following symlink subdirs is broken in this solution.
Do any of the alternatives handle symlinks correctly?
What does this mean??
You figured it out in your next comment :wink:
even though it is pretty certain Atom will have access to git in its PATH.
At least on Windows, it is not required to have Git installed when using Atom.
I can't figure out what kind of test would be useful here. Ultimately, this change is great because all the regression tests pass, which proves that this change is functionally identical where it matters.
Do you have any ideas as to what kind of test would be good here?
As for the symlink issue, we can potentially use find
for that. It wouldn't work on Windows, which is probably fine because symlinks don't work on Windows anyway, afaik. The strategy would look like this:
find -type l -xtype d
to get all the directory symbolic links.git ls-files
(-x
)find
to traverse the symbolic linked directories and append output to results from previous step. This step would only be needed if there are actual folders.I don't think that is ideal, but I also see a conflict with core.followSymlinks and core.excludeVcsIgnoredPaths. Symlinks are never followed in git, it sees them as a leaf node, so their contents are implicitly ignored. So, if you want to exclude VCS ignored paths, you would never follow a version controlled symlink. The only time you would follow a symlink in a VCS folder is if it were untracked. With that in mind, I would suggest the equivalent of these steps:
git ls-files -c --exclude-standard
git ls-files -o --exclude-standard
pipe output to find -L
Do you have any ideas as to what kind of test would be good here?
Disclaimer: I don't know what the code coverage for fuzzy-finder looks like. It could be that the test I'm suggesting already exists.
I'd like to see a test asserting that we don't attempt to use the Git method when loading paths for a non-Git project.
I'd like to see a test asserting that we don't attempt to use the Git method when loading paths for a non-Git project.
This is already tested for!
Updated! (don't know if you get notified when another commit is pushed)
:+1: Sorry for the late reply; was on vacation. I've brought this PR up to the other team members for consideration.
sorry for the late arrival to this PR. Here are the following test scenario I plan to run through:
I'll be spending time this week to look into these, please let me know if you feel like anything is missing from this list. I'll be testing first before the changes and reporting back if anything changed or is the same.
One thing I see is:
Following symlink subdirs is broken in this solution.
@eeejay do you know if this has been fixed with a more recent commit?
Thanks for the work you've put into this PR.
Unfortunately, we won't be able to accept this as it is currently written because it causes a change in behavior with regard to symlinks. We are also looking at making indexing better by incorporating some of the work we've invested in our file watcher infrastructure. If you can update this PR to address the symlinks issue, we'll reevaluate it after the indexing changes.
Thanks again for your help!
Pull request #367 does this now.
Description of the Change
Leverage git to index a git-controlled project quickly by using git-ls-files where appropriate
Background
Walking though a large codebase with node fs is very slow. Gecko has almost 200k files, and it takes 17 seconds on a fast machine. The project is constantly being reindexed after each editor focus. There needs to be a better way.
Solution
Allow a native binary to walk the tree. A simple
find
orgit ls-files
is 40x faster than the current nodejs task. I used dugite here to wrap the git commands instead of using child_process because it is more portable and Atom already pulls it in as a dependency elsewhere.EDIT: This is faster because the non-git solution needs to query each file if it is ignored or not. This means reading, at least,
.gitignore
+.git/info/exclude
on each traversed node.Alternate Designs
find
on POSIX systems performs just as well, I decided to go withgit-ls-files
because it is more portable, and should be available on Windows, and because it has a simple flag for excluding VCS ignored files.libgit2
based, library.git-utils
doesn't expose an API that would allow this. I tried replicating git-ls-files withnodegit
to see if it is possible, and it is 4x slower. So I think the git command line solution is the best.child_process.spawn
. This is twice as fast as this patch, because it would allow streaming the stdout.dugite
currently does not allow that and needs a fixed buffer. I went withdugite
because it seemed the most portable, even though it is pretty certain Atom will have access to git in its PATH.Benefits
A 40x speedup. Time to index Gecko before patch: 16.5s. After patch: 0.4s
Possible Drawbacks
Following symlink subdirs is broken in this solution.
Applicable Issues
What does this mean??