-
Notifications
You must be signed in to change notification settings - Fork 25
rgrep branch update #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
I'll be code reviewing this. |
|
Verification Testing Code Walkthrough Formatting Overall High Level Checks:
Code Checks:
|
|
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:
Code Checks:
|
Verification :Issue : Everything works as expected — recursion, -v, and pattern matching all behave correctly. Testing : 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. Formatting : High Level Checks: Code Checks: |
|
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]Does the code achieve its intended purpose? |
|
Looks great overall. I agree that this should use I started testing by creating 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. |
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.