Files with matches #42

andyleejordan · 2016-09-24T03:19:48Z

Resolves #26 (or at least starts to).

Needs tests
Needs docs
Needs to terminate the search at the first match
Needs to be refactored after a review

andyleejordan · 2016-09-24T03:26:21Z

@BurntSushi So I think, if I'm understanding this code correctly, to terminate early for mmap searching I could just break out of

        for m in self.grep.iter(self.buf) {
            if self.opts.invert_match {
                self.print_inverted_matches(last_end, m.start());
            } else {
                self.print_match(m.start(), m.end());
            }
            last_end = m.end();
        }

t the first match. Is this loop "for matches in lazy iter"? I could use some pointers for sure 😉 (pun totally intended).

For now I'm going to eat some dinner and get to those video games, but this was fun!

BurntSushi · 2016-09-24T03:50:17Z

Is this loop "for matches in lazy iter"? I could use some pointers for sure 😉 (pun totally intended).

You got it, that's right. Breaking out of that loop is perfect.

Note that you'll also need to do this in search_stream.rs, which is a little less nice.

BurntSushi · 2016-09-24T03:50:57Z

@andschwa Thanks for working on this by the way! Enjoy the video games. :-)

andyleejordan · 2016-09-25T00:29:46Z

src/search_stream.rs

