Re: [PATCH v2 10/13] pack-objects: refactor path-walk delta phase

> > I imagine that "start" here is setup for the following commit which will
> > parallelize this task across multiple threads, with each thread starting
> > at a different position.
> >
> > I wonder if (as an alternative) we could get away with passing in a
> > (struct packing_region *, size_t) pair and drop "start". I think this
> > can work if you pass in the result of adding whatever the value of
> > "start" would be to to_pack.regions here.
> >
> > That somewhat hides the fact that this code is meant to be run across
> > multiple threads, but in a way that I think is worth doing. It lets the
> > function avoid having to do things like:
> >
> >    for (size_t i = start; i < start + nr; i++)
> >
> > and instead do the simpler:
> >
> >    for (size_t i = 0; i < nr; i++)
> >
> > since the caller already adjusted the regions pointer for us. As a
> > side-effect, it also means that the call below in prepare_pack() can
> > avoid passing a literal zero.
>
> I generally avoid this kind of pattern, because it makes it more
> difficult to understand what is going on when debugging. The cost
> of adding 'start' to the index seems low relative to having a
> consistent indexing scheme when accessing from the "global" list
> of items.
>
> This only gets more complicated when the threaded version iterates
> on a "consecutive sublist of regions".

> > This is an extreme nitpick, but I think that "/**" (as opposed to "/*")
> > is preferred, as the former is used for Doxygen-style comments. I feel
> > like I have seen Junio comment on this before, but searching 'f:Junio
> > "/**"' yields a measly 83,000+ results, so I am unlikely to find it ;-).
> >
> > (Not worth rerolling on its own, but if you are rerolling anyway, which
> > is what I gathered from some of your earlier replies, it may be worth
> > picking up along the way.)
>
> I'm confused. Are you saying I should change from "/**" to "/*" (which
> is the opposite of your preferred statement) or did you misread that I
> had used "/**"? I can't determine which statement is the intended one.