Skip to content

Conversation

@yifanwane
Copy link

Description

This PR implements Project 1 for CS 326 by enhancing the cat user-space command in FogOS/xv6.

New features

-n – number all output lines

-b – number nonempty output lines only

-E – display $ at the end of each line

-s – squeeze multiple consecutive blank lines into a single blank line

Testing

Commands verified inside QEMU:

cat -n test.txt # number all lines
cat -b test.txt # number nonempty lines only
cat -E test.txt # show $ at line ends
cat -s test.txt # squeeze consecutive blank lines
cat -nEs test.txt # combine options
echo "abc" | cat -n # read from stdin
cat -n file1 file2 # multiple files

Edge cases tested: empty file, consecutive blank lines, multiple files, stdin input.
make clean && make qemu builds and runs successfully with no regressions.

Checklist

✔Documentation included (README.md with build/run/test instructions)

✔PR fully resolves the Project 1 proposal

✔Manual tests included and verified inside QEMU

✔Code follows xv6/FogOS formatting standards

✔OS compiles and runs without errors

@pjpatil30
Copy link

I will review this PR.

Copy link

@pjpatil30 pjpatil30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Functionality works as expected in most cases. Documentation was present, but in the README.md, instead of a new file in docs called cat.md. So first recommendation would be to move the documentation there!

I created a new test file as well with more blank lines in between to double check functionality worked. All the individual additions (-n, -b, -E, -s) all work as expected. However, with the multiple one for cat -nEs, it adds an extra number at the end of the file. In my test file, there is a blank line at the end, but -nEs should get rid of all the blank lines and number the remaining lines (as it does through the rest of the file). So, it successfully does it for every line before the last line, just a minor issue with the last line.

Another edge case I caught is when you do -bEs, if the file has blank spaces in between the lines, there is some error with the numbering process (ie it skips over some of the lines).

Other than that, everything else looks good! It is clean, reusable code, and has good comments throughout making it easy to follow.

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?
  • [Partially] Is documentation included? <-- Just fix the location.

Code Checks:

  • Does the code achieve its intended purpose?
  • [Mostly] Were all edge cases considered? <-- Just the two mentioned above.
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

@bnhtphh
Copy link

bnhtphh commented Oct 3, 2025

The code looks great! I can see how the code is progressing and what's happening with it. The documentation, including user/test.txt, Readme.md. I also recommend creating a cat.md file by itself and moving it under the docs folder. Second, I suggest adding one more test file to check multiple empty lines.

I created file.txt
"Line 1: Hello World
Line 2: This is a test

Line 4: After empty line

Line 6: Multiple

consecutive empty lines

Line 10: End of file"

When i test for -s flag, cat -s file.txt, it printed
"Line 1: Hello World
Line 2: This is a test
Line 4: After empty line
Line 6: Multiple
consecutive empty lines
Line 10: End of file"

which I believed should be
"Line 1: Hello World
Line 2: This is a test

Line 4: After empty line

Line 6: Multiple

consecutive empty lines

Line 10: End of file"

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?

@manasakri
Copy link

manasakri commented Oct 11, 2025

Verification
This PR implements an enhanced cat utility with formatting options including -n (number all lines), -b (number nonempty lines), -E (show dollar signs at line ends), and -s (squeeze blank lines). The implementation covers all requirements, and the code compiles successfully.

Testing
I was able to build the project and test the cat utility with multiple flag combinations. Most of the basic functionality works well, but I found a few issues during testing:

- -b flag for blank lines: I tested cat -b test.txt on a file with some empty lines, but the line numbering seemed off. It seems like either it runs with lines that should be skipped or the count gets messed up somewhere. Looking at line 18 in the code, the logic checks to see if a line is empty or not, but this only looks at the first character. If a line starts with a newline, it might not handle it correctly.

- Combining flags: When I tried cat -nb test.txt, the output was confusing because both the -n and -b flags were active at the same time. I see tht the code lets you set both flags , but they conflict with each other since, and i think that the program should probably prevent or warn the user from using both conditions at once.

- Test file: The test file is pretty consistent, but it might be better to include more cases such as blank lines, mixed content, or different edge cases to properly test the flags.

Code Walkthrough
The code structure is pretty simple and easy to follow. cat reads from a file descriptor in chunks and processes each character one at a time, tracking whether we're at the start of a line in order to decide when to the print line numbers. The main function parses user input flags and then either reads from stdin or opens each file argument. The flag parse logic correctly handles combined options, but the logic for the -b and -s flags seem to have issues where the -b check doesn't properly detect empty lines, and the -s logic doesn't correctly squeeze consecutive blanks. The code has good error handling for failed file opens and read errors. The Makefile changes correctly add $U/_cat to the UPROGS list. Overall, the implementation works well.

Formatting
The code was consistent throughout with comments, syntax, spacing, and naming.

High Level Checks:
[x] 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:
[x] 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?

@malensek
Copy link
Contributor

Great work. The CRs here do a great job giving you a roadmap for what to fix, this is one of the few that I can't say there's much to add beyond what was already covered above!

4.5/5

+0.5 -> address the issues raised by the reviewers 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.

5 participants