Adding "cmp"

[RebeccaLLittle]

Turing in project to add "cmp" functionality.

Contains:

• documentation
• test cases
• cmp logic
• changes to Makefile

[GitMeACoffeeToday]

I am code reviewing this (as the code review ethics dictate)

[GitMeACoffeeToday]

Verification
The original scope of the issue was to make a utility that compares two files byte by byte, or char by char. PR addresses the functionality in whole. No missing functionality.

Testing
“cmp same1.txt same2.txt” returns nothing as expected behavior, but probably should return something to indicate there is no match as unhandled edge cases may result in a similar no-print to terminal, which might cause some confusion.
“cmp tests/same1.txt” has one missing output and is handled, but the print afterwards needs a newline char.
No major issues detected during testing.

Code Walkthrough
1.) cmp.c needs some documentation, i.e. like comments for the compare_files() and main() funcs that describes its function (literally) arguments, and return value.

2.) compare_files(), Getting 1 char from the file at a time may result in many syscalls being made with each use of the read() func. Alternatively, can read larger chunks of bytes at a time (ex. 128 bytes) and store in two large buffers and then compare bytes one by one.

2a.) The buffers can be refilled once empty and additionally, if there is a size mismatch between them afterwards, then it’s indicative of the files not being the same (mismatched sizes).
This excessive syscall issue will probably only come into effect on really large files, of which the test cases are not.
main()

3.) Additionally for main(), could have a more “graceful” exit with print statements to match than just exit(result), as if something goes wrong, the user has more information about it.

Formatting
Formatting is consistent, no issues here.

Overall
The codebase is clean, simple yet quite functional and accomplishes the functionality as described in issue #9. There are only a few small concerns to address, namely a potential performance drop with large file comparisons, missing documentation, and a few QOL changes. Good work.

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)

[NeuerWang]

I am going to review this PR.

[NeuerWang]

Verification
Issue:
“For my project, I want to implement cmp. It will go byte by byte between two files and once it reaches a difference, it will report. If no difference is found, that will also be reported.”

The cmp utility partially meets the requirements of the issue. When there is no difference between the two files, it won't report.

Testing
After testing, the basic functionality of the cmp utility is good. Also, the tests confirm that the utility handles missing files (no_such_file.txt) and comparisons with empty files (empty1.txt) gracefully, which is great.

Code Walkthrough
Most of the code did very well. I think the only thing that is missing is the code for "If no difference is found, that will also be reported.".

Formatting
The code formatting is generally consistent and follows the style of the other user-space utilities in the project. There are no major formatting issues to address.

High Level Checks:
[ ] PR fully resolves the task/issue.
[X] Does the PR have tests?
[X] Does the PR follow project code formatting standards?
[X] Does the OS still compile and run?
[X] Is documentation included?

Code Checks:
[ ] Does the code achieve its intended purpose?
[x] Were all edge cases considered?
[x] Is the code organized and maintainable?
[x] Does the code maintain the same level of performance?

[YuhanZhangxxx]

Verification:
Goal was a tiny file-compare tool and shipping sample files on the image. That works: cmp is present and tests/ is included. Behavior matches expectations.

Testing (in xv6):

• cmp tests/same1.txt tests/same2.txt → no output (same)
• cmp tests/dif1.txt tests/dif2.txt → differ: byte 41, line 1
• cmp tests/dif1.txt tests/eof.txt → differ: byte 26, line 1
• Sanity: echo hello > a.txt; echo hello > b.txt; cmp a.txt b.txt → no output
  Change first letter and re-run → byte 1, line 1

Code Walkthrough:
Straightforward, readable, and does exactly what it should: validate args, open files, compare until first mismatch or EOF.

Formatting:
Looks consistent.

Small suggestion:
There are two fs.img rules in the Makefile. GNU Make merges deps but only runs the last recipe, so order can bite you and tests/ might get skipped later. Keep a single rule (the one with tests) and drop the duplicate:

fs.img: mkfs/mkfs README.md $(UPROGS) tests
	mkfs/mkfs fs.img README.md $(UPROGS) tests

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)

[malensek]

I can understand why folks don't like that it doesn't print anything when there's a mismatch, but that is expected behavior. The way you can tell whether it was the same or not is to look at the exit status of the program.

This works well, does not have any limitations, and meets the requirements. Great work!

I agree with the comment re: system calls being excessive here. But that's something someone could tackle in the future.

5/5