On Tue, Sep 14, 2021 at 09:02:10PM +0200, Martin Ågren wrote:
> > 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.
> > +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
