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