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

[Junio C Hamano <gitster@pobox.com>]

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.