| Message ID | YUDAtbHcbv6zNFxe@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | reducing memory allocations for v2 servers | expand |
On Tue, Sep 14, 2021 at 11:33:09AM -0400, Jeff King 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). I tend to fall the same way, especially when bisecting things in ancient (to me) versions of Git where my current compiler complains. So I think the approach that you took here is just fine. (This patch, as with all leading up to it, looks good to me.) Thanks, Taylor
On Tue, Sep 14, 2021 at 12:55:01PM -0400, Taylor Blau wrote: > On Tue, Sep 14, 2021 at 11:33:09AM -0400, Jeff King 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). > > I tend to fall the same way, especially when bisecting things in ancient > (to me) versions of Git where my current compiler complains. So I think > the approach that you took here is just fine. To be clear, the approach here is conservative: it will bisect even with -Werror. I think what you're saying is I _could_ have done the other approach, and put the removal into its own commit? -Peff
On Tue, Sep 14, 2021 at 01:06:42PM -0400, Jeff King wrote: > On Tue, Sep 14, 2021 at 12:55:01PM -0400, Taylor Blau wrote: > > > On Tue, Sep 14, 2021 at 11:33:09AM -0400, Jeff King 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). > > > > I tend to fall the same way, especially when bisecting things in ancient > > (to me) versions of Git where my current compiler complains. So I think > > the approach that you took here is just fine. > > To be clear, the approach here is conservative: it will bisect even with > -Werror. I think what you're saying is I _could_ have done the other > approach, and put the removal into its own commit? Yes. I would have been fine with dropping has_capability() in its own patch, since the result would have been more readable (and since I never bisect with `-Werror`). But this (the conservative approach of persevering bisect-ability even with `-Werror` is fine with me, too). > -Peff Thanks, Taylor
On Tue, 14 Sept 2021 at 17:34, Jeff King <peff@peff.net> wrote: > > Rather than pulling the session-id string from the list of collected > capabilities, we can handle it as soon as we receive it. This gets us > closer to dropping the collected list entirely. Looking good. > As this removes the last caller of the static has_capability(), we can > remove it, as well (and in fact we must to avoid -Wunused-function > complaining). > 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"). > +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. :) Martin
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
diff --git a/serve.c b/serve.c index f6ea2953eb..6bbf54cbbe 100644 --- a/serve.c +++ b/serve.c @@ -57,6 +57,14 @@ static int session_id_advertise(struct repository *r, struct strbuf *value) return 1; } +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); +} + struct protocol_capability { /* * The name of the capability. The server uses this name when @@ -121,6 +129,7 @@ static struct protocol_capability capabilities[] = { { .name = "session-id", .advertise = session_id_advertise, + .receive = session_id_receive, }, { .name = "object-info", @@ -221,26 +230,6 @@ static int parse_command(const char *key, struct protocol_capability **command) return 0; } -static int has_capability(const struct strvec *keys, const char *capability, - const char **value) -{ - int i; - for (i = 0; i < keys->nr; i++) { - const char *out; - if (skip_prefix(keys->v[i], capability, &out) && - (!*out || *out == '=')) { - if (value) { - if (*out == '=') - out++; - *value = out; - } - return 1; - } - } - - return 0; -} - enum request_state { PROCESS_REQUEST_KEYS, PROCESS_REQUEST_DONE, @@ -252,7 +241,6 @@ static int process_request(void) struct packet_reader reader; struct strvec keys = STRVEC_INIT; struct protocol_capability *command = NULL; - const char *client_sid; packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE | @@ -319,9 +307,6 @@ static int process_request(void) the_repository->hash_algo->name, hash_algos[client_hash_algo].name); - if (has_capability(&keys, "session-id", &client_sid)) - trace2_data_string("transfer", NULL, "client-sid", client_sid); - command->command(the_repository, &reader); strvec_clear(&keys);
Rather than pulling the session-id string from the list of collected capabilities, we can handle it as soon as we receive it. This gets us closer to dropping the collected list entirely. The behavior should be the same, with one exception. Previously if the client sent us multiple session-id lines, we'd report only the first. Now we'll pass each one along to trace2. This shouldn't matter in practice, since clients shouldn't do that (and if they do, it's probably sensible to log them all). As this removes the last caller of the static has_capability(), we can remove it, as well (and in fact we must to avoid -Wunused-function complaining). Signed-off-by: Jeff King <peff@peff.net> --- 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. serve.c | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-)