Re: [PATCH v2 0/7] Better threaded delta resolution in index-pack

[Josh Steadmon <steadmon@google.com>]

On 2019.10.17 13:17, Jonathan Tan wrote:
> Thanks, Stolee and Peff, for taking a look at it. Here is a v2. It is
> mostly unchanged, except for expanded commit messages and code comments.
> 
> I've also added a documentation clarification that
> core.deltaBaseCacheLimit is per-thread, appearing as the first patch in
> this patch series.
> 
> From patch 3 (now patch 4):
> 
> > > +	int i;
> > 
> > Technically this probably ought to be a size_t as well, but I'm much
> > more concerned about the allocation ones, where we might accidentally
> > overflow and underallocate a buffer. Overflowing "i" would probably just
> > lead to an error or bad result.
> 
> I believe this needs to be signed, since we're iterating in reverse
> order, so I made it a ssize_t instead (note the extra "s" in front).
> 
> From patch 4 (now patch 5):
> 
> > > Whenever we make a struct base_data, immediately calculate its delta
> > > children. This eliminates confusion as to when the
> > > {ref,ofs}_{first,last} fields are initialized.
> > 
> > That _seems_ like a good idea, but I'm a little worried just because I
> > don't entirely understand why it was being done lazily before. If you've
> > puzzled all that out, it would be nice to make the argument in the
> > commit message.
> 
> I've added an explanation in the commit message.
> 
> Jonathan Tan (7):
>   Documentation: deltaBaseCacheLimit is per-thread
>   index-pack: unify threaded and unthreaded code
>   index-pack: remove redundant parameter
>   index-pack: remove redundant child field
>   index-pack: calculate {ref,ofs}_{first,last} early
>   index-pack: make resolve_delta() assume base data
>   index-pack: make quantum of work smaller
> 
>  Documentation/config/core.txt |   2 +-
>  builtin/index-pack.c          | 446 ++++++++++++++++++----------------
>  2 files changed, 242 insertions(+), 206 deletions(-)

This series mostly looks good to me. There were a few parts I had
trouble following or convincing myself were safe, so there could be some
improvements with comments or more explicit commit messages, but no
problems apart from that.