
Ripgrep code review - GolDDranks
http://blog.mbrt.it/2016-12-01-ripgrep-code-review/
======
burntsushi
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](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...](https://github.com/BurntSushi/ripgrep/issues/94#issuecomment-261761687)
\--- The author did a great job reviewing the previous solution I used for
colors though, and was something I really wasn't proud of!

~~~
mbrt
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...

~~~
burntsushi
> 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?

~~~
mbrt
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.

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

~~~
orivej
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...](https://ayende.com/blog/posts/series/162754/reviewing-lightning-
memory-mapped-database-library?page=2)

------
kaushalmodi
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!

~~~
nprescott
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](https://github.com/marsmining/ox-
twbs)

~~~
kaushalmodi
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.

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

~~~
alschwalm
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.

~~~
quodlibetor
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/](http://plantuml.com/)

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

~~~
jjm
You could get close with graphiz.

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

[http://www.webgraphviz.com](http://www.webgraphviz.com)

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

~~~
quodlibetor
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:

    
    
      @startuml
      component ripgrep
      component ignore
      component globset
      component grep
    
      ripgrep --> ignore
      ignore --> globset
      ripgrep --> globset
      ripgrep --> grep
      @enduml
    

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...](http://www.plantuml.com/plantuml/uml/SoWkIImgAStDuU9ApiyjoCzBpIjHACeiI2zABK0IoKpFoozAHH98pybFAaujGHO1wU22QbNGrRM3QO9GO16OBcHLMCL0PZcavgK07GO0)

