On 05/19, Michael Haggerty wrote: > I think it is possible to remove the last magic number and also to > make the CRC handling easier. I have pushed some suggested changes > to github [1]: > > 1. With the current code, trying to read a file that is less than 24 > bytes long would result in a struct.error (because it would try to > unpack a string that is shorter than the struct) whereas the > underlying error in this case should almost always be reported as a > signature error. So it is correct to read the signature separately > from the rest of the header, but it is even more correct to check > the signature (including its length) before reading on. I looked at the current git code, and the filesize is used for checking if the index size is big enough. I'll implement it that way in the prototype for now. > 2. I introduce a class CRC to hold checksums, so that (a) the code > for handling checksums can be encapsulated, and (b) an instance of > this class can be passed into functions and mutated in-place, which > is less cumbersome than requiring functions to return (data, crc) > tuples. This, in turn, makes possible... > > 3. ...a new function read_struct(f, s, crc), which reads the data > for a struct.Struct from f, checksums it, and returns the unpacked > data. This function is more convenient to use than the old > read_calc_crc(). > > 4. The checksum instance can also be made responsible for verifying > that the next four bytes in the file agree with the expected > checksum. This removes some more code duplication. (See > CRC.matches().) > > 5. You read the extension offsets using CRC_STRUCT.size, which is > technically correct but misleading. In fact, the extension offsets > should be documented using their own EXTENSION_INDEX_STRUCT. Also, > it makes more sense to store the unpacked integer offsets to > extoffsets rather than the raw 4-byte strings. > > 6. With a couple of more minor changes it is possible to replace the > magic number "24" in read_index_entries(). With this change the > computation is documented very explicitly and is also (somewhat) > robust against future changes in the format. > > Look over my changes and take whatever you want. Thanks, I didn't merge them directly, but I took the basics out and merged them with my code. -- Thomas -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html