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

From: Josh Steadmon <steadmon@google.com>
Date: Nov 5, 2020, 18:44 UTC
Message-ID: <20201105184455.GA36751@google.com>
In-Reply-To: <xmqqwnz2p0wo.fsf@gitster.c.googlers.com>

On 2020.11.03 13:48, Junio C Hamano wrote:

Show 11 quoted lines

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