Re: [PATCH 0/6] Yet another approach to handling empty snapshots
Hi Michael, On Wed, 24 Jan 2018, Michael Haggerty wrote: > This patch series fixes the handling of empty packed-refs snapshots > (i.e., those with `snapshot->buf` and friends equal to `NULL`), partly > by changing `snapshot` to store a pointer to the start of the > post-header `packed-refs` content instead of `header_len`. It makes a > couple of other improvements as well. > > I'm not sure whether I like this approach better than the alternative > of always setting `snapshot->buf` to a non-NULL value, by allocating a > length-1 bit of RAM if necessary. The latter is less intrusive, though > even if that approach is taken, I think patches 01, 02, and 04 from > this patch series would be worthwhile improvements. Thank you for Cc:ing me on this patch series. I tried to find some time to review it, I really did, but failed. As I saw that others already had a good look at it, I will just archive the mail thread. I hope you do not mind! Ciao, Dscho
Re: [PATCH 0/6] Yet another approach to handling empty snapshots
Michael Haggerty writes: > This patch series fixes the handling of empty packed-refs snapshots > (i.e., those with `snapshot->buf` and friends equal to `NULL`), partly > by changing `snapshot` to store a pointer to the start of the > post-header `packed-refs` content instead of `header_len`. It makes a > couple of other improvements as well. > > I'm not sure whether I like this approach better than the alternative > of always setting `snapshot->buf` to a non-NULL value, by allocating a > length-1 bit of RAM if necessary. The latter is less intrusive, though > even if that approach is taken, I think patches 01, 02, and 04 from > this patch series would be worthwhile improvements. I do not have a strong preference either way, but somehow feel that this is more "coherent" ;-) That is certainly subjective, though. Thanks.
Re: [PATCH 0/6] Yet another approach to handling empty snapshots
On Wed, Jan 24, 2018 at 12:14:10PM +0100, Michael Haggerty wrote: > This patch series fixes the handling of empty packed-refs snapshots > (i.e., those with `snapshot->buf` and friends equal to `NULL`), partly > by changing `snapshot` to store a pointer to the start of the > post-header `packed-refs` content instead of `header_len`. It makes a > couple of other improvements as well. > > I'm not sure whether I like this approach better than the alternative > of always setting `snapshot->buf` to a non-NULL value, by allocating a > length-1 bit of RAM if necessary. The latter is less intrusive, though > even if that approach is taken, I think patches 01, 02, and 04 from > this patch series would be worthwhile improvements. This looks good to me. I agree that 1, 2, and 4 are improvements regardless (but 4 as it is now depends on 3, right?). I don't have a strong opinion between this series and the other options presented. It's probably not worth agonizing over, so we should pick one and move on. -Peff
[PATCH 0/6] Yet another approach to handling empty snapshots
This patch series fixes the handling of empty packed-refs snapshots (i.e., those with `snapshot->buf` and friends equal to `NULL`), partly by changing `snapshot` to store a pointer to the start of the post-header `packed-refs` content instead of `header_len`. It makes a couple of other improvements as well. I'm not sure whether I like this approach better than the alternative of always setting `snapshot->buf` to a non-NULL value, by allocating a length-1 bit of RAM if necessary. The latter is less intrusive, though even if that approach is taken, I think patches 01, 02, and 04 from this patch series would be worthwhile improvements. Michael Kim Gybels (1): packed_ref_cache: don't use mmap() for small files Michael Haggerty (5): struct snapshot: store `start` rather than `header_len` create_snapshot(): use `xmemdupz()` rather than a strbuf find_reference_location(): make function safe for empty snapshots packed_ref_iterator_begin(): make optimization more general load_contents(): don't try to mmap an empty file refs/packed-backend.c | 106 ++ 1 file changed, 55 insertions(+), 51 deletions(-) -- 2.14.2
