Add -R, -F, and -v flags to grep

[rnayakusf]

For Issue #54

Overview

This pull request adds the -R, -F, and -v flags to grep, which matches against files in a directory recursively, matches against fixed strings, and inverts matching respectively. A limitation is that the flags must be specified before the pattern or paths, and they must be specified separately.

Additionally, if the -R flag is passed or if multiple paths are specified, matches will be printed with the path prefixed in red.

Examples

• grep -R pattern dir matches pattern against all files in dir and its subdirectories (that can be opened).
• grep -F F.g README.md matches README.md against the fixed string F.g. The pattern is not interpreted as a regular expression, so Fog does not match.
• grep -v Fog README.md matches README.md and prints the lines that don't match the pattern Fog.

Files changed

• user/grep.c

[geoweb999]

I can code review

[atheneem]

I like this project a lot; super useful and well implemented! You have everything and it seems to work exactly as expected. (Unfortunately xv6 being awful with directories makes using the shell for file navigation equally awful, so testing it isn’t the most fun. I imagine it was even more insufferable trying to test your own code for this).

Only things I might consider:

• The three flags being case insensitive would be kinda nice, given that there’s only those and no repeated characters (even though I’m aware you’re matching linux)
• Maybe also assuming the home or current directory if none is given for -r? (But of course not necessary)
• It might be helpful to have a javadoc comment for grep(), especially since it becomes much more complex.
• Also, I know its less straightforward when it’s a solely command line utility, but it would be good to have some sort of tests/proof it works.
• ^ some ideas for this: single and combination flags, empty/nonexistent file, directory as an input w/o -r, special characters and different file lengths… These are all things you can test from the command line of course, and maybe that ends up being more plausible.

High Level Checks:

• PR fully resolves the task/issue
• Does the PR have tests?
• Does the PR follow project code formatting standards?
• Does the OS still compile and run?
• Is documentation included?
  (^maybe just the javadoc thing or notes on how to use it)

Code Checks:

• Does the code achieve its intended purpose?
• Were all edge cases considered?
• Is the code organized and maintainable?
• Does the code maintain the same level of performance? (no performance regressions)

[geoweb999]

I concur with my code review partner's comments, and in addition:

• Thanks for linking the issue# into the PR, very nice!

Testing

• The red text on subdirectories is excellent work and really well done.

For -R flag it seems to work up to 2 nested directories but crashes on the third:

$ mkdir foo1
$ cd foo1
$ /cat /README.md > README.md.1
$ /mkdir foo2
$ cd foo2
$ /cat /README.md > README.md.2
$ /mkdir foo3
$ cd foo3
$ /cat /README.md > README.md.3
$ cd /
$ /grep -R OS foo1
foo1/README.md.1:# FogOS
foo1/README.md.1:![FogOS](docs/fogos.gif)
foo1/foo2/README.md.2:# FogOS
foo1/foo2/README.md.2:![FogOS](docs/fogos.gif)
usertrap(): unexpected scause 0x000000000000000f pid=18
            sepc=0x000000000000013a stval=0x0000000000002d78

grep -R pattern / has a display issue (/ is displayed twice):

$ grep -R OS /
//README.md:# FogOS
//README.md:![FogOS](docs/fogos.gif)
//foo1/README.md.1:# FogOS
//foo1/README.md.1:![FogOS](docs/fogos.gif)
//README.copy.md:# FogOS
//README.copy.md:![FogOS](docs/fogos.gif)

I tried some other really ugly/gnarly combinations of directory structures and grep worked really well.

• As noted above, a test program would be very handy.

Documentation

• A man-style page of documentation with examples would be very handy. It's not clear how much regex is supported.
• Javadoc style comments would be excellent for grep() as noted above

Coding

• Code is really easy to read, particularly the dirgrep function

High Level Checks:

• PR fully resolves the task/issue
• Does the PR have tests?
• Does the PR follow project code formatting standards?
• Does the OS still compile and run?
• Is documentation included?

Code Checks:

• Does the code achieve its intended purpose?
• Were all edge cases considered?
• Is the code organized and maintainable?
• Does the code maintain the same level of performance? (no performance regressions)

[geoweb999]

Confirmed: code fix resolved nested directory crashes.

[geoweb999]

Doc looks fantastic.

[jcchu0]

Will review this.

Similar to own project, flags.

[jcchu0]

Nicely done project.
Though there is hardly any room to add onto comments already mentioned by previous reviewers; I agree with the comments and suggestions made.

Namely:

• The suggestion for the case-insensitive matching
• Dedicated test cases
• Some additional javadoc documentation/explanation on top of the already inserted comments.
• Also like the link/use of regexp, but agree, some documentation of use and scope would be helpful

Suggestion:

• Allowing for handling multiple flags in a single command line argument

High Level Checks:

• PR fully resolves the task/issue
• Does the PR have tests?
• Does the PR follow project code formatting standards?
• Does the OS still compile and run?
• Is documentation included?

Code Checks:

• Does the code achieve its intended purpose?
• Were all edge cases considered?
• Is the code organized and maintainable?
• Does the code maintain the same level of performance? (no performance regressions)

Also notice that effort was made to handle crash fixes already, nice!

[malensek]

This was well reviewed and corrections were already made, awesome!

I agree regarding adding a man page, supporting multiple flags in a single arg, and defaulting to . for -R (I think that's the default behavior for GNU grep at least). These are all great suggestions.

The code is nicely structured. I know the original had a global buf, but I'd probably move that at this point because it doesn't seem necessary and buf gets used in multiple places (same name, different var).

Nice work!