Skip to content

Conversation

@MiloCrespo
Copy link

@MiloCrespo MiloCrespo commented Oct 3, 2025

RGREP

A recursive Version of grep that searches through the entire
directory trees. It finds the thing you are looking for.

Makefile:
Make sure to add rgrep.c to your user folder, and update your UPROGS
section to include -rgrep.

Main Info:
rgrep takes the pattern matching code from regular grep and
adds recursive directory traversal. When it hits a directory,
it opens it, reads all the entries, and recursively searches
subdirectories. Files get searched using the same line-by-line
matching as regular grep.

-V

Flag:
Show all lines that DON'T start with a comment '-v'
rgrep [-v] pattern [directory]

Extras

Note:
Implements custom strcat since xv6's user library doesn't have it

Side Note:
Buffer size: 1024 bytes - Not 512 :'(

TESTING

Tests:
To test I did wget on the time-machine.txt and the used fogOS file.
My series of tests would run as follows

To test flags:

rgrep fog
rgrep -v fog

To Test Case Sensitivity:

rgrep save
rgrep Save

This works when you do the time-machine.txt rm tm.txt and
add to Makefile.

Linux-1024x562

@GitMeACoffeeToday
Copy link

I'll be code reviewing this.

@GitMeACoffeeToday
Copy link

GitMeACoffeeToday commented Oct 3, 2025

Verification
1.) Original scope of issue #2 was to implement a recursive version of the normal grep utility with the addition of the ‘-v’ flag for searching through directories. PR seems to address all the specs as described in the original Github issue.

Testing
1.) No major issues occurred through testing, -v flag correctly inverts the logic for grep and displays all nonmatching entries while normal testing with specific patterns returns accurate results, specifying the directory name works correctly as described.
2.) Minor issue, I noticed that nonmatching words do not result in any output or text being printed out to the terminal. Having a message like “No results matched” or along those lines would be more informative to the user in the case rgrep doesn’t find anything.
3.) Another small thing, I noticed that there weren’t any subdirectories included in your PR with test files (you’ll need the mkfs.c changed file for that) for recursive directory testing, but I didn’t encounter any issues when creating a directory with some dummy txt files.

Code Walkthrough
1.) A little more documentation for the functions would be nice, just for consistency and information, especially for the rgrep(), search_directory(0, main(), matchhere(), matchstar() funcs and so on, otherwise the code is clear and quite readable.
2.) Additionally for the matchhere() and matchstar() funcs, due to the lack of func documentation I’m a bit unsure on their purpose in the rgrep util.

Formatting
1.) Small note on documentation, any .md files describing the function of the project should ideally go in the docs/ directory, otherwise good work.

Overall
No glaring errors and the utility functions as described. There were a few gaps in documentation and possibly test-case wise (no subdirectory testing?), but nothing that shouldn’t take too long to address. Your code was clean and easily readable. Good work on bringing recursive grep to FogOS.

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)

@ccarter2304
Copy link

The implementation and functionality works as expected. The recrusive grep (rgrep) and -v flag works, and when check with case sensitivity it works well. For formatting I think the code is pretty cohesive, but the management of files could be better. Placing the readme for rgrep in docs folder would be more readable but besides that its good. For code readability, I think it would good to add switch cases instead of if statements in the user/rgrep.c, especially when checking the st.type == T_FILE or st.type == T_DIR, switch cases might be a better approach. Also you implement you own strcat, I think it would be better to add it to string.c where all the other string functions are, just so the OS can be cohesive. Other than that there aren't any other comments overall. The tests you mentioned in your docs I felt was good enough for tests, but maybe adding some tests files besides time-machine.txt since that's a large file it's hard to verify if the results from rgrep is correct or not.

High Level Checks:

  • [X ] Is documentation included? This should explain how to build, run, and test the software.
  • PR fully resolves the task/issue
  • Does the PR have tests? ( has commands no test files)
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
  • Is the code organized and maintainable? ( there could be some improvements on organization but not major)
  • Does the code maintain the same level of performance? (no performance regressions)

@YuhanZhangxxx
Copy link

YuhanZhangxxx commented Oct 11, 2025

Verification :

Issue :
This PR adds a recursive version of grep with a -v flag to show non-matching lines. It searches through subdirectories and prints results as filename: line.

Everything works as expected — recursion, -v, and pattern matching all behave correctly.

Testing :
I ran the commands from the README and added a few of my own. Most tests passed with no problems.

The only small issues are that the last line without a newline isn’t printed, and very long lines (over 1024 chars) can get skipped. Both are minor edge cases. Performance is good overall.

Code Walkthrough :

The code is clear and easy to follow. The recursion and matching logic make sense.
Maybe add short comments for rgrep, search_directory, and matchstar.
my_strcat works fine but could check buffer size. Everything else looks clean.

Formatting :
Formatting and indentation are consistent, and the code style looks good.

High Level Checks:
[✅] PR fully resolves the task/issue
[⚠️] Does the PR have tests? (Manual testing only; no separate test files or mkfs fixtures.)
[✅] Does the PR follow project code formatting standards?
[✅] Does the OS still compile and run?
[✅] Is documentation included? (README included and clear enough for testing.)

Code Checks:
[✅] Does the code achieve its intended purpose? (Recursive search and -v flag work as expected.)
[⚠️] Were all edge cases considered? (Minor issues: missing last line without newline, long lines >1024.)
[✅] Is the code organized and maintainable? (Readable and consistent; maybe add a few comments.)
[✅] Does the code maintain the same level of performance? (No slowdowns or regressions.)

@stevenbuks
Copy link

High Level Checks:

So I ran your code and I am confused about why you use write several times instead of printf() in your rgrep.c? Also in your doc I think you got confused by what you want -v flag to do as the docs say it looks for non comments but your code has the flag be an inverse mode. I ran a test by creating nested directories and cat on a str in tm.txt and that worked well. Overall, nice code. There are no tests shown thoooo.

[X ] Is documentation included? This should explain how to build, run, and test the software.
[X] PR fully resolves the task/issue
[NO]Does the PR have tests? ( has no test files)
[X]Does the PR follow project code formatting standards?
[X]Does the OS still compile and run?
Code Checks:

[X]Does the code achieve its intended purpose?
[X]Were all edge cases considered?
[X]Is the code organized and maintainable? ( there could be some improvements on organization but not major)
[X]Does the code maintain the same level of performance? (no performance regressions)

@malensek
Copy link
Contributor

malensek commented Oct 28, 2025

Looks great overall.

I agree that this should use printf instead of write, the docs should be moved, and it really does need more in the way of tests. I was confused what -v did based on the description above as well, but it works as expected based on normal grep.

I started testing by creating a/b/c/d/e and putting tm.txt in there. It crashed when trying to recurse that deep.

I'm not too bothered by fixed sized buffers in this project in general, but it looks like having a line larger than 1024 means the entire rest of the file gets ignored. I modified tm.txt to have a 1300 character line and it just stopped checking lines after that. I think it would make sense to still use a fixed size buffer, but it should still be able to find matches on long lines (maybe those get broken up into 1024-char max outputs, but that's okay) and keep processing the file as well. Looks like it has to do with sizeof(buf)-m-1 prematurely being 0 and causing the whole thing to stop.

4.5/5

+0.5 -> fix recursion bug, long line bug, add tests, and minor cleanups suggested (esp the newline at the end issue) above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants