On Tue, Sep 14, 2021 at 01:18:06PM -0400, Taylor Blau wrote:
> > @@ -156,8 +163,16 @@ int ls_refs(struct repository *r, struct packet_reader *request)
> > data.peel = 1;
> > else if (!strcmp("symrefs", arg))
> > data.symrefs = 1;
> > - else if (skip_prefix(arg, "ref-prefix ", &out))
> > - strvec_push(&data.prefixes, out);
> > + else if (skip_prefix(arg, "ref-prefix ", &out)) {
> > + if (too_many_prefixes) {
> > + /* ignore any further ones */
> > + } else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {
> > + strvec_clear(&data.prefixes);
> > + too_many_prefixes = 1;
> > + } else {
> > + strvec_push(&data.prefixes, out);
> > + }
> > + }
>
> The order of this if-statement is a little odd to me, but obviously
> correct. I might have wrote:
>
> if (too_many_prefixes)
> continue;
>
> if (data.prefixes.nr < MAX_ALLOWED_PREFIXES) {
> strvec_push(&data.prefixes, out);
> } else {
> too_many_prefixes = 1;
> strvec_clear(&data.prefixes);
> }
>
> But certainly what you wrote here works just fine (so this is a cosmetic
> comment, and not a functional one).
My view of it was: check every case that may avoid us pushing a prefix,
and then finally push one. But that may have been related to my goal in
writing the patch. :)
> > +test_expect_success 'ignore very large set of prefixes' '
> > + # generate a large number of ref-prefixes that we expect
> > + # to match nothing; the value here exceeds MAX_ALLOWED_PREFIXES
> > + # from ls-refs.c.
> > + {
> > + echo command=ls-refs &&
> > + echo object-format=$(test_oid algo)
> > + echo 0001 &&
> > + perl -le "print \"refs/heads/$_\" for (1..65536+1)" &&
> > + echo 0000
> > + } |
> > + test-tool pkt-line pack >in &&
> > +
> > + # and then confirm that we see unmatched prefixes anyway (i.e.,
> > + # that the prefix was not applied).
> > + cat >expect <<-EOF &&
> > + $(git rev-parse HEAD) HEAD
> > + $(git rev-parse refs/heads/dev) refs/heads/dev
> > + $(git rev-parse refs/heads/main) refs/heads/main
> > + $(git rev-parse refs/heads/release) refs/heads/release
> > + $(git rev-parse refs/tags/annotated-tag) refs/tags/annotated-tag
> > + $(git rev-parse refs/tags/one) refs/tags/one
> > + $(git rev-parse refs/tags/two) refs/tags/two
>
> You could have written this as a loop over the unmatched prefixes, but I
> vastly prefer the result you came up with, which is much more explicit
> and doesn't require readers to parse out what the loop does.
I actually think that:
git for-each-ref --format='%(objectname) %(refname)' >expect
is pretty readable (and is much more efficient, and nicely avoids the
master/main brittle-ness, which I ran into while backporting this). But:
- this matches what the rest of the script does
- for-each-ref doesn't report on HEAD, so we have to add that in
separately
- the "pkt-line unpack" will include the flush packet, so we'd have to
add that in, too.
-Peff
