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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jul 01 2021, Jeff King wrote:

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