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

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