Re: [PATCH 1/1] ls-refs.c: minimize number of refs visited

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 19, 2021, 23:00 UTC
Message-ID: <YAdkg7ZijzfB8eMc@nand.local>
In-Reply-To: <YAdjBC7z2eE1Z0uk@coredump.intra.peff.net>

On Tue, Jan 19, 2021 at 05:53:56PM -0500, Jeff King wrote:

Show 8 quoted lines

> Thanks for posting this. I have a vague recollection that we considered
> this either when we did the for-each-ref prefixes, or when we added
> ls-refs prefixes, but I can't seem to find either. At any rate, at
> GitHub we haven't generally found it to be a problem because our
> horrifically-large repos tend to be aggregated alternates repos, not the
> ones people serve upload-pack out of (though I did just time it, and
> some of our largest repos should save a few hundred milliseconds per
> advertisement, which is certainly not nothing).

Great on all counts!

Show 7 quoted lines

> > This commit also fixes a bug in ls-refs.c that was not triggered
> > before: we were using a strvec set to zero, which is not how you are
> > supposed to initialize a strvec. We now call strvec_init after zeroing.
>
> Good catch. It didn't matter until now because nobody relied on having a
> NULL entry when no prefix had been added (instead, they always iterated
> over prefixes->nr). IMHO that is worth fixing as a separate commit.

Yeah. Even after calling it out as such myself, I promptly forgot it when preparing the first patch I sent back to Jacob!

I didn't pull it out into its own patch, and rather folded it in to my 2/2. It has a small call-out of its own, but if you'd prefer it by itself, I'm happy to resubmit it with that change included.

> -Peff

Thanks, Taylor