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

[Taylor Blau <me@ttaylorr.com>]

On Fri, May 16, 2025 at 12:27:29PM -0400, Derrick Stolee wrote:
> > 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".

That's fair. After thinking about it some more, I agree that enumerating
[start, start + nr) is clearer than enumerating [0, nr) and adjusting
the offset at each invocation of the loop body.

Funny enough, I applied that suggestion in a couple of spots in one of
the incremental MIDX patches I sent recently.

> > 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.

Oops, this is a typo on my part. I was saying that we should prefer "/*"
to "/**".

The "/**"-style lives on in v4, but this is *extremely* minor and I
don't think is worth sending another round when everything (of what I
have read so far from v4) looks good to me.

Thanks,
Taylor