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.
