Re: [PATCH 0/3] Supplements to "packed_ref_cache: don't use mmap() for small files"

[Junio C Hamano]

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"

[Johannes Schindelin]

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"

[Michael Haggerty]

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