Re: [PATCH 7/9] ls-refs: ignore very long ref-prefix counts

From: Jeff King <peff@peff.net>
Date: Sep 14, 2021, 17:27 UTC
Message-ID: <YUDbgBfvFU+33Fuz@coredump.intra.peff.net>
In-Reply-To: <YUDZTvI4hrqRK8Th@nand.local>

On Tue, Sep 14, 2021 at 01:18:06PM -0400, Taylor Blau wrote:

Show 32 quoted lines

> > @@ -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. :)

Show 27 quoted lines

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