Re: [PATCH v2 6/8] serve.c: add trace2 regions for advertise & command

On Thu, Jul 01 2021, Jeff King wrote:

On Mon, Jun 28, 2021 at 09:19:23PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

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

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

Will change it to be conditional, pending further discussion...