Conversation
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on c697ed4..43f6882
| Severity | Location | Issue | Delete |
|---|---|---|---|
| LibArchive.Net/LibArchiveReader.cs:384 | Integer truncation for large files | ||
| LibArchive.Net/LibArchiveReader.cs:426 | Integer truncation for large files |
✅ Files analyzed, no issues (2)
• Test.LibArchive.Net/ReaderTests.cs
• Test.LibArchive.Net/Test.LibArchive.Net.csproj
|
@YoshiRulz Could we handle it better when |
|
I don't know how much better that could be. Maybe -1 for dirs and 0 for empty files? You're already exposing the type. |
|
@YoshiRulz I mean to distinguish between "0 byte file" and "file of unknown length" - probably use |
- Use archive_entry_size_is_set() to check if size is known - LengthBytes is now long? (null when size unknown, value when set) - FileStream.Length throws NotSupportedException when size unknown - Fix IsExternalInit preprocessor symbol (NET5_0_OR_GREATER)
There was a problem hiding this comment.
Pull request overview
This PR adds a LengthBytes property to the Entry class that exposes archive entry sizes via the libarchive archive_entry_size function. The property returns a nullable long (null when size is unknown) and is used to populate the FileStream.Length property, replacing a NotSupportedException with actual size data when available.
Key changes:
- Added
archive_entry_sizeandarchive_entry_size_is_setP/Invoke declarations for both NET7+ (LibraryImport) and older frameworks (DllImport) - Implemented
Entry.LengthBytesproperty that checks if size is set before retrieving it - Updated
FileStreamto accept and expose the length value - Enhanced tests to verify length extraction for various file types (directories, empty files, large files)
However, the PR introduces critical compatibility issues with the net462 target framework by removing language version constraints and using C# 9+ features (records) without conditional compilation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 21 comments.
| File | Description |
|---|---|
| Test.LibArchive.Net/Test.LibArchive.Net.csproj | Removes conditional LangVersion/ImplicitUsings for net462, breaking compatibility with this still-targeted framework |
| Test.LibArchive.Net/ReaderTests.cs | Adds LengthBytes to test assertions; converts ExtractedEntry to unconditional record syntax incompatible with net462; adds IsExternalInit polyfill |
| LibArchive.Net/LibArchiveReader.cs | Implements LengthBytes property using archive_entry_size API; updates FileStream to use length; includes whitespace formatting changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...using
archive_entry_size. The docs mention aarchive_entry_size_is_setbutI guess it's only unset if you're creating an archive, since the unit test checks that a directory has length 0 and that passedit's documented as returning 0 whenarchive_entry_size_is_setis false.High-level PR Summary
This PR adds a
LengthBytesproperty to theEntryclass that exposes the size of archive entries using the underlyingarchive_entry_sizefunction. The new property is populated during entry creation and is used to set theLengthproperty of theFileStreamclass, replacing aNotSupportedExceptionwith an actual value. Tests are updated to verify that the length is correctly extracted for files and directories (including 0-byte files, directories, and larger files like the 1GB zero file).⏱️ Estimated Review Time: 15-30 minutes
💡 Review Order Suggestion
Test.LibArchive.Net/Test.LibArchive.Net.csprojLibArchive.Net/LibArchiveReader.csTest.LibArchive.Net/ReaderTests.csTest.LibArchive.Net/Test.LibArchive.Net.csprojTest.LibArchive.Net/ReaderTests.cs