Skip to content

Add HexSHA256File for computing the SHA256 checksum of a file - #6505

Merged
fingolfin merged 2 commits into
gap-system:masterfrom
fingolfin:mh/sha256-file
Aug 14, 2026
Merged

Add HexSHA256File for computing the SHA256 checksum of a file#6505
fingolfin merged 2 commits into
gap-system:masterfrom
fingolfin:mh/sha256-file

Conversation

@fingolfin

@fingolfin fingolfin commented Aug 12, 2026

Copy link
Copy Markdown
Member

Checksumming a file means HexSHA256(InputTextFile(name)), which reads the
whole file into memory and reads it as text: a .gz name is silently
decompressed, and where text and binary mode differ the line endings are
translated. Neither is what you want when checking a download against a
published checksum.

HexSHA256File( <filename>[, <decompress>] ) reads in binary and in chunks.
Passing true opts into the decompression instead, since SyFopen offers it
anyway.

HexSHA256 on a stream now also hashes in chunks; the comment saying the
streams API could not do that predates ReadAll gaining its length argument.

Where the file reading belongs. I put it in the kernel because the library
has no binary-safe read — that is the missing primitive, and this is one
consumer of it. Exposing a general binary read instead would be more useful
and more design; happy to go that way.

make check: 0 failures in 314 files.

Written with Claude Opus 5 via Claude Code; reviewed by me. The commit lists
the tool as co-author.

Checksumming a file so far meant HexSHA256(InputTextFile(name)), which reads
the whole file into memory, and reads it as text: a file whose name ends in
'.gz' is silently decompressed, and on systems distinguishing text and binary
mode the line endings are translated. Neither is what one wants when checking
a downloaded file against a published checksum.

HexSHA256File reads the file in binary and in chunks, so the digest describes
the bytes on disk and the file size is not bounded by memory. Passing 'true'
as second argument opts into the transparent decompression instead, since
SyFopen offers it anyway.

HexSHA256 on a stream now also hashes in chunks rather than calling ReadAll;
the comment claiming the streams API cannot do this predates ReadAll gaining
its length argument.

Assistance from Claude Opus 5 via Claude Code: implementation, documentation
and tests, reviewed by me.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@fingolfin
fingolfin marked this pull request as draft August 12, 2026 12:41
@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Aug 12, 2026
@fingolfin

Copy link
Copy Markdown
Member Author

I've not yet reviewed this AI generated PR myself. So you may wish to hold off a review until I did so to avoid potentially being exposed to slop.

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.61017% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.04%. Comparing base (104ccfa) to head (479241a).

Files with missing lines Patch % Lines
src/sha256.c 91.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6505      +/-   ##
==========================================
- Coverage   79.06%   79.04%   -0.03%     
==========================================
  Files         685      685              
  Lines      293835   293882      +47     
  Branches     8666     8669       +3     
==========================================
- Hits       232325   232295      -30     
- Misses      59710    59783      +73     
- Partials     1800     1804       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fingolfin
fingolfin marked this pull request as ready for review August 12, 2026 20:25
@fingolfin

Copy link
Copy Markdown
Member Author

I feel this is ready for review.

@ThomasBreuer ThomasBreuer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, two minor comments.

Comment thread lib/files.gi Outdated
if not args[2] in [ true, false ] then
ErrorNoReturn("<decompress> must be 'true' or 'false'");
fi;
decompress := args[2];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps assign decompress before the check for true/false, then the error message fits better.

Comment thread lib/files.gi Outdated
end);

InstallGlobalFunction( HexSHA256File,
function(args...)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
function(args...)
function(filename, decompress...)

and then simplify the code accordingly

@lgoettgens lgoettgens left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM (up to Thomas' comments)

@limakzi limakzi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as colleagues.

@ChrisJefferson

Copy link
Copy Markdown
Contributor

Honestly, part of me would prefer a 'binary read'. Also, maybe make that 'decompression' argument compulsory, it's a bit strange if this is the only function in GAP which transparently doesn't decompress, if everything else does -- at least if the argument is required it's obvious (and if someone is using this function they should probably think about if they do want decompression or not).

Everything else in GAP decompresses a '.gz' file as it reads it, so a
default of 'false' here is the one function that quietly does the
opposite.  Requiring the argument makes the choice visible at every call
site, and it removes the optional-argument handling entirely.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@fingolfin

fingolfin commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

Claude replying here:

decompress is required now (9c88d91).

@ChrisJefferson — agreed, and for the reason you give: everything else in GAP
decompresses, so a default of false makes this the one function that quietly
does the opposite. Required also means the reader of a call site sees which of
the two checksums was wanted, which is the thing that is easy to get wrong.

@ThomasBreuer — both your comments dissolve rather than get applied, I hope
acceptably: with two required arguments there is no args... to unpack and no
assignment to move before the check. The usage error goes away too, since
GAP's own arity check reports it.

On the binary read: I agree it is the better primitive and said so in the
description — this is one consumer of a ReadFileBinary that does not exist.
I would rather not design that inside this PR, since it wants its own thought
about streams, chunking and what it returns. If you would like it instead of
this, say so and I will close this and open that; otherwise this can land and
be reimplemented on top of it later, as its whole body is one kernel call.

make testinstall: 0 failures in 314 files. make doc clean.

@fingolfin

Copy link
Copy Markdown
Member Author

OK, the real Max here now (I am sorry for letting loose the AI without checking its comment first, will do that next time):

contrary to what it says, I'd really like to get this PR merged as-is (well, up to fixing bugs and other stuff you guys point out of course). We can look into adding a good "binary file access API" to GAP itself in the future, but I think this is a non-trivial task if we want something that (unlike IO) is not just a thin wrapper around POSIX and hence annoying to support on Windows (I still dream of a "native" MingW GAP) or worse, on future OSes. Can do that, find it interesting -- but's a lot more involved than this PR.

If we ever add ReadFileBinary we can just discard the low-level C implementation here, while the GAP wrapper is kept and just rewritten to use ReadFileBinary.

@fingolfin
fingolfin merged commit e5b44f2 into gap-system:master Aug 14, 2026
30 checks passed
@fingolfin
fingolfin deleted the mh/sha256-file branch August 14, 2026 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: kernel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants