On Tue, Sep 19, 2017 at 08:22:16AM +0200, Michael Haggerty wrote: > Instead of copying data from the `packed-refs` file one line at time > and then processing it, process the data in place as much as possible. > > Also, instead of processing one line per iteration of the main loop, > process a reference line plus its corresponding peeled line (if > present) together. > > Note that this change slightly tightens up the parsing of the > `parse-ref` file. Previously, the parser would have accepted multiple s/parse-ref/packed-refs/, I assume The patch itself looks good, though I did notice an interesting tangent. > + if (eof - pos < GIT_SHA1_HEXSZ + 2 || > + parse_oid_hex(p, &oid, &p) || > + !isspace(*p++)) > + die_invalid_line(refs->path, pos, eof - pos); I wondered why you didn't just check the output of parse_oid_hex(), and included the length check (since in the long run we'd like to get rid of uses of the static GIT_SHA1_HEXSZ macro). I imagine the answer is that this is an mmap'd buffer, and we can't guarantee that parse_oid_hex() wouldn't walk off the end of it. That's fine for now, but I suspect it may become a problem when we move to having a second hash function with a different length. You can't just say "it must have as many bytes as the longest hash", because of course we could have the shorter hash at the end of the buffer. But we also can't say "it must have as many bytes as the shortest hash", because if the content implies it's a longer hash, we'd read off the end of the buffer. I think in the long run we will need a parse_oid_hex() function that takes a ptr/len (or start/end) pair. Sorry, that got a bit off-topic. I don't think there's anything to be done now in your series. It just raises an interesting question for people working on the hash conversion stuff. +cc brian as an FYI. -Peff