@@ -203,6 +213,9 @@ impl<'a, R: io::Read, W: Terminal + Send> Searcher<'a, R, W> {
                break;
            }
            while self.inp.pos < self.inp.lastnl {
+                if self.opts.files_with_matches && self.match_count > 0 {
+                    break;


@BurntSushi in my experimentation, this seemed to work correctly to terminate after one match. However, I expected to be able to just break at the first match further down in the loop... but when I tried that I found that the match_count was > 1 at the top of the loop, which was unexpected. I'm missing something.

match_count could increase by more than than you might expect if you're doing inverted matching. For regular searching, match_count should only be incremented by 1 at most on each iteration.

I would probably move this check to right after the call to read_match. i.e., if self.opts.files_with_matches && matched { break; }.

It can't be right after it because the first match needs to be handled. But what's odd is that if I put it at the end of the while loop, I get this:

loop { println!("{:?} {}", self.path, self.match_count); ... while ... { ... if self.opts.files_with_matches && matched { break; } } }

"src/args.rs" 1 "src/args.rs" 2 "src/args.rs" 3 "src/args.rs" 3

I think I have to break out of both loops maybe.

Oh, yes, indeed you do. The outer loop corresponds to refilling the buffer with data from the file and the inner loop corresponds to iterating over all matches within that buffer. Breaking out of the inner loop won't cause the outer loop to stop.

You might consider changing loop to while !self.opts.files_with_matches || self.match_count == 0 or something similar. (With the case unfortunately repeated inside the while loop as well.)

Added a small terminate() function; it's working just grand.

andyleejordan · 2016-09-25T00:31:53Z

src/search_stream.rs

@@ -281,7 +297,7 @@ impl<'a, R: io::Read, W: Terminal + Send> Searcher<'a, R, W> {

    #[inline(always)]
    fn print_before_context(&mut self, upto: usize) {
-        if self.opts.count || self.opts.before_context == 0 {
+        if self.opts.count || self.opts.files_with_matches || self.opts.before_context == 0 {


@BurntSushi I think it might not be a bad refactor to add an opts.just_file = self.opts.count || self.opts.files_with_matches, which can replace four instances of this or statement; eh?

That's fine, but I wouldn't add a new field. I'd just add a method on the Options struct. :-)

Added opts.skip_matches(), definitely feels better.

andyleejordan · 2016-09-25T01:21:54Z

Just FYI I'm already using this; I wanted to rename a function I'd plumbed through:

./target/debug/rg -l just_file | xargs gsed -i 's/just_file/skip_matches/g'

🎉

andyleejordan · 2016-09-25T02:27:27Z

So either --invert-match or there's a bug: when I combine it with --count (i.e. without these changes), I still get files that have matches listed (is this expected?). So I've removed --files-without-match as I was using the same logic and so had the same bug.

andyleejordan · 2016-09-25T02:49:35Z

@BurntSushi okay this is ready for review!

andyleejordan · 2016-09-25T03:03:55Z

Hm my test failures are due to a few runners swapping the order in which the output was written.

BurntSushi · 2016-09-25T03:04:32Z

Run with -j1.

On Sep 24, 2016 11:03 PM, "Andrew Schwartzmeyer" notifications@github.com
wrote:

Hm my test failures are due to a few runners swapping the order in which
the output was written.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#42 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAb34gPWaSfGYAvmqG4HeVpcGLS2ZWeCks5qteSbgaJpZM4KFiaJ
.

andyleejordan · 2016-09-25T03:23:46Z

Sadly, while -j1 does make it deterministic, it does not account for ordering being different across platforms, which caused most of the failures in build #168 (although a couple failed with very odd panics on tests these changes didn't touch).

Since I don't actually need to test with multiple files... I'm going the simpler route and just testing with sherlock. Slightly less robust; much less prone to pointless failure.

BurntSushi · 2016-09-25T03:25:54Z

Sounds good to me!

On Sep 24, 2016 11:23 PM, "Andrew Schwartzmeyer" notifications@github.com
wrote:

Sadly, while -j1 does make it deterministic, it does not account for
ordering being different across platforms, which caused most of the
failures in build #168
https://travis-ci.org/BurntSushi/ripgrep/builds/162517709 (although a
couple failed with very odd panics on tests these changes didn't touch).

Since I don't actually need to test with multiple files... I'm going the
simpler route and just testing with sherlock. Slightly less robust; much
less prone to pointless failure.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#42 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAb34jSjzwepbCSJGJynrRbxvbC6diNOks5qtelCgaJpZM4KFiaJ
.

andyleejordan · 2016-09-25T03:48:45Z

So, this is admittedly strange, but Travis CI appears to be caching the built targets (and so left around my created test file file from build #168 on the next run build #169, which didn't expect it), causing failures. This appears to be what's happening though because when I added a cargo clean to the CI scripts, they're all now passing build #170. I can squash that into this commit if you agree.

BurntSushi · 2016-09-25T04:21:52Z

That's fine sure, although I haven't seen that happen before!

On Sep 24, 2016 11:48 PM, "Andrew Schwartzmeyer" notifications@github.com
wrote:

So, this is admittedly strange, but Travis CI appears to be caching the
built targets (and so left around my created test file file from build
#168 https://travis-ci.org/BurntSushi/ripgrep/builds/162517709 on the
next run build #169
https://travis-ci.org/BurntSushi/ripgrep/builds/162518787, which didn't
expect it), causing failures. This appears to be what's happening though
because when I added a cargo clean to the CI scripts, they're all now
passing build #170
https://travis-ci.org/BurntSushi/ripgrep/builds/162519607. I can squash
that into this commit if you agree.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#42 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAb34ncj9yfag16hKbsOupFKj9EEQO0Qks5qte8dgaJpZM4KFiaJ
.

Closes BurntSushi#26. Acts like --count but emits only the paths of files with matches, suitable for piping to xargs. Both mmap and no-mmap searches terminate after the first match is found. Documentation updated and tests added.

BurntSushi · 2016-09-25T18:53:32Z

I just reviewed it and this is high quality work. Thank you so much. :-)

Your code refactoring is also going to help fix #77. Yay!

andyleejordan · 2016-09-25T19:31:00Z

Thank you! I really wanted this feature, and missed programming in Rust a lot. Cheers!

Files with matches

andyleejordan commented Sep 25, 2016

View reviewed changes

andyleejordan force-pushed the files-with-matches branch 2 times, most recently from 8c3b62b to 7e83fa2 Compare September 25, 2016 01:16

andyleejordan force-pushed the files-with-matches branch from 7e83fa2 to 7373738 Compare September 25, 2016 02:24

andyleejordan force-pushed the files-with-matches branch 2 times, most recently from 616f51f to 83d5720 Compare September 25, 2016 02:48

andyleejordan force-pushed the files-with-matches branch 3 times, most recently from deef5b5 to f18cb54 Compare September 25, 2016 03:20

andyleejordan force-pushed the files-with-matches branch from f18cb54 to 6869ec0 Compare September 25, 2016 03:24

Add --files-with-matches flag.

a8f3d9e

Closes BurntSushi#26. Acts like --count but emits only the paths of files with matches, suitable for piping to xargs. Both mmap and no-mmap searches terminate after the first match is found. Documentation updated and tests added.

andyleejordan force-pushed the files-with-matches branch from 31ba079 to a8f3d9e Compare September 25, 2016 04:40

BurntSushi merged commit 95edcd4 into BurntSushi:master Sep 25, 2016

andyleejordan deleted the files-with-matches branch September 25, 2016 19:30

amsharma91 added a commit to amsharma91/ripgrep that referenced this pull request Sep 27, 2016

Merge pull request BurntSushi#42 from andschwa/files-with-matches

1a3badc

Files with matches

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Files with matches #42

Files with matches #42

andyleejordan commented Sep 24, 2016 •

edited

Loading

andyleejordan commented Sep 24, 2016

BurntSushi commented Sep 24, 2016

BurntSushi commented Sep 24, 2016

andyleejordan Sep 25, 2016

BurntSushi Sep 25, 2016

andyleejordan Sep 25, 2016

BurntSushi Sep 25, 2016

andyleejordan Sep 25, 2016

andyleejordan Sep 25, 2016

BurntSushi Sep 25, 2016

andyleejordan Sep 25, 2016

andyleejordan commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

BurntSushi commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

BurntSushi commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

BurntSushi commented Sep 25, 2016

BurntSushi commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

Files with matches #42

Files with matches #42

Conversation

andyleejordan commented Sep 24, 2016 • edited Loading

andyleejordan commented Sep 24, 2016

BurntSushi commented Sep 24, 2016

BurntSushi commented Sep 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleejordan commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

BurntSushi commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

BurntSushi commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

BurntSushi commented Sep 25, 2016

BurntSushi commented Sep 25, 2016

andyleejordan commented Sep 25, 2016

andyleejordan commented Sep 24, 2016 •

edited

Loading