Hacker News new | past | comments | ask | show | jobs | submit login
Ripgrep code review (mbrt.it)
189 points by GolDDranks on Dec 3, 2016 | hide | past | favorite | 21 comments

ripgrep author here! This is a great review, thanks for doing it!

I'd like to respond to a few of the bad things pointed out. :P

The search code is indeed in a less than ideal state. I've mostly avoided refactoring it because I want to move it to its own separate crate. I've been steadily doing this for other things. Namely, ripgrep used to be a single main crate plus a small regex handling crate (grep), but now it's several: globset, grep, ignore, termcolor and wincolor. I'd like to roll the search code into the grep crate so that others can use it. Once that's done, ripgrep proper will be a pretty small, limited mostly to argv handling and output handling.

I do sometimes get overzealous with inline(always) and inline(never). Both are almost always a result of trying things while profiling, and then forgetting to remove them. If you look closely, most of them are in the core searching code where performance is quite important!

Finally, this code review was done while I was in the middle of moving more of ripgrep code out into the `ignore` and `termcolor` crates. The `ignore` crate does all the gitignore handling (which is quite tricky and is now being used by the tokei project: https://github.com/Aaronepower/tokei) and provides a parallel recursive directory iterator, which made ripgrep even faster! The `termcolor` crate handles cross platform coloring shenanigans, including Windows consoles and mintty. It wasn't fun: https://github.com/BurntSushi/ripgrep/issues/94#issuecomment... --- The author did a great job reviewing the previous solution I used for colors though, and was something I really wasn't proud of!

blog author here. Thanks for your feedback, I appreciate that! I'm gonna edit the post with it. And don't worry about your code... it's the best crate I've seen so far ;)

By the way, shouldn't msys_tty_on_handle be marked 'unsafe' too? I'm wondering if what happens I pass in some garbage as a handle...

> By the way, shouldn't msys_tty_on_handle be marked 'unsafe' too? I'm wondering if what happens I pass in some garbage as a handle...

Possibly. How would you construct said garbage? Would it require unsafe?

I'm not sure, probably yes. It all depends on the winapi crate. If you can get an handle of a different "type" than the expected one, then it's garbage for GetFileInformationByHandleEx. Probably you can't do it without unsafe, so maybe it's fine.

Code review blogs are a cool concept, I hadnt seen it done before. Anyone know of any other good blogs that do this?

Ayende Rahien made an illustrative code review series of LMDB in 2013, particularly good due to its author Howard Chu's responses: https://ayende.com/blog/posts/series/162754/reviewing-lightn...

Some excellent ones here:


Mostly video game related.

Relevant to the discussion but not exactly code reviews is 'The Architecture Of Open Source Applications' - http://aosabook.org/en/index.html

I don't know any about Rust, but indeed Fabien has a really good one about game engines.

I love the ripgrep utility, and it is slowly replacing ag for me. But I am more interested in your blog design. I am an Emacs and Org enthusiast. Would you like to share your blog generation setup? I'd like to learn how you creates the TOC at the end of the page (looks like that on phone) and see if I can recreate that in hugo. Thanks!

Not the author but if you view source or follow their linked github you can find generation is the result of org-mode and org-twbs[0].

[0] https://github.com/marsmining/ox-twbs

Thanks! I was going to do that on getting to a computer. That confirms.. I had seen ox-twbs exported sites before, but not on a phone. It's interesting that the TOC is still retained but put at the bottom.

Indeed it's a default org-twbs export. I have manually adjusted some stuff like the navigation at the top. Yeah the TOC is at the bottom, but on a desktop is on the right. It's bootstrap behavior when you have two columns with div 'col-md-9' and 'col-md-3': on a mobile you get the second column after the first. I couldn't find a way to customize it within Emacs, so I just used it as is. Otherwise I'd have put the TOC at the top, for sure.

What are the benefits of ripgrep over ag?

ripgrep's README will tell you: https://github.com/BurntSushi/ripgrep

The short version:

1. It's faster.

2. It's not just faster on code repos, it's also faster than grep itself on single files, so you can use ripgrep in lieu of both ag and grep.

3. It has Unicode support. (More importantly, its Unicode support is fast, and doesn't exhibit huge slow downs like GNU grep does.)

4. Its support for reading gitignore files is quite a bit less buggy than ag.

5. I've heard folks tell me ag was hard to install on Windows, but ripgrep provides first class support for Windows. I've never personally tried ag on Windows, so take that with a grain of salt.

The (much) longer version is my blog post, which goes into a lot more detail, specifically on 1+2+3: http://blog.burntsushi.net/ripgrep/

I love the graphics and clear examples :) Would read more posts like this.

I'm a fan of the graphics as well. Does anyone know what they were made with? I see the org-mode reference at the bottom of the page, but I assume that doesn't include the images.

Darn, I was hoping that they were created with some custom [plantuml] theme that I could steal, but it seems like they're [manually created] with inkscape.

[plantuml]: http://plantuml.com/

[manually created]: view-source:http://blog.mbrt.it/2016-12-01-ripgrep-code-review/crates.sv...

You could get close with graphiz.

  digraph G {
    "Ripgrep" -> "ignore"
    "ignore" -> "globset"
    "Ripgrep" -> "globset"
    "Ripgrep" -> "grep"

You could also set color, note I didn't do that in this example.

Plantuml is (I think) built on top of graphviz. In any case the syntax is extraordinarily similar. This[1] is similar to the first example:

  component ripgrep
  component ignore
  component globset
  component grep

  ripgrep --> ignore
  ignore --> globset
  ripgrep --> globset
  ripgrep --> grep
You can get the structure right, and easily, but those beautiful icons are not something that I know how to create, and the 90-degree turns are I think actually impossible.

The fact that it is so elegantly expressed in plantuml but it's still so ugly (by default) makes me really wish for a theme that's as nice as the hand-created diagram the author wrote.

[1]: http://www.plantuml.com/plantuml/uml/SoWkIImgAStDuU9ApiyjoCz...

Yes, I used Inkscape, but I'm not a designer, so I bet you can do something similar with a little bit of effort :) I'm not a fan of high level tools... they usually give a correct result, but a crappy look and feel...

Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact