Re: [PATCH v2 6/8] serve.c: add trace2 regions for advertise & command
From: Jeff King <hidden>
Date: 2021-07-01 16:30:06
On Mon, Jun 28, 2021 at 09:19:23PM +0200, Ævar Arnfjörð Bjarmason wrote:
Now that we've factored out "call_{advertise,command}()" in a
preceding commit it becomes easy to trace all these callbacks with
trace2. Let's do that. As the tests demonstrate there's no v2 push
protocol, which the tests assert.Seems reasonable. I haven't ever wanted these myself, but it seems like a natural spot to mention when debugging server-side actions (especially because we may get multiple rounds of "fetch" for a single upload-pack invocation).
quoted hunk ↗ jump to hunk
diff --git a/serve.c b/serve.c index 85cd3eab26e..6dbd05248b9 100644 --- a/serve.c +++ b/serve.c@@ -111,14 +111,34 @@ static struct protocol_capability capabilities[] = { static int call_advertise(struct protocol_capability *command, struct repository *r, struct strbuf *value) { - return command->advertise(r, value); + int ret; + struct strbuf sb = STRBUF_INIT; + const char *msg; + + strbuf_addf(&sb, "advertise/%s", command->name); + trace2_region_enter("serve", sb.buf, r); + ret = command->advertise(r, value); + msg = ret ? "advertised" : "hidden"; + trace2_region_leave_printf("serve", sb.buf, r, "%s", msg); + strbuf_release(&sb);
We'll do these allocations even if trace2 isn't enabled. I guess that's probably not that big a deal in practice. I think: if (trace2_is_enabled()) strbuf_addf(&sb, "advertise/%s", command->name); would work (everything else is cheap and handles the unallocated state fine), but it might not be worth the readability hit (and it's probably premature optimization anyway). -Peff