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

On 2020.11.05 11:21, Junio C Hamano wrote:

Josh Steadmon [off-list ref] writes:

quoted

quoted

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.