Re: [PATCH 5/9] serve: provide "receive" function for session-id capability

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

quoted

I had originally dropped has_capability() in a separate patch, to keep
this one more readable. That breaks bisectability, but only with
-Werror. I'm not sure where we should fall on that spectrum (I generally
bisect with -Wno-error just because warnings may come and go when
working with different compilers than what was normal at the time).

Not that big a deal either way for this patch, but I wonder if people
have opinions in general.

First of all, agreed about the "not that big a deal" part. Just a random
thought: You could do the opposite of what Elijah sometimes does by
first adding a "MAYBE_UNUSED" function, then actually using it. You'd
add "MAYBE_UNUSED" here, then the next commit would drop the whole
thing. It could be worth it if you're removing many many lines so that
the "actual" change gets lost in the noise. But this patch isn't near
any such threshold, IMHO (if there even is such a "threshold").

Yeah, I considered that (because I had seen Elijah do it; I didn't think
of it myself). I don't love it, if only because now the extra
MAYBE_UNUSED is a head-scratcher for somebody reading the patch. I think
it makes sense if the code will exist in that maybe-unused state for a
while, but here it's just going away immediately anyway. I dunno.

quoted

+static void session_id_receive(struct repository *r,
+                              const char *client_sid)
+{
+       if (!client_sid)
+               client_sid = "";
+       trace2_data_string("transfer", NULL, "client-sid", client_sid);
+}

Handling NULL. Nice. :)

Otherwise segfault if the client just says "session-id". :)

To be clear, the old code behaved the same way. It's just that
has_capability() returned the empty string for this case instead of
NULL. I changed get_capability() to distinguish the two so that the
later fixes for "command=ls-refs=whatever" could treat them differently.

I didn't add tests for this case (nor for "object-format" without a
value), but we could do that if anybody cares.

-Peff