[savetheclocktower]

…when searching modified buffers.

Even after adding all those specs in #1455, somehow this regressed.

When we search the entire project for a string, we outsource the vast majority of the task to a background job that uses either scandal or ripgrep. But since those tools can only search what's on disk, we must scour the workspace for modified buffers and search those manually. We even have different code paths for whether the buffer was modified before or after the search was done.

The last few times we've touched find-and-replace it's largely had to do with the complexity of this modified buffer search. It used to ignore path inclusions and exclusions, but we've addressed that… except somewhere along the way it regressed for this simple case:

• Open a project with files a.txt and b.txt.
• Give each one idential dummy (Lorem ipsum dolor sit amet…) text and save them both.
• Open a.txt and add a space at the end of the file so it's considered a modified buffer.
• Do a project-wide search for dolor and leave the file/directory pattern blank.
• You'll see b.txt in the results, but not a.txt.

This happens because

• We are zealous about making sure that a.txt, as a modified buffer, matches the specified file/directory pattern before we consider whether it should show up in the project search results…
• …so when we loop through open modified buffers, we take the buffer's path and try to match it against the specified path inclusions/exclusions…
• …but we're incorrectly interpreting a blank exclusions field as a matcher for a pattern of "" (an empty string).

This last step is where we incorrectly exclude the buffer because we think its path does not match.

The fix — one I've applied both in Workspace::scan and the originating find-and-replace code from results-model.js — is to filter out empty patterns, treating them as invalid.

Testing

A new test covers this scenario; I'm surprised there wasn't one before. The test is in the first commit; this allows you to checkout just that commit and run the spec in order to demonstrate the failure. The next commit fixes it and makes the new spec pass.

[savetheclocktower]

I've just smuggled another small change into this PR: the fix for a trailing / in a path inclusion as discussed here. Two new tests are present in their own separate commit (so that, once again, you can back up one commit and verify that the tests used to fail but pass once the fix is introduced).

[DeeDeeG]

I've tested each improvement locally with yarn start --test node_modules/find-and-replace/spec.

Each spec seems kinda self-explanatory, and they pass once the corresponding fix commit is applied. (I confirm also the inverse case, that the specs fail without their respective fixes, so this ought to show they are sensitive to the intended behavior.)

Tangential details about test method (not super important) (click to expand, if you want):

(An aside: After checking out a given code fix corresponding to one of the spec change commits I had just previously checked out... I had to do rm -rf node_modules/find-and-replace, delete the "find-and-replace@file:packages/find-and-replace": [ . . . ] entry from yarn.lock and then do yarn install again to get the updated src/ code to populate into my node_modules dir, but that's not the PR author's fault!)

In any case, looks great to me!

---

Please consider a code comment in the spot I point out in my comment below. But not a blocker I suppose, though I think the bit of code does need a brief comment I am not going to block on it.

Thanks for doing these fixes, and adding related specs!!! Doing the lord's work, as they say!