Hi Michael, On Mon, 15 Jan 2018, Michael Haggerty wrote: > Thanks for your patch. I haven't measured the performance difference > of `mmap()` vs. `read()` for small `packed-refs` files, but it's not > surprising that `read()` would be faster. > > I especially like the fix for zero-length `packed-refs` files. (Even > though AFAIK Git never writes such files, they are totally legitimate > and shouldn't cause Git to fail.) With or without the additions > mentioned below, > > Reviewed-by: Michael Haggerty <mhagger@xxxxxxxxxxxx> > > While reviewing your patch, I realized that some areas of the existing > code use constructs that are undefined according to the C standard, > such as computing `NULL + 0` and `NULL - NULL`. This was already wrong > (and would come up more frequently after your change). Even though > these are unlikely to be problems in the real world, it would be good > to avoid them. > > So I will follow up this email with three patches: > > 1. Mention that `snapshot::buf` can be NULL for empty files > > I suggest squashing this into your patch, to make it clear that > `snapshot::buf` and `snapshot::eof` can also be NULL if the > `packed-refs` file is empty. > > 2. create_snapshot(): exit early if the file was empty > > Avoid undefined behavior by returning early if `snapshot->buf` is > NULL. > > 3. find_reference_location(): don't invoke if `snapshot->buf` is NULL > > Avoid undefined behavior and confusing semantics by not calling > `find_reference_location()` when `snapshot->buf` is NULL. > > Michael > > Michael Haggerty (3): > SQUASH? Mention that `snapshot::buf` can be NULL for empty files > create_snapshot(): exit early if the file was empty > find_reference_location(): don't invoke if `snapshot->buf` is NULL I reviewed those patches and find the straight-forward (and obviously good). Thanks, Dscho
