Re: [PATCH v2 08/11] fetch-pack: advertise trace2 SID in capabilities

[Josh Steadmon <steadmon@google.com>]

On 2020.11.05 11:21, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> >> The same comment as 05/11 and 06/11 applies to this repeated
> >> appearance of this boolean expression:
> >> 
> >> 	advertise_trace2_sid && trace2_is_enabled()
> >> 
> >> If we are committed to stick to the "even if we were told to
> >> advertise, do not alllocate a session ID" design, perhaps it is
> >> cleaner to clear advertise_trace2_sid at the very beginning,
> >> immediately after we learn that the tracing is disabled and the
> >> advertiseSID configuration is read.  That way, everybody needs to
> >> only care about advertise_trace2_sid variable.
> >> 
> >> Incidentally, if we decide to change the semantics to auto allocate
> >> the session ID if advertiseSID configuration asks us to advertise
> >> (it is OK if we do not enable the full trace2 suite), we can still
> >> make the code only check advertise_trace2_sid variable, without
> >> adding repeated check of trace2_is_enabled() everywhere at all.
> >
> > Good point. Once we settle on whether or not to advertise when tracing
> > is enabled, I'll update these conditionals in V3.
> 
> Well, we can update these conditionals _before_ deciding that, and
> that is the whole point of the part of my message you are responding
> to.
> 
> Whether the advertise_trace2_sid means 
> 
>  (1) config asked and tracing enabled, or
> 
>  (2) config asked and we do not care whether tracing is enabled or not
> 
> it makes it easier to flip between (1) and (2) later if you clean up
> the conditional first.
> 
> Thanks.
> 

Done in V3. After some additional offline discussions, I've been
convinced that it makes sense to advertise the session-id capability
even when trace2 is not enabled. Specifically, it can be useful on the
server side to identify multiple connections that make up a single
larger operation, such as in a single `repo` invocation.