Skip to content

[pset4] [recover] Add hashes for jpeg files without trailing zeros #243

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

Closed
wants to merge 2 commits into from

Conversation

RCW679
Copy link

@RCW679 RCW679 commented May 11, 2024

I wrote Recover from pset4 in such a way that it would actually parse the jpeg headers and look for EOI (FF D9), and stop there. This means that my implementation doesn't save the trailing zeros after EOI -- only to realize that the check50 checks failed with 'recovered image does not match', even though the actual content in the JPEG was the same. I then went to the checks and realized that it was using hashes of JPEG files with the trailing zeros. I then wrote a new Recover in such a way that it preserved the trailing zeros, and the checks passed.

The modifications in this PR add the hashes of the JPEG files without trailing zeros, while preserving the hashes of the JPEG files with the zeros.

@RCW679 RCW679 closed this Nov 2, 2024
@RCW679 RCW679 reopened this Nov 3, 2024
@rongxin-liu rongxin-liu added the CS50x CS50x label Jan 26, 2025
@rongxin-liu
Copy link
Contributor

rongxin-liu commented Jun 28, 2025

Thanks for the thoughtful PR and explanation. Your approach is technically valid—stopping at FF D9 aligns with the JPEG spec. However, for this pset, we're leaning toward requiring students to recover files exactly as they appear on disk, including any trailing zeros. This somewhat reflects real-world data recovery and forensic principles, where byte-for-byte fidelity matters. Supporting multiple output variants would also increase complexity in check50. For those reasons, we’re going to stick with the current strict checks, but we appreciate your effort to improve the check50 checks.

cc @bxngyn @ivanharvard

@rongxin-liu rongxin-liu self-assigned this Jun 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CS50x CS50x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants