On 2020.11.05 11:21, Junio C Hamano wrote: > Josh Steadmon <steadmon@xxxxxxxxxx> 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.
