Closed jrwdunham closed 6 years ago
Thanks for the feedback, I will try and review this on monday. Have a great weekend!
@jrwdunham Thanks so much for the feedback, it was very helpful! Below are some comments to go along with my updates
x
for the hex values was probably just left over from an example I borrowed from. We don't actually require it.VCHAR
to hopefully make it more clear it can be any valid character for that encoding except for newlines@jrwdunham if you have more feedback on the changes please feel free to reopen this issue. Thanks!
This issue contains feedback on the bagit1.0 branch, in particular on the ABNF grammars in section 7.
First, a small grammatical issue in the prose:
Future version of BagIt may disallow
should maybe beFuture versions of BagIt may disallow
orA future version of BagIt may disallow
at https://github.com/LibraryOfCongress/bagit-spec/blob/bagit1.0/bagit.xml#L719.ABNF Grammar feedback
The following commentary on the ABNF section of the BagIt1.0 spec, is based on an attempt to create parsers from the supplied ABNF grammars using the Instaparse library. The (still in-development) baglidate repository can be used to test proposed ABNF grammars and determine if they can be used to parse inputs that are expected to be valid.
7.1. Bag Declaration: bagit.txt
bagit.txt
on the lefthand side of the start rule seems a bit odd given that no other nonterminals contain periods; maybebagit-txt
would be better?7.2. Payload Manifest: manifest-algorithm.txt
Here is the original grammar:
The original grammar only captures one line of a manifest-algorithm.txt file. If it is desirable to capture the entire file, the provisional grammar given below shows one approach.
The original grammar requires the character
"x"
as a prefix for all "hex-encoded checksums". Is this intentional? Previous versions of BagIt did not, I believe, require this.The original grammar is missing the
"e"
and"E"
characters from its hexadecimal digit sets.The original grammar allows for effectively empty filenames, i.e., "data/". See the definition of
filename
above. I presume this should not be permitted.The original grammar does not appear to allow for solidus (forward slash) characters in filenames.
A provisional amended grammar is given below. (Note that it does not allow for Unicode characters in filenames, unlike the provisional amended grammar given for 7.3 below.) This grammar addresses the issues described above:
7.3. Bag Metadata: bag-info.txt
Here is the original grammar:
This grammar only captures one line of a bag-info.txt file. If it is desirable to capture the entire file, one could add a rule like
metadata-set = 1*metadata
.The grammar does not allow for non-ASCII (i.e., Unicode) characters despite the fact that the prose of the spec makes it clear that they are licit. The ABNF spec would appear to lack a provision for Unicode also. A provisional definition of a Unicode counterpart to ABNF's
VCHAR
(using Java-compatible regexes) would beUNICODE-VCHAR = ( #'\p{Punct}' / #'\p{L}' / #'\d' )
.The prose clearly states that the keys of bag-info.txt lines can contain spaces, yet the grammar does not allow for this. One solution would be to define
key
as follows (usingUNICODE-VCHAR
defined in the bullet above):key = UNICODE-VCHAR *( UNICODE-VCHAR / WSP )
.In the original grammar,
ending
occurs both at the end ofmetadata
andvalue
. This causes problems for parsing.The original grammar does not capture the fact that continuation lines of a key-value pair must be indented by whitespace.
A provisional amended grammar is given below. Putting aside the fact that it would need revision because it uses regexes outside of the ABNF spec, it does seem to solve the issues described above:
7.4. Fetch File: fetch.txt
Here is the original grammar:
Again, this describes a single line of a fetch.txt file, and not the entire file.
The
length
non-terminal only allows integers 0-9.The
length
rule does not allow for use of the hyphen"-"
instead of an integer, as the text states is permissible.Again, empty filenames like
data/
are permitted when they probably should not be.Small point, but
line-terminator
is used in this grammar whereending
was used for the same rule in the previous grammars.Here is a provisional revised grammar that addresses the issues listed above: