Re: [PATCH 0/3] Supplements to "packed_ref_cache: don't use mmap() for small files"
Michael Haggerty writes: > 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. These look all sensible with today's code and with v2 from this thread. With the v3, i.e. "do the xmalloc() even for size==0", however, snapshot->buf would never be NULL, so I'd shelve them for now, though. Thanks.
Re: [PATCH 0/3] Supplements to "packed_ref_cache: don't use mmap() for small files"
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 > > 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
[PATCH 0/3] Supplements to "packed_ref_cache: don't use mmap() for small files"
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 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 refs/packed-backend.c | 21 ++--- 1 file changed, 14 insertions(+), 7 deletions(-) -- 2.14.2