6 ms·

Ripgrep code review

189 points by GolDDranks 10y ago

trustyhank 10y ago
Code review blogs are a cool concept, I hadnt seen it done before. Anyone know of any other good blogs that do this?
- orivej 10y ago
  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-lightning-memory-mapped-database-library?page=2 https://ayende.com/blog/posts/series/162754/reviewing-lightn...
- pjob 10y ago
  Some excellent ones here: http://fabiensanglard.net/ http://fabiensanglard.net/ Mostly video game related.
- udkl 10y ago
  Relevant to the discussion but not exactly code reviews is 'The Architecture Of Open Source Applications' - http://aosabook.org/en/index.html http://aosabook.org/en/index.html
- mbrt 10y ago
  I don't know any about Rust, but indeed Fabien has a really good one about game engines.
GolDDranks 10y ago
I love the graphics and clear examples :) Would read more posts like this.
- alschwalm 10y ago
  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 10y ago
    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.svg http://blog.mbrt.it/2016-12-01-ripgrep-code-review/crates.sv...
    - jjm 10y ago
      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 10y ago
        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/SoWkIImgAStDuU9ApiyjoCzBpIjHACeiI2zABK0IoKpFoozAHH98pybFAaujGHO1wU22QbNGrRM3QO9GO16OBcHLMCL0PZcavgK07GO0 http://www.plantuml.com/plantuml/uml/SoWkIImgAStDuU9ApiyjoCz...
    - mbrt 10y ago
      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...
kaushalmodi 10y ago
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 10y ago
  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 10y ago
    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.
- YPCrumble 10y ago
  What are the benefits of ripgrep over ag?
  - burntsushi 10y ago
    ripgrep's README will tell you: https://github.com/BurntSushi/ripgrep 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/ http://blog.burntsushi.net/ripgrep/
burntsushi 10y ago
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-261761687 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!
- mbrt 10y ago
  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 10y ago
    > 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?

Ripgrep code review

189 points by GolDDranks 10y ago

trustyhank 10y ago

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

orivej 10y ago
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-lightning-memory-mapped-database-library?page=2 https://ayende.com/blog/posts/series/162754/reviewing-lightn...
pjob 10y ago
Some excellent ones here: http://fabiensanglard.net/ http://fabiensanglard.net/ Mostly video game related.
udkl 10y ago
Relevant to the discussion but not exactly code reviews is 'The Architecture Of Open Source Applications' - http://aosabook.org/en/index.html http://aosabook.org/en/index.html
mbrt 10y ago
I don't know any about Rust, but indeed Fabien has a really good one about game engines.

GolDDranks 10y ago

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

alschwalm 10y ago
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 10y ago
  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.svg http://blog.mbrt.it/2016-12-01-ripgrep-code-review/crates.sv...
  - jjm 10y ago
    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 10y ago
      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/SoWkIImgAStDuU9ApiyjoCzBpIjHACeiI2zABK0IoKpFoozAHH98pybFAaujGHO1wU22QbNGrRM3QO9GO16OBcHLMCL0PZcavgK07GO0 http://www.plantuml.com/plantuml/uml/SoWkIImgAStDuU9ApiyjoCz...
  - mbrt 10y ago
    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...

kaushalmodi 10y ago

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 10y ago
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 10y ago
  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.
YPCrumble 10y ago
What are the benefits of ripgrep over ag?
- burntsushi 10y ago
  ripgrep's README will tell you: https://github.com/BurntSushi/ripgrep 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/ http://blog.burntsushi.net/ripgrep/

burntsushi 10y ago

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

mbrt 10y ago
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 10y ago
  > 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?