read_header_int: allow leading spaces #116
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
StefanKarpinski
force-pushed
the
sk/checksum
branch
from
9343121
to
42bd8f2
Compare
Codecov Report
@@ Coverage Diff @@
## master #116 +/- ##
==========================================
+ Coverage 96.05% 96.84% +0.79%
==========================================
Files 4 4
Lines 684 729 +45
==========================================
+ Hits 657 706 +49
+ Misses 27 23 -4
Continue to review full report at Codecov.
|
StefanKarpinski
force-pushed
the
sk/checksum
branch
3 times, most recently
from
2443d4e
to
3175c90
Compare
Some old versions of `tar` (e.g. Solaris) emitted integer fields with
leadings *spaces* which is technically contrary to the standard, but
seems harmless to support. This adds a fragment of this tarball:
https://sparse.tamu.edu/MM/Oberwolfach/LF10.tar.gz
This is a collection of example sparse matrix files. The fragment is a
single header entry followed by one block of data and is a valid tarball
by itself with the idiosyncratic field formatting of Solaris `tar`.
StefanKarpinski
force-pushed
the
sk/checksum
branch
from
3175c90
to
cd623c6
Compare
This is a fairly significant refactoring of the error checking aspect of header parsing with a few high-level improvements. The first major improvement is that this now extracts and checks the version string first, then the checksum and only if those are valid does it try extracting any other fields. This makes the error message a bit clearer: previously you would get a cryptic complaint about an invalid octal digit if you tried to parse something that wasn't a tar file, now at least you get an "invalid version string for tar file" error. This now also dumps the entire header data if there is any error parsing the header, which helps with diagnosis. In addition, the error checking and reporting for boundary conditions in fields which can be malformed (i.e. octal integers) is much improved. Last but not least, the test coverage of all of this is much increased.
StefanKarpinski
force-pushed
the
sk/checksum
branch
from
cd623c6
to
45197a8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a number of changes in this PR, but the main one is the fix for #111 which is to allow leading spaces in integer fields since some old tar programs emitted these fields that way (contrary to the spec). The other substantial change is improved checking and reporting of errors in headers. Here is the list of all changes: