Re: [PATCH v2 04/11] upload-pack: advertise trace2 SID in v0 capabilities

[Josh Steadmon <steadmon@google.com>]

On 2020.11.03 13:48, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > When trace2 is enabled and trace2.advertiseSID is true, advertise
> > upload-pack's trace2 session ID via the new trace2-sid capability.
> 
> I would have imagined when advertiseSID is enabled, trace2, at least
> the part that allocates and assigns the session ID, ought to be
> enabled automatically.
> 
> But the above goes in a different direction and requires both to be
> enabled.  Any compelling reason behind the choice?

My reasoning was that by advertising the capability, you are telling the
remote side "I have definitely produced a log using this session ID. If
you need it later, you can find it with this key". If we advertise a
session ID even when trace2 is not enabled, the remote side can't be as
sure that the received session ID actually points to any useful logs on
the other side.

Of course, this is a weak guarantee since a client could send whatever
it likes regardless of whether anything was logged, or one side could
delete or lose its logs before the other decides it needs to view them.

I think your idea in a different subthread about having a general
session ID not tied to trace2 is interesting, and would also be a point
in favor of changing the current behavior here, but I have some thoughts
on that point that I'll add in the other subthread.

I'm still leaning towards advertising a session ID only if we actually
produced logs locally, but I'm open to further discussion.

> Does the documentation added by this series make it clear that
> asking for advertiseSID does NOT automatically enable allocation of
> session IDs (even if it does not explain why it does not happen)?

In V3 I'll update the docs to call out whichever decision we reach on
this point.

> Thanks.
> 
>