Re: [PATCH 6/9] serve: drop "keys" strvec

On Tue, Sep 14, 2021 at 12:59:18PM -0400, Taylor Blau wrote:

On Tue, Sep 14, 2021 at 11:33:18AM -0400, Jeff King wrote:

quoted

We collect the set of capabilities the client sends us in a strvec.
While this is usually small, there's no limit to the number of
capabilities the client can send us (e.g., they could just send us
"agent" pkt-lines over and over, and we'd keep adding them to the list).

Since all code has been converted away from using this list, let's get
rid of it. This avoids a potential attack where clients waste our
memory.

...because now we only bother to tell capabilities about information the
client sent as it happened, instead of accumulating an unbounded set of
strings together into a single strvec.

Right. By the way, one thing I noticed while working on this (that is
unchanged by my series) is that some capabilities are ignored. That make
sense for "agent", where the primary goal is telling the client about
our version (and letting them speak their version, which might be
helpful for debug logs but doesn't impact the protocol itself). But we
do nothing at all with "server-option".

The client wouldn't send one by default, but we do support "git fetch
--server-option". So I assume that it would be a sort of "I'll tell you
this string, and you might feed it to hooks, etc" function, like
push-options. But we just throw it away (well, before my series we
stuffed it into a strvec that nobody looked at).

So possibly we could stop advertising it, but I wonder if any clients
would get unhappy.

I also wonder if jgit uses it. Grepping in jgit.git shows that it is
recorded but I don't see anybody doing anything useful with it; but
maybe some user of the library does.

quoted

@@ -263,10 +263,11 @@ static int process_request(void)
 			/* collect request; a sequence of keys and values */
 			if (parse_command(reader.line, &command) ||
 			    receive_client_capability(reader.line))
-				strvec_push(&keys, reader.line);
+				seen_capability_or_command = 1;
 			else
 				die("unknown capability '%s'", reader.line);

+

Nit; unnecessary whitespace change (but obviously not worth a re-roll on
its own).

Thanks. While this section ended up quite simple, it got rebased a whole
lot of times as I figured out all of quirks that led to the final
protocol-tightening patches. :)

I don't know how I missed the extra line on my final read-through.

-Peff