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

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

Josh Steadmon <steadmon@google.com> writes:

> When trace2 is enabled, the server sent a trace2-sid capability, and
> trace2.advertiseSID is true, advertise fetch-pack's own session ID back
> to the server.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  fetch-pack.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index b10c432315..7fbefa7b8e 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -35,6 +35,8 @@ static int fetch_fsck_objects = -1;
>  static int transfer_fsck_objects = -1;
>  static int agent_supported;
>  static int server_supports_filtering;
> +static int server_sent_trace2_sid;
> +static int advertise_trace2_sid;
>  static struct shallow_lock shallow_lock;
>  static const char *alternate_shallow_file;
>  static struct strbuf fsck_msg_types = STRBUF_INIT;
> @@ -326,6 +328,8 @@ static int find_common(struct fetch_negotiator *negotiator,
>  			if (deepen_not_ok)      strbuf_addstr(&c, " deepen-not");
>  			if (agent_supported)    strbuf_addf(&c, " agent=%s",
>  							    git_user_agent_sanitized());
> +			if (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())
> +				strbuf_addf(&c, " trace2-sid=%s", trace2_session_id());
>  			if (args->filter_options.choice)
>  				strbuf_addstr(&c, " filter");
>  			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
> @@ -979,6 +983,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  				      agent_len, agent_feature);
>  	}
>  
> +	if (server_supports("trace2-sid"))
> +		server_sent_trace2_sid = 1;
> +
>  	if (server_supports("shallow"))
>  		print_verbose(args, _("Server supports %s"), "shallow");
>  	else if (args->depth > 0 || is_repository_shallow(r))
> @@ -1191,6 +1198,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
>  		packet_buf_write(&req_buf, "command=fetch");
>  	if (server_supports_v2("agent", 0))
>  		packet_buf_write(&req_buf, "agent=%s", git_user_agent_sanitized());
> +	if (advertise_trace2_sid && server_supports_v2("trace2-sid", 0) && trace2_is_enabled())
> +		packet_buf_write(&req_buf, "trace2-sid=%s", trace2_session_id());
>  	if (args->server_options && args->server_options->nr &&
>  	    server_supports_v2("server-option", 1)) {
>  		int i;
> @@ -1711,6 +1720,7 @@ static void fetch_pack_config(void)
>  	git_config_get_bool("repack.usedeltabaseoffset", &prefer_ofs_delta);
>  	git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects);
>  	git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects);
> +	git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid);
>  	if (!uri_protocols.nr) {
>  		char *str;

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.