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

From: Jeff King <peff@peff.net>
Date: Sep 14, 2021, 19:14 UTC
Message-ID: <YUD0hXddD1+BF82a@coredump.intra.peff.net>
In-Reply-To: <CAN0heSrCibpcU0NXfZfJvb7puRVLPRz2qEbko6+SEh_BcKtgvA@mail.gmail.com>

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

Show 16 quoted lines

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

Show 9 quoted lines

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