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

feat(fuzzy finder): use ! to search ignored files #856

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

claytonrcarter
Copy link
Contributor

@claytonrcarter claytonrcarter commented Jan 7, 2024

Requirements

  • Filling out the template is required. ✅
  • All new code requires tests to ensure against regressions. ✅

Description of the Change

This PR includes a few changes to the bundled fuzzy finder package:

  • Misc cleanup of unused parameters, unused imports, and some minor cleanup of the package-internal API
  • Removal of the last metrics from fuzzy-finder (re-do of removed metrics fuzzy-finder#4, closes Remove metrics remnants from fuzzy-finder #548)
  • Adds a new feature to the project view (aka file finder) to search for files ignored by git (or other supported VCS). The rest of this description is about this new feature.

This PR adds the ability to search for git/vcs ignored files by prefixing the file finder query with an exclamation point (!). This feature has been a long requested feature in Atom (see #148, dating back to 2015) that has been described by maintainers as something that they would not implement but would welcome a PR for. Well, here it is. 😄 I have been using a version of this feature since as far back as 2017, but is has seen several updates:

  • When the ripgrep path loader was merged back in ... whenever, I updated this to support that new mode. I used that version daily for years, as I recall, and found it to work as expected.
  • I had to update things again just now to work with the recent updates to this package since the pulsar fork, such as moving the fuzzy matcher into core. This is the version presented here, and it has seen little daily use, though the core idea hasn't changed.

Alternate Designs

I chose ! because of it's connotation with negation and this feature feels like you're inverting what you're searching for. Other characters would work just as easily. One possibly drawback of using ! is that it could conflict with a possible future feature that might try to implement a "show files that don't match this query" feature, but I haven't seen that requested, nor have I personally ever felt the need for such a thing.

As I recall, I did not add support for Teletype in this PR, and I don't know that that matters anymore now anyway (see #536). I didn't add this because it seemed like an edge case that might not be worth it; it should still search your local ignored items in Teletype sessions, though. (Again, I have not tested this, but that part of the code was pretty small and straightforward.)

The main thing I don't like about this implementation is how the ripgrep loader depends on the order of indexing to determine if a file is tracked vs ignored. This is because the ripgrep crawler doesn't instantiate a git repo (so isIgnored(file) won't work), and ripgrep doesn't provide a way to search only in ignored files**. I felt that the only way to continue using a similar-ish technique to what already exists in this package was to first index all tracked files, then (if the user has set indexIgnoredPaths to true) go through and immediately reindex all files, tracked and otherwise. We use the existing state from the first crawl (of tracked files) to basically say that any files not in that set must be ignored files. This is not the case for the non-ripgrep loader; that one should only crawl files 1x.

If this is not acceptable, another possibility might be to have rg dump the list of files it would search into a temporary file, then feed that back into rg as a "pattern file" while asking it print non-matching lines. This is trivial from command line, but would involve more Node/fs/process plumbing and it's not clear to me if writing to temp files for things like this is frowned up or not. Open to suggestions on this.

Here's an example of doing this on the command line:

# dump the files that would be searched
rg --files > tracked.txt
# pipe ALL files back through rg, omitting all lines that occur in `tracked.txt`
rg --files --no-ignore | rg --file tracked.txt --fixed-strings --line-regexp --invert-match

** ripgrep searches in tracked files by default, and the --no-ignores flag lets you search all files. There is no way to list/search only ignored files with ripgrep. (See BurntSushi/ripgrep#2029)

Benefits

My main use case is exactly that mentioned in atom/fuzzy-finder#148 (comment): source diving. I use Laravel and I have the vendor/ directory ignored so that find-and-replace and fuzzy finding are limited just to my app files and do not include package dependencies. I often find myself source diving into the framework files, though, and this feature has been incredibly helpful for that.

Another example (also Laravel): we have a .env.example file that is a generic environment config file and is tracked, but we keep our real/dev values in a .env file that is not tracked. When ignoring VCS files, .env never comes up (even though we need to edit pretty regularly) and I have to go hunting for it with my mouse. With this feature, !env pulls it right up.

Possible Drawbacks

Speed is an obvious one, both in indexing and searching through the index, because enabling this feature will then index all of your vendor/, node_modules/, etc directories, and these can been quite large. In practice, though, I've been indexing projects with 30k-60k total files in them and everything has been pretty sprightly, even the older 2011 MBP that I originally used while implementing this.

I did not set the order of the config settings, so the "index ignored files" ends up before the "useRipGrep" option in the settings.

The interaction with the core.excludeVcsIgnoredPaths setting is a little tricky. For example, the behavior of this feature when excludeVcsIgnoredPaths is true makes sense, but when excludeVcsIgnoredPaths is false (ie ignored paths should be included in normal results), then what should this feature do for ! queries? As coded, it shows an empty list, as if nothing matched. Open to suggestions on this.

This also gets to another trick w/ this: there are a lot of odd combos of settings and states that affect how the fuzzy matcher works: use ripgrep or not, is project a git repo or no, is excludeVcsIgnoredPaths true of false, is indexIgnoredFiles true or false, etc. I tried to consider them all, but if you see where I got one of these combinations wrong, please let me know!

The only breaking change that I can think of is for users that are already in the habit of starting their queries with exclamation points as a way to search for tracked files with exclamation marks in their name. If these users set indexIgnoredPaths to true, then these queries will stop working. A workaround is to begin their search with a space, then an exclamation point. This feature is only triggered if the first character of the non-trimmed query is an exclamation point. If the query starts w/ a space, then this feature is bypassed entirely.

Applicable Issues

atom/atom#124

atom/atom#148

atom/atom#214

@claytonrcarter
Copy link
Contributor Author

Hmm, CI failed for 2 packages, one of which isn't related to these changes? Tests are passing locally for me on a mac, and the feature seems to be working as intended when I play with it. I'll try to spin up a linux VM soon to see if it's a platform difference. (I think the CI tests are only run on linux.)

@savetheclocktower
Copy link
Contributor

savetheclocktower commented Jan 7, 2024

I am getting similar failures on macOS as I'm seeing in CI.

Just in case this explains it: if you open a project window on the pulsar root folder, the package tests aren't run when you execute Run Package Specs — only the core editor specs. What I typically do is

cd packages/fuzzy-finder
pulsar --dev .

and then invoke Run Package Specs from there.

(Of course, you've added specs, so that's not likely to be the problem.)

@claytonrcarter
Copy link
Contributor Author

and then invoke Run Package Specs from there.

I didn't know (or had forgotten about) Run Package Specs. Thanks!

So ... Weird. Or interesting? If I run specs from the cli, I see:

  • pulsar -t spec all PASS
  • pulsar -d -t spec all PASS
  • ATOM_DEV_RESOURCE_PATH=~/src/Atom/pulsar pulsar -d -p spec all PASS

If I open the package dir and use Run Package Specs, I see:

  • pulsar package/fuzzy-finder FAIL on 2 specs (match highlighting / when splitting panes with ripgrep both true and false)
  • pulsar -d package/fuzzy-finder FAIL on same 2 specs
  • ATOM_DEV_RESOURCE_PATH=~/src/Atom/pulsar pulsar -d package/fuzzy-finder FAIL on only 1 specs (same as above but only for ripgrep=true)

So, none of these are failing in the same way as in CI. Fun! I still haven't tried this out in a VM though.

@savetheclocktower
Copy link
Contributor

In a few days, once there's less on my plate, we can attempt to figure out the discrepancy.

@claytonrcarter claytonrcarter force-pushed the toggle-ignored branch 4 times, most recently from 8554ff6 to a68d3ce Compare January 16, 2024 12:42
@claytonrcarter
Copy link
Contributor Author

I've only peeked at the failures in CI, but there are two things to note:

  1. the original failure in find-and-replace seems to have been flakey because it cleared up in the latest run (and I didn't change anything in that package),
  2. of the 30 failures in fuzzy-finder, all but 1 are Error: Timed out waiting on anonymous condition

I'll try to scratch at this more soon, to see if I can uncover anything.

Usage of CompositeDisposable was removed when this package was imported into
Pulsar.

Ref: 90566ed
This appears to have been unused for quite some time, since before this was imported into Pulsar.
Most of the metrics were already gone, so this just cleans up the
remnants.

Ref: pulsar-edit/fuzzy-finder#4

Co-Authored-By: @sertonix
There are a lot of parameters being passed around, to lets just bundle them
into an options param and use destructuring to reduce some of the code noise.
@claytonrcarter claytonrcarter force-pushed the toggle-ignored branch 4 times, most recently from 3ed3574 to 4575137 Compare January 24, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove metrics remnants from fuzzy-finder
2 participants