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

On Tue, Sep 14, 2021 at 09:06:55PM +0200, Martin Ågren wrote:

quoted

But we can do better. Since supporting the ref-prefix capability is
optional anyway, the client has to further cull the response based on
their own patterns. So we can simply ignore the patterns once we cross a
certain threshold. Note that we have to ignore _all_ patterns, not just
the ones past our limit (since otherwise we'd send too little data).

This all makes sense to me. At some point, we should be able to go "I
don't know what you're trying to do, but let me just ignore all this
craziness and instead try to give you a useful result sooner rather than
later".

I do wonder if we should document that the client can't trust us to
actually do all this culling. In general, I find that it's a matter of
hygiene for the client to do its own checks, but with this change they
actually *need* to do them. (Unless they know our limit and that they're
on the right side of it, but that kind of magic is even less hygienic.)

Perhaps we could say so more explicitly in the v2 protocol spec. I'll
take a look.

quoted

+               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);
+                       }
+               }

Is it easier to reason about with something like this
(whitespace-damaged) on top?

You're the second person to complain about this if-else chain. I'll take
the hint. ;)

Hmm. At first I liked this, because it reduces the number of cases (and
variables!). But there's something really subtle going on here. I
thought at first it should be "<", but you are intentionally
over-allocating by one entry to indicate the overflow. I.e., you've
essentially stuffed the too_many_prefixes boolean into the count.

This is far from the parser, but I think that's OK. I'd probably couple
it with a comment explaining why we need to clear rather than using what
we got.

Maybe even name the macro TOO_MANY_PREFIXES (and bump it by one)
to make the logic instead be

        if (data.prefixes.nr < TOO_MANY_PREFIXES)
                strvec_push(&data.prefixes, out);
 ...
        if (data.prefixes.nr >= TOO_MANY_PREFIXES)
                strvec_clear(&data.prefixes);

At first I thought this was just being cute, but it's an attempt to
compensate for the off-by-one subtlety in the early check. I'll give it
some thought. I kind of like it, but the fact that it took me a minute
or three to be sure the code is correct makes me worried it's being too
clever.

-Peff