Supplements to "packed_ref_cache: don't use mmap() for small files"
Get series with b4
b4 am [email protected]Versions newest first
- v1 Latest3 patches 2 repliesIn review
Patches in v1 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/3 | SQUASH? Mention that `snapshot::buf` can be NULL for empty filesrefs/ | +5 | 0 replies | |
| 2/3 | create_snapshot(): exit early if the file was emptyrefs/ | +1 | 0 replies | |
| 3/3 | find_reference_location(): don't invoke if `snapshot->buf` is NULLrefs/ | +8 | 0 replies |
Reviews trailers per patch
None of the 3 patches has a Reviewed-by or Acked-by of their own; the series has one for all of them.
| Patch | Michael Haggerty |
|---|---|
| Series | R-b |
| 1/3 | |
| 2/3 | |
| 3/3 |
Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.
Cover letter [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,
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(-)
Signature · 1 line
2.14.2