Skip to content

Conversation

@kailash-turimella
Copy link

No description provided.

@peterregal8
Copy link

peterregal8 commented Oct 10, 2025

Verification
The scope of the issue was to create a compress utility that allows users to compress and decompress files using a simple RLE scheme. This PR successfully implements this in full.

Testing
I was able to compile the program and the /compress_test runner worked as expected according to the documentation. However, doing some of my own testing, I noticed a couple of things.

  1. Running compress -c README.md gives the following output:
# FogOS
Fal 2025 Edition
![FogOS](docs/fogos.gif)

Comparing it to the original README file, one of the characters is missing entirely (the extra l in Fall 2025). I'm not sure if that's intentional or not, but still something I thought was important.

  1. Running compress -d with no inputs causes the program to crash. I would expect this behavior, since the decompression needs to take in a file, but it would be nice if there was some sort of error handling to catch these edge cases.

  2. Running compress -d with an input program that is not compressed (such as README.md) causes the console to print out a lot of unhelpful output before printing compress: corrupt input (odd byte count). I don't know if this is possible, but it might be helpful to somehow just skip straight to the last line and avoid all the extra output.

Code Walkthrough
Your compress.c looks great, it's pretty complicated but I appreciated all the comments you added and overall the file is very readable. I notice that a lot of your code is very compact; it's just a personal thing for me but I appreciate if conditionals were given some extra space. Nothing that I would say you have to change though.

Formatting
Despite what I said, your formatting is consistent throughout the codebase, which is great. No issues to report.

High Level Checks:

  • 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?
  • 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?
  • Does the code maintain the same level of performance? (no performance regressions)

@HenrikanHa
Copy link

Even though I did 2 code reviews but I pretty like this one so I do this too!

@manasakri
Copy link

manasakri commented Oct 11, 2025

Verification
This PR implements a compression and decompression utility for FogOSv2, and includes both the main program and a test file for it. The implementation covers all requirements from and successfully compiles without erros.

Testing
I was able to successfully build the project and run the test file in compress_test.c, and all test cases were able to pass without any issues. However, there were a few edge cases, mainly with empty file handling.

  • Missing files/inputs: While running compress -c or compress -d, compress -c without specifying any files, the program just waits for input, and doesn’t handle that case. While it doesn’t crash, it's difficult for the user to know how to proceed, so an error message/instructions on redirection would be helpful. I also think including a ‘help’ or ‘man’ file could be helpful if the user needs more instructions or information.

  • Repetition: I created and tested a file that contains words/characters that appeared and repeated multiple times to see how that would compress. I found that the RLE actually makes the file larger, when I expected the opposite. I’m assuming it’s because the RLE runs for specific instances where the bytes are the same. An error handling or message to the user might help explain why this happens.

  • Test files: The test runs and is successful, but it could be good to have more input validation checks on additional or edge cases, like for those i mentioned above.

Code Walkthrough
The codes for both compress and decompress are fairly straightforward. The compression function correctly tracks the previous byte and counts repeats, writing [count][byte] pairs when it hits a different byte or if it reaches 255. The decompression does the opposite , with some extra logic using a "carry" variable to handle pairs that get split across buff reads. The test file forks new processes to run the actual compress program and compares files byte by byte. Overall I think it looks clean and works well, and wasn’t too difficult to follow along.

Formatting
The code was consistent throughout, and followed idiomatic logics, so I didn’t see any issues with this.

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?
[ ] Were all edge cases considered?
[x] Is the code organized and maintainable?
[x] Does the code maintain the same level of performance?

@HenrikanHa
Copy link

HenrikanHa commented Oct 11, 2025

Verification
The PR adds a working compress utility with -c and -d options, plus documentation and tests. The tool builds and runs in xv6, and /compress_test shows all tests passed, so it meets its goal.

Testing
Basic compression and decompression work correctly when a file is provided. However, when running compress -c or compress -d with no file specified, the program waits for input instead of showing a usage message or an error. Even if the user types something manually, the program produces unexpected or corrupted output. This behavior can confuse users and should be handled more clearly.

$ compress -c 
README.md README.rle
README.md README.rle
$ compress -d
README.rle README.out
EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEErrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrreeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMM.....................................................................uuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu

Code Walkthrough
The code is logically organized, with separate handling for compress and decompress modes. The core RLE logic is easy to follow. However, the program does not handle missing input correctly (e.g., running compress -d with no file causes it to wait or behave unexpectedly). Adding clear input validation or usage messages would improve reliability.

Formatting
The code is formatted consistently and easy to read. Functions and variables are named clearly, and indentation is proper. There are no major formatting issues.

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
Copy link
Contributor

I think that maybe the reviewers did not notice that the program states if no file is included that stdin / stdout are the defaults, so the program is behaving in the intended way and follows usual conventions.

However, this does end up being problematic when you try to do something like cat compressed.bin | compress -d restored.txt because now the program doesn't know whether the positional argument is the file to read from or be written to. I think a good workaround there would be to allow the user to specify "-" as the filename to indicate stdout/stderr.

Another fun thing most compression programs do: you can let the program check if its name (argv[0]) is "decompress" so you wouldn't need the -d flag, and then you can create a hard link from decompress -> compress so the 2nd "utility" doesn't actually take up any space on disk.

The codes and docs read like they have been heavily processed by an LLM, but no credit is given to an LLM. If one was used in this assignment, it should be cited.

5/5

@malensek
Copy link
Contributor

Ah, one more thing: this should be printing error / usage info to stderr, not stdout.

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