Advertise trace2 SID in protocol capabilities

In order to more easily debug remote operations, it is useful to be able to inspect both client-side and server-side traces. This series allows clients to record the server's trace2 session ID, and vice versa, by advertising the SID in a new "trace2-sid" protocol capability.

Two questions in particular for reviewers:

1) Is trace2/tr2_sid.h intended to be visible to the rest of the code, or is the trace2/ directory supposed to be opaque implementation detail? If the latter, would it be acceptable to move tr2_sid_get() into trace2.h?

2) upload-pack generally takes configuration via flags rather than gitconfig. From offline discussions, it sounds like this is an intentional choice to limit potential vulnerability from malicious configs in local repositories accessed via the file:// URL scheme. Is it reasonable to load the trace2.announceSID option from config files in upload-pack, or should this be changed to a flag?

Josh Steadmon (10):
docs: new capability to advertise trace2 SIDs
docs: new trace2.advertiseSID option
upload-pack: advertise trace2 SID in v0 capabilities
receive-pack: advertise trace2 SID in v0 capabilities
serve: advertise trace2 SID in v2 capabilities
transport: log received server trace2 SID
fetch-pack: advertise trace2 SID in capabilities upload-pack, serve: log received client trace2 SID
send-pack: advertise trace2 SID in capabilities
receive-pack: log received client trace2 SID

 Documentation/config/trace2.txt               |  4 +
 .../technical/protocol-capabilities.txt       | 13 ++-
 Documentation/technical/protocol-v2.txt       |  9 +++
 builtin/receive-pack.c                        | 16 ++++
 fetch-pack.c                                  | 11 +++
 send-pack.c                                   |  9 +++
 serve.c                                       | 19 +++++
 t/t5705-trace2-sid-in-capabilities.sh         | 79 +++++++++++++++++++
 transport.c                                   | 10 +++
 upload-pack.c                                 | 23 +++++-
 10 files changed, 190 insertions(+), 3 deletions(-)
 create mode 100755 t/t5705-trace2-sid-in-capabilities.sh

2.29.1.341.ge80a0c044ae-goog

In future patches, we will add the ability for Git servers and clients to advertise their trace2 session IDs via protocol capabilities. This allows for easier debugging when both client and server logs are available.

---
 Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--
 Documentation/technical/protocol-v2.txt           |  9 +++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

index ba869a7d36..73d4ee7f27 100644
@@ -27,8 +27,8 @@ and 'push-cert' capabilities are sent and recognized by the receive-pack
 (push to server) process.
 
 The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
-by both upload-pack and receive-pack protocols.  The 'agent' capability
-may optionally be sent in both protocols.
+by both upload-pack and receive-pack protocols.  The 'agent' and 'trace2-sid'
+capabilities may optionally be sent in both protocols.
 
 All other capabilities are only recognized by the upload-pack (fetch
 from server) process.

@@ -365,3 +365,12 @@ If the upload-pack server advertises the 'filter' capability,
 fetch-pack may send "filter" commands to request a partial clone
 or partial fetch and request that the server omit various objects
 from the packfile.
+
+trace2-sid=<session-id>
+-----------------------
+
+If trace2 tracing is enabled on the server, it may advertise its session ID via
+this capability. The client may choose to log the server's session ID in its
+trace logs, and advertise its own session ID back to the server for it to log
+as well. This allows for easier debugging of remote sessions when both client
+and server logs are available.

index e597b74da3..a5b9ef04f6 100644
@@ -492,3 +492,12 @@ form `object-format=X`) to notify the client that the server is able to deal
 with objects using hash algorithm X.  If not specified, the server is assumed to
 only handle SHA-1.  If the client would like to use a hash algorithm other than
 SHA-1, it should specify its object-format string.
+
+trace2-sid=<session-id>
+~~~~~~~~~~~~~~~~~~~~~~~
+
+If trace2 tracing is enabled on the server, it may advertise its session ID via
+this capability. The client may choose to log the server's session ID in its
+trace logs, and advertise its own session ID back to the server for it to log
+as well. This allows for easier debugging of remote sessions when both client
+and server logs are available.

2.29.1.341.ge80a0c044ae-goog

Document a new config option that allows users to determine whether or not to advertise their trace2 session IDs to remote Git clients and servers.

---
 Documentation/config/trace2.txt | 4 ++++
 1 file changed, 4 insertions(+)

index 01d3afd8a8..3f2e3b4425 100644
@@ -69,3 +69,7 @@ trace2.maxFiles::
 	write additional traces if we would exceed this many files. Instead,
 	write a sentinel file that will block further tracing to this
 	directory. Defaults to 0, which disables this check.
+
+trace2.advertiseSID::
+	Boolean. When true, client and server processes will advertise their
+	trace2 session IDs to their remote counterpart. Defaults to false.

2.29.1.341.ge80a0c044ae-goog

When trace2 is enabled and trace2.advertiseSID is true, advertise upload-pack's trace2 session ID via the new trace2-sid capability.

---
 upload-pack.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

index 3b858eb457..862656010c 100644
@@ -27,6 +27,7 @@
 #include "commit-graph.h"
 #include "commit-reach.h"
 #include "shallow.h"
+#include "trace2/tr2_sid.h"
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE	(1u << 11)

@@ -110,6 +111,7 @@ struct upload_pack_data {
 	unsigned done : 1;					/* v2 only */
 	unsigned allow_ref_in_want : 1;				/* v2 only */
 	unsigned allow_sideband_all : 1;			/* v2 only */
+	unsigned advertise_trace2_sid : 1;
 };
 
 static void upload_pack_data_init(struct upload_pack_data *data)

@@ -141,6 +143,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	packet_writer_init(&data->writer, 1);
 
 	data->keepalive = 5;
+	data->advertise_trace2_sid = 0;
 }
 
 static void upload_pack_data_clear(struct upload_pack_data *data)

@@ -1178,6 +1181,11 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)
 		strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util);
 }
 
+static void format_trace2_info(struct strbuf *buf, struct upload_pack_data *d) {
+	if (d->advertise_trace2_sid && trace2_is_enabled())
+		strbuf_addf(buf, " trace2-sid=%s", tr2_sid_get());
+}
+
 static int send_ref(const char *refname, const struct object_id *oid,
 		    int flag, void *cb_data)
 {

@@ -1193,9 +1201,11 @@ static int send_ref(const char *refname, const struct object_id *oid,
 
 	if (capabilities) {
 		struct strbuf symref_info = STRBUF_INIT;
+		struct strbuf trace2_info = STRBUF_INIT;
 
 		format_symref_info(&symref_info, &data->symref);
-		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s object-format=%s agent=%s\n",
+		format_trace2_info(&trace2_info, data);
+		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n",
 			     oid_to_hex(oid), refname_nons,
 			     0, capabilities,
 			     (data->allow_uor & ALLOW_TIP_SHA1) ?

@@ -1205,9 +1215,11 @@ static int send_ref(const char *refname, const struct object_id *oid,
 			     data->stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     data->allow_filter ? " filter" : "",
+			     trace2_info.buf,
 			     the_hash_algo->name,
 			     git_user_agent_sanitized());
 		strbuf_release(&symref_info);
+		strbuf_release(&trace2_info);
 	} else {
 		packet_write_fmt(1, "%s %s\n", oid_to_hex(oid), refname_nons);
 	}

@@ -1299,6 +1311,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
 		data->allow_sideband_all = git_config_bool(var, value);
 	} else if (!strcmp("core.precomposeunicode", var)) {
 		precomposed_unicode = git_config_bool(var, value);
+	} else if (!strcmp("trace2.advertisesid", var)) {
+		data->advertise_trace2_sid = git_config_bool(var, value);
 	}
 
 	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&

2.29.1.341.ge80a0c044ae-goog

When trace2 is enabled and trace2.advertiseSID is true, advertise receive-pack's trace2 session ID via the new trace2-sid capability.

---
 builtin/receive-pack.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index bb9909c52e..1ff83c874b 100644
@@ -29,6 +29,7 @@
 #include "commit-reach.h"
 #include "worktree.h"
 #include "shallow.h"
+#include "trace2/tr2_sid.h"
 
 static const char * const receive_pack_usage[] = {
 	N_("git receive-pack <git-dir>"),

@@ -54,6 +55,7 @@ static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int advertise_atomic_push = 1;
 static int advertise_push_options;
+static int advertise_trace2_sid;
 static int unpack_limit = 100;
 static off_t max_input_size;
 static int report_status;

@@ -248,6 +250,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "trace2.advertisesid") == 0) {
+		advertise_trace2_sid = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 

@@ -268,6 +275,8 @@ static void show_ref(const char *path, const struct object_id *oid)
 			strbuf_addf(&cap, " push-cert=%s", push_cert_nonce);
 		if (advertise_push_options)
 			strbuf_addstr(&cap, " push-options");
+		if (advertise_trace2_sid && trace2_is_enabled())
+			strbuf_addf(&cap, " trace2-sid=%s", tr2_sid_get());
 		strbuf_addf(&cap, " object-format=%s", the_hash_algo->name);
 		strbuf_addf(&cap, " agent=%s", git_user_agent_sanitized());
 		packet_write_fmt(1, "%s %s%c%s\n",

2.29.1.341.ge80a0c044ae-goog

When trace2 is enabled and trace2.advertiseSID is true, advertise the server's trace2 session ID for all protocol v2 connections via the new trace2-sid capability.

---
 serve.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

index f6341206c4..6ad73d69ab 100644
@@ -7,6 +7,9 @@
 #include "ls-refs.h"
 #include "serve.h"
 #include "upload-pack.h"
+#include "trace2/tr2_sid.h"
+
+static int advertise_trace2_sid;
 
 static int always_advertise(struct repository *r,
 			    struct strbuf *value)

@@ -30,6 +33,15 @@ static int object_format_advertise(struct repository *r,
 	return 1;
 }
 
+static int trace2_advertise(struct repository *r, struct strbuf *value)
+{
+	if (!advertise_trace2_sid || !trace2_is_enabled())
+		return 0;
+	if (value)
+		strbuf_addstr(value, tr2_sid_get());
+	return 1;
+}
+
 struct protocol_capability {
 	/*
 	 * The name of the capability.  The server uses this name when

@@ -66,6 +78,7 @@ static struct protocol_capability capabilities[] = {
 	{ "fetch", upload_pack_advertise, upload_pack_v2 },
 	{ "server-option", always_advertise, NULL },
 	{ "object-format", object_format_advertise, NULL },
+	{ "trace2-sid", trace2_advertise, NULL },
 };
 
 static void advertise_capabilities(void)

@@ -261,6 +274,8 @@ static int process_request(void)
 /* Main serve loop for protocol version 2 */
 void serve(struct serve_options *options)
 {
+	git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid);
+
 	if (options->advertise_capabilities || !options->stateless_rpc) {
 		/* serve by default supports v2 */
 		packet_write_fmt(1, "version 2\n");

2.29.1.341.ge80a0c044ae-goog

When a client receives a trace2-sid capability from a protocol v0, v1, or v2 server, log the received session ID via a trace2 data event.

---
 t/t5705-trace2-sid-in-capabilities.sh | 64 +++++++++++++++++++++++++++
 transport.c                           | 10 +++++
 2 files changed, 74 insertions(+)
 create mode 100755 t/t5705-trace2-sid-in-capabilities.sh

new file mode 100755
index 0000000000..0870e78f7c
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='trace2 SID in capabilities'
+
+. ./test-lib.sh
+
+REPO="$(pwd)/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for trace2 SID capability tests' '
+	git init "$REPO" &&
+	test_commit -C "$REPO" a &&
+	git clone "file://$REPO" "$LOCAL_PRISTINE" &&
+	test_commit -C "$REPO" b
+'
+
+for PROTO in 0 1 2
+do
+	test_expect_success "trace2 session IDs not advertised by default (fetch v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local fetch origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+	'
+
+	test_expect_success "trace2 session IDs not advertised by default (push v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		git -C local pull --no-rebase origin &&
+		GIT_TRACE2_EVENT_NESTING=5 \
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local push origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+	'
+done
+
+test_expect_success 'enable SID advertisement' '
+	git -C "$REPO" config trace2.advertiseSID true &&
+	git -C "$LOCAL_PRISTINE" config trace2.advertiseSID true
+'
+
+for PROTO in 0 1 2
+do
+	test_expect_success "trace2 session IDs advertised (fetch v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local fetch origin &&
+		grep \"key\":\"server-sid\" tr2-client-events
+	'
+
+	test_expect_success "trace2 session IDs advertised (push v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		git -C local pull --no-rebase origin &&
+		GIT_TRACE2_EVENT_NESTING=5 \
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local push origin &&
+		grep \"key\":\"server-sid\" tr2-client-events
+	'
+done
+
+test_done

index 47da955e4f..d16be597bd 100644
@@ -286,6 +286,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
 	struct git_transport_data *data = transport->data;
 	struct ref *refs = NULL;
 	struct packet_reader reader;
+	int sid_len;
+	const char *server_trace2_sid;
 
 	connect_setup(transport, for_push);
 

@@ -297,6 +299,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
 	data->version = discover_version(&reader);
 	switch (data->version) {
 	case protocol_v2:
+		if (server_feature_v2("trace2-sid", &server_trace2_sid))
+			trace2_data_string("trace2", NULL, "server-sid", server_trace2_sid);
 		if (must_list_refs)
 			get_remote_refs(data->fd[1], &reader, &refs, for_push,
 					ref_prefixes,

@@ -310,6 +314,12 @@ static struct ref *handshake(struct transport *transport, int for_push,
 				 for_push ? REF_NORMAL : 0,
 				 &data->extra_have,
 				 &data->shallow);
+		server_trace2_sid = server_feature_value("trace2-sid", &sid_len);
+		if (server_trace2_sid) {
+			char *server_sid = xstrndup(server_trace2_sid, sid_len);
+			trace2_data_string("trace2", NULL, "server-sid", server_sid);
+			free(server_sid);
+		}
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");

2.29.1.341.ge80a0c044ae-goog

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.

---
 fetch-pack.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

index b10c432315..40065fc7dd 100644
@@ -23,6 +23,7 @@
 #include "fetch-negotiator.h"
 #include "fsck.h"
 #include "shallow.h"
+#include "trace2/tr2_sid.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;

@@ -35,6 +36,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 +329,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", tr2_sid_get());
 			if (args->filter_options.choice)
 				strbuf_addstr(&c, " filter");
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);

@@ -979,6 +984,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 +1199,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", tr2_sid_get());
 	if (args->server_options && args->server_options->nr &&
 	    server_supports_v2("server-option", 1)) {
 		int i;

@@ -1711,6 +1721,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;
 

2.29.1.341.ge80a0c044ae-goog

When upload-pack (protocol v0/v1) or a protocol v2 server receives a trace2-sid capability from a client, log the received session ID via a trace2 data event.

---
 serve.c                               |  4 ++++
 t/t5705-trace2-sid-in-capabilities.sh | 19 +++++++++++++------
 upload-pack.c                         |  7 +++++++
 3 files changed, 24 insertions(+), 6 deletions(-)

index 6ad73d69ab..6a4f0c64a9 100644
@@ -202,6 +202,7 @@ static int process_request(void)
 	struct packet_reader reader;
 	struct strvec keys = STRVEC_INIT;
 	struct protocol_capability *command = NULL;
+	const char *client_sid;
 
 	packet_reader_init(&reader, 0, NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |

@@ -265,6 +266,9 @@ static int process_request(void)
 
 	check_algorithm(the_repository, &keys);
 
+	if (has_capability(&keys, "trace2-sid", &client_sid))
+		trace2_data_string("trace2", NULL, "client-sid", client_sid);
+
 	command->command(the_repository, &keys, &reader);
 
 	strvec_clear(&keys);

index 0870e78f7c..3fad9462d3 100755
@@ -17,11 +17,14 @@ test_expect_success 'setup repos for trace2 SID capability tests' '
 for PROTO in 0 1 2
 do
 	test_expect_success "trace2 session IDs not advertised by default (fetch v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local fetch origin &&
-		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+		git -c protocol.version=$PROTO -C local fetch \
+			--upload-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-upload-pack" \
+			origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)" &&
+		test -z "$(grep \"key\":\"client-sid\" tr2-server-events)"
 	'
 
 	test_expect_success "trace2 session IDs not advertised by default (push v${PROTO})" '

@@ -43,11 +46,15 @@ test_expect_success 'enable SID advertisement' '
 for PROTO in 0 1 2
 do
 	test_expect_success "trace2 session IDs advertised (fetch v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
 		cp -r "$LOCAL_PRISTINE" local &&
+		git -C local config trace2.advertiseSID true &&
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local fetch origin &&
-		grep \"key\":\"server-sid\" tr2-client-events
+		git -c protocol.version=$PROTO -C local fetch \
+			--upload-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-upload-pack" \
+			origin &&
+		grep \"key\":\"server-sid\" tr2-client-events &&
+		grep \"key\":\"client-sid\" tr2-server-events
 	'
 
 	test_expect_success "trace2 session IDs advertised (push v${PROTO})" '

index 862656010c..4238e06510 100644
@@ -1059,6 +1059,7 @@ static void receive_needs(struct upload_pack_data *data,
 		const char *features;
 		struct object_id oid_buf;
 		const char *arg;
+		int feature_len;
 
 		reset_timeout(data->timeout);
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL)

@@ -1110,6 +1111,12 @@ static void receive_needs(struct upload_pack_data *data,
 		if (data->allow_filter &&
 		    parse_feature_request(features, "filter"))
 			data->filter_capability_requested = 1;
+		if ((arg = parse_feature_value(features, "trace2-sid", &feature_len, 0)))
+		{
+			char *client_sid = xstrndup(arg, feature_len);
+			trace2_data_string("trace2", NULL, "client-sid", client_sid);
+			free(client_sid);
+		}
 
 		o = parse_object(the_repository, &oid_buf);
 		if (!o) {

2.29.1.341.ge80a0c044ae-goog

When trace2 is enabled, the server sent a trace2-sid capability, and trace2.advertiseSID is true, advertise send-pack's own session ID back to the server.

---
 send-pack.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index eb4a44270b..56c6740e65 100644
@@ -16,6 +16,7 @@
 #include "gpg-interface.h"
 #include "cache.h"
 #include "shallow.h"
+#include "trace2/tr2_sid.h"
 
 int option_parse_push_signed(const struct option *opt,
 			     const char *arg, int unset)

@@ -424,6 +425,8 @@ int send_pack(struct send_pack_args *args,
 	int use_sideband = 0;
 	int quiet_supported = 0;
 	int agent_supported = 0;
+	int server_sent_trace2_sid = 0;
+	int advertise_trace2_sid = 0;
 	int use_atomic = 0;
 	int atomic_supported = 0;
 	int use_push_options = 0;

@@ -435,6 +438,8 @@ int send_pack(struct send_pack_args *args,
 	const char *push_cert_nonce = NULL;
 	struct packet_reader reader;
 
+	git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid);
+
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status-v2"))
 		status_report = 2;

@@ -450,6 +455,8 @@ int send_pack(struct send_pack_args *args,
 		quiet_supported = 1;
 	if (server_supports("agent"))
 		agent_supported = 1;
+	if (server_supports("trace2-sid"))
+		server_sent_trace2_sid = 1;
 	if (server_supports("no-thin"))
 		args->use_thin_pack = 0;
 	if (server_supports("atomic"))

@@ -506,6 +513,8 @@ int send_pack(struct send_pack_args *args,
 		strbuf_addf(&cap_buf, " object-format=%s", the_hash_algo->name);
 	if (agent_supported)
 		strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
+	if (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())
+		strbuf_addf(&cap_buf, " trace2-sid=%s", tr2_sid_get());
 
 	/*
 	 * NEEDSWORK: why does delete-refs have to be so specific to

2.29.1.341.ge80a0c044ae-goog

When receive-pack receives a trace2-sid capability from the client, log the received session ID via a trace2 data event.

---
 builtin/receive-pack.c                |  7 +++++++
 t/t5705-trace2-sid-in-capabilities.sh | 20 ++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

index 1ff83c874b..34199e4933 100644
@@ -2040,6 +2040,7 @@ static struct command *read_head_info(struct packet_reader *reader,
 		if (linelen < reader->pktlen) {
 			const char *feature_list = reader->line + linelen + 1;
 			const char *hash = NULL;
+			const char *client_trace2_sid;
 			int len = 0;
 			if (parse_feature_request(feature_list, "report-status"))
 				report_status = 1;

@@ -2062,6 +2063,12 @@ static struct command *read_head_info(struct packet_reader *reader,
 			}
 			if (xstrncmpz(the_hash_algo->name, hash, len))
 				die("error: unsupported object format '%s'", hash);
+			client_trace2_sid = parse_feature_value(feature_list, "trace2-sid", &len, NULL);
+			if (client_trace2_sid) {
+				char *client_sid = xstrndup(client_trace2_sid, len);
+				trace2_data_string("trace2", NULL, "client-sid", client_sid);
+				free(client_sid);
+			}
 		}
 
 		if (!strcmp(reader->line, "push-cert")) {

index 3fad9462d3..15f78cb7d2 100755
@@ -28,13 +28,17 @@ do
 	'
 
 	test_expect_success "trace2 session IDs not advertised by default (push v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
+		test_when_finished "git -C local push --delete origin new-branch" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		git -C local pull --no-rebase origin &&
 		GIT_TRACE2_EVENT_NESTING=5 \
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local push origin &&
-		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+		git -c protocol.version=$PROTO -C local push \
+			--receive-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-receive-pack" \
+			origin HEAD:new-branch &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)" &&
+		test -z "$(grep \"key\":\"client-sid\" tr2-server-events)"
 	'
 done
 

@@ -58,13 +62,17 @@ do
 	'
 
 	test_expect_success "trace2 session IDs advertised (push v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
+		test_when_finished "git -C local push --delete origin new-branch" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		git -C local pull --no-rebase origin &&
 		GIT_TRACE2_EVENT_NESTING=5 \
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local push origin &&
-		grep \"key\":\"server-sid\" tr2-client-events
+		git -c protocol.version=$PROTO -C local push \
+			--receive-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-receive-pack" \
+			origin HEAD:new-branch &&
+		grep \"key\":\"server-sid\" tr2-client-events &&
+		grep \"key\":\"client-sid\" tr2-server-events
 	'
 done
 

2.29.1.341.ge80a0c044ae-goog

Very nice! This should be very helpful when matching up client and server commands.

I put all the trace2-private stuff in that sub-directory and gave everything tr2_ prefixes to hide the details as much as I could (and as an alternative to the usual tendency of having everything be static within a massive .c file).

So, yeah, my intent was to make all of it opaque.
So that just trace2.h contains the official API.

Perhaps in trace2.c you could create:

const char *trace2_session_id(void)

{
    return tr2_sid_get();
}

I don't have the history to comment on this.

One thing to consider is that the SID for a Git process is built up from the SID of the parent and the unique data for the current process. So for example, if git fetch has SID <sid1> and it spawns git upload-pack, the child will have SID <sid1>/<sid2> and if that spawns git index-pack, that child will have <sid1>/<sid2>/<sid3>. This is very helpful when tracking down parent/child relationships and perf hunting.

This SID inheritance is passed via an environment variable to the child, which extends it and passes the longer version to its children.

So the value being passed between client and server over the protocol may look like <sid1>/<sid2>/<sid3> rather than just a single <sid_x> term. For your purposes, do you want or care if you get the single term or the full SID ?

Also, there's nothing to stop someone from seeding that environment variable in their shell with some mischief before launching the top-level Git command. So the above example might see the SID as <mischief>/<sid1>/<sid2>/<sid3>. I'm not sure if this could be abused when inserted into the V0/V1/V2 protocol or your logging facility.

    $ GIT_TRACE2_EVENT=1 GIT_TRACE2_PARENT_SID=hello git version
    {"event":"version","sid":"hello/20201030T154143.660608Z-H86606a97- 
 P00001d30",...}
    ...

So maybe we want to have a public API to return a pointer to just the final <sid_x> term ? (Then again, maybe I worry too much.)

Thanks,
Jeff

Done in V2, thanks.

I'm not sure we care too much one way or the other. A single component of the SID should be enough to join client & server logs, but it's easier to just send the whole thing.

Yeah, it's certainly possible to muck with the SID as you describe, but I'm not sure I see any big problems that could be caused. If someone points out an issue I've missed, I'll be happy to change this, though.

Thanks for the review!

It may be worth documenting this design decision in the protocol doc; even though protocol doc may say this should be treated as an opaque token, people may assume certain structure.

good point. let me make a note to revisit that text.

thanks
Jeff

Done in V3.

I do not know about your offline discussion, but it certainly would make it easier to reason about the attack surface if we know it never gets affected by any configuration files.

Having said that, upload-pack.c::upload_pack_config() already reads a lot from the configuration file, many of these variables are named "allowSomething", so...

IOW, I do not see why the announceSID (should it be in trace2.* hierarchy, though?) needs to be treated in any more sensitive than say uploadpack.allowrefinwant or *.allowfilter variables.

In order to more easily debug remote operations, it is useful to be able to inspect both client-side and server-side traces. This series allows clients to record the server's trace2 session ID, and vice versa, by advertising the SID in a new "trace2-sid" protocol capability.

Changes since V1:
* Added a public trace2_session_id() function to trace2.{h,c}. Used this in place of tr2_sid_get().
* Fixed a style issue regarding using NULL rather than 0.

Josh Steadmon (11):
docs: new capability to advertise trace2 SIDs
docs: new trace2.advertiseSID option
trace2: add a public function for getting the SID
upload-pack: advertise trace2 SID in v0 capabilities
receive-pack: advertise trace2 SID in v0 capabilities
serve: advertise trace2 SID in v2 capabilities
transport: log received server trace2 SID
fetch-pack: advertise trace2 SID in capabilities upload-pack, serve: log received client trace2 SID
send-pack: advertise trace2 SID in capabilities
receive-pack: log received client trace2 SID

 Documentation/config/trace2.txt               |  4 +
 .../technical/protocol-capabilities.txt       | 13 ++-
 Documentation/technical/protocol-v2.txt       |  9 +++
 builtin/receive-pack.c                        | 15 ++++
 fetch-pack.c                                  | 10 +++
 send-pack.c                                   |  8 ++
 serve.c                                       | 18 +++++
 t/t5705-trace2-sid-in-capabilities.sh         | 79 +++++++++++++++++++
 trace2.c                                      |  5 ++
 trace2.h                                      |  2 +
 transport.c                                   | 10 +++
 upload-pack.c                                 | 22 +++++-
 12 files changed, 192 insertions(+), 3 deletions(-)
 create mode 100755 t/t5705-trace2-sid-in-capabilities.sh

Range-diff against v1:

 -:  ---------- >  1:  d04028c3c7 docs: new capability to advertise trace2 SIDs
 -:  ---------- >  2:  5d5097b671 docs: new trace2.advertiseSID option
 -:  ---------- >  3:  7f42aacd2b trace2: add a public function for getting the SID
 1:  c07bf8aede !  4:  4912af5f2b upload-pack: advertise trace2 SID in v0 capabilities
    @@ Commit message
     
      ## upload-pack.c ##
    -@@
    - #include "commit-graph.h"
    - #include "commit-reach.h"
    - #include "shallow.h"
    -+#include "trace2/tr2_sid.h"
    - 
    - /* Remember to update object flag allocation in object.h */
    - #define THEY_HAVE	(1u << 11)
     @@ upload-pack.c: struct upload_pack_data {
      	unsigned done : 1;					/* v2 only */
      	unsigned allow_ref_in_want : 1;				/* v2 only */
    @@ upload-pack.c: static void format_symref_info(struct strbuf *buf, struct string_
      
     +static void format_trace2_info(struct strbuf *buf, struct upload_pack_data *d) {
     +	if (d->advertise_trace2_sid && trace2_is_enabled())
    -+		strbuf_addf(buf, " trace2-sid=%s", tr2_sid_get());
    ++		strbuf_addf(buf, " trace2-sid=%s", trace2_session_id());
     +}
     +
      static int send_ref(const char *refname, const struct object_id *oid,
 2:  d42118ac41 !  5:  7003189c81 receive-pack: advertise trace2 SID in v0 capabilities
    @@ Commit message
     
      ## builtin/receive-pack.c ##
    -@@
    - #include "commit-reach.h"
    - #include "worktree.h"
    - #include "shallow.h"
    -+#include "trace2/tr2_sid.h"
    - 
    - static const char * const receive_pack_usage[] = {
    - 	N_("git receive-pack <git-dir>"),
     @@ builtin/receive-pack.c: static int receive_unpack_limit = -1;
      static int transfer_unpack_limit = -1;
      static int advertise_atomic_push = 1;
    @@ builtin/receive-pack.c: static void show_ref(const char *path, const struct obje
      		if (advertise_push_options)
      			strbuf_addstr(&cap, " push-options");
     +		if (advertise_trace2_sid && trace2_is_enabled())
    -+			strbuf_addf(&cap, " trace2-sid=%s", tr2_sid_get());
    ++			strbuf_addf(&cap, " trace2-sid=%s", trace2_session_id());
      		strbuf_addf(&cap, " object-format=%s", the_hash_algo->name);
      		strbuf_addf(&cap, " agent=%s", git_user_agent_sanitized());
      		packet_write_fmt(1, "%s %s%c%s\n",
 3:  ff80f70d33 !  6:  9573428cc0 serve: advertise trace2 SID in v2 capabilities
    @@ Commit message
     
      ## serve.c ##
     @@
    - #include "ls-refs.h"
      #include "serve.h"
      #include "upload-pack.h"
    -+#include "trace2/tr2_sid.h"
    -+
    -+static int advertise_trace2_sid;
      
    ++static int advertise_trace2_sid;
    ++
      static int always_advertise(struct repository *r,
      			    struct strbuf *value)
    + {
     @@ serve.c: static int object_format_advertise(struct repository *r,
      	return 1;
      }
    @@ serve.c: static int object_format_advertise(struct repository *r,
     +	if (!advertise_trace2_sid || !trace2_is_enabled())
     +		return 0;
     +	if (value)
    -+		strbuf_addstr(value, tr2_sid_get());
    ++		strbuf_addstr(value, trace2_session_id());
     +	return 1;
     +}
     +
 4:  1519767ead =  7:  21bdbf23f3 transport: log received server trace2 SID
 5:  ee4b3a7a55 !  8:  11b5b1b54f fetch-pack: advertise trace2 SID in capabilities
    @@ Commit message
     
      ## fetch-pack.c ##
    -@@
    - #include "fetch-negotiator.h"
    - #include "fsck.h"
    - #include "shallow.h"
    -+#include "trace2/tr2_sid.h"
    - 
    - static int transfer_unpack_limit = -1;
    - static int fetch_unpack_limit = -1;
     @@ fetch-pack.c: static int fetch_fsck_objects = -1;
      static int transfer_fsck_objects = -1;
      static int agent_supported;
    @@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,
      			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", tr2_sid_get());
    ++				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);
    @@ fetch-pack.c: static int send_fetch_request(struct fetch_negotiator *negotiator,
      	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", tr2_sid_get());
    ++		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;
 6:  b160941c65 !  9:  23f44bc904 upload-pack, serve: log received client trace2 SID
    @@ upload-pack.c: static void receive_needs(struct upload_pack_data *data,
      		if (data->allow_filter &&
      		    parse_feature_request(features, "filter"))
      			data->filter_capability_requested = 1;
    -+		if ((arg = parse_feature_value(features, "trace2-sid", &feature_len, 0)))
    ++		if ((arg = parse_feature_value(features, "trace2-sid", &feature_len, NULL)))
     +		{
     +			char *client_sid = xstrndup(arg, feature_len);
     +			trace2_data_string("trace2", NULL, "client-sid", client_sid);
 7:  609958bd1a ! 10:  c0b1ffc6d8 send-pack: advertise trace2 SID in capabilities
    @@ Commit message
     
      ## send-pack.c ##
    -@@
    - #include "gpg-interface.h"
    - #include "cache.h"
    - #include "shallow.h"
    -+#include "trace2/tr2_sid.h"
    - 
    - int option_parse_push_signed(const struct option *opt,
    - 			     const char *arg, int unset)
     @@ send-pack.c: int send_pack(struct send_pack_args *args,
      	int use_sideband = 0;
      	int quiet_supported = 0;
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      	if (agent_supported)
      		strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
     +	if (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())
    -+		strbuf_addf(&cap_buf, " trace2-sid=%s", tr2_sid_get());
    ++		strbuf_addf(&cap_buf, " trace2-sid=%s", trace2_session_id());
      
      	/*
      	 * NEEDSWORK: why does delete-refs have to be so specific to

8: 3bc412fc39 = 11: c47eddd9df receive-pack: log received client trace2 SID

2.29.1.341.ge80a0c044ae-goog

In future patches, we will add the ability for Git servers and clients to advertise their trace2 session IDs via protocol capabilities. This allows for easier debugging when both client and server logs are available.

---
 Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--
 Documentation/technical/protocol-v2.txt           |  9 +++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

index ba869a7d36..73d4ee7f27 100644
@@ -27,8 +27,8 @@ and 'push-cert' capabilities are sent and recognized by the receive-pack
 (push to server) process.
 
 The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
-by both upload-pack and receive-pack protocols.  The 'agent' capability
-may optionally be sent in both protocols.
+by both upload-pack and receive-pack protocols.  The 'agent' and 'trace2-sid'
+capabilities may optionally be sent in both protocols.
 
 All other capabilities are only recognized by the upload-pack (fetch
 from server) process.

@@ -365,3 +365,12 @@ If the upload-pack server advertises the 'filter' capability,
 fetch-pack may send "filter" commands to request a partial clone
 or partial fetch and request that the server omit various objects
 from the packfile.
+
+trace2-sid=<session-id>
+-----------------------
+
+If trace2 tracing is enabled on the server, it may advertise its session ID via
+this capability. The client may choose to log the server's session ID in its
+trace logs, and advertise its own session ID back to the server for it to log
+as well. This allows for easier debugging of remote sessions when both client
+and server logs are available.

index e597b74da3..a5b9ef04f6 100644
@@ -492,3 +492,12 @@ form `object-format=X`) to notify the client that the server is able to deal
 with objects using hash algorithm X.  If not specified, the server is assumed to
 only handle SHA-1.  If the client would like to use a hash algorithm other than
 SHA-1, it should specify its object-format string.
+
+trace2-sid=<session-id>
+~~~~~~~~~~~~~~~~~~~~~~~
+
+If trace2 tracing is enabled on the server, it may advertise its session ID via
+this capability. The client may choose to log the server's session ID in its
+trace logs, and advertise its own session ID back to the server for it to log
+as well. This allows for easier debugging of remote sessions when both client
+and server logs are available.

2.29.1.341.ge80a0c044ae-goog

The computed part of trace2 SIDs are relatively safe (both for length and funky characters). And funky characters are protected by JSON encoding rules when writing to the GIT_TRACE2_EVENT target.

And I think the computed part would be safe in this context. I've not seen commands that spawn more than about 6 levels of child processes, and those would be fine here.

However, the opportunity to introduce a prefix as I suggested earlier does offer the opportunity to introduce funky chars that would not have any protection, so you may want to c-quote the string when inserting it into the wire protocol.

    $ GIT_TRACE2_EVENT=1 GIT_TRACE2_PARENT_SID=hello git version
    {"event":"version","sid":"hello/20201030T154143.660608Z-H86606a97- 
P00001d30",...}
    ...

(Allowing the user to insert a prefix like that has been useful for tracking control/experiment testing, so I wouldn't want to disable it.)

Jeff

AFAICT the way it's documented now is the "is the session-id[...]" paragraph in api-trace2.txt.

I'd like to see us document the implementation a bit better and explicitly support the "hack" of setting GIT_TRACE2_PARENT_SID=hello.

I.e. maybe I've missed something but we just say "session-id is prepended with the session-id of the parent" but don't mention that we separate them with slashes, so you can split on that to get the depth & individual ID's.

My reading of the updated doc patch in v3 is that not allowing "non-printable or whitespace" allows you to e.g. have slashes in those custom session IDs, which would be quite inconvenient since it would break that property.

And we should explicitly support the GIT_TRACE2_PARENT_SID=* setting from an external process, and make the SID definition loose enough to allow for SIDs that don't look like Git's in that chain. I.e. a useful property (and one I've seen in the wild) is to have some external programt that already has SIDs/UUID run IDs spawn git, setting GIT_TRACE2_PARENT_SID=<that program's SID> makes things convenient for the purposes of logging.n

A few things I want to see stakeholders agree on:

- In "a/b/c", what's the "session ID" of the leaf child process? "a/b/c"? or "c"? If the former (which is what I am guessing), do we have name to refer to "b" or "c" alone (if not, we should have one)?

- Do we encourage/force other implementations of Git protocol to adopt a similar "slash-separated non-whitespace ASCII printable" structure? I do not think such a structure is too limiting but others may feel differently. Or is a "session ID" supposed to be an opaque token without any structure, and upon seeing "a/b/c" the recipient should not read anything into its slash, or any relation with another session whose ID is "a/b/d"?

I've occasionally used that hack for control/experiment-type testing, but not that often.

I was more pointing out that I had to use that environment inheritance mechanism so that child processes can be associated with their Git process ancestry. And so it is possible for someone to abuse that mechanism for other purposes (and introduce injections into what Josh is proposing).

I consider a process' SID to be the complete "a/b/c" string. But I do know that when I look at my logging data, that I will also find data for a process with SID of "a" and data for another process with SID "a/b".

So perhaps we should have names for:

    [1] "a/b/c"  -- my process' complete SID name
    [2] "c"      -- my process' SID component name
    [3] "a/b"    -- my parent's complete SID name

When analyzing Git perf data, there are times when you basically want to "SELECT * where SID startswith 'a/b/' ..." and summarize over the child processes of "a/b". So data from "a/b/c" and "a/b/d" would be aggregated. (I do have some of that data in the "child_exit" events reported for the "a/b" process, but sometimes you need to actually see the records for the child processes.)

So I guess I'm saying that the hierarchy has been useful and we should try to keep it as is.

Yes, it can be useful for external tools that drive Git to be able to set a SID prefix to help track that into Git process.

Likewise, it would be nice to add code to some of the Git shell script commands (such as git-mergetool and git-prompt) to augment the SID being passed to child Git commands to help track why they are being invoked.

Jeff

Documented in V3. For argument's sake, I'm going to say that the tokens should be limited to printable, non-whitespace characters, and should fit on a single pkt-line.

Document a new config option that allows users to determine whether or not to advertise their trace2 session IDs to remote Git clients and servers.

---
 Documentation/config/trace2.txt | 4 ++++
 1 file changed, 4 insertions(+)

index 01d3afd8a8..3f2e3b4425 100644
@@ -69,3 +69,7 @@ trace2.maxFiles::
 	write additional traces if we would exceed this many files. Instead,
 	write a sentinel file that will block further tracing to this
 	directory. Defaults to 0, which disables this check.
+
+trace2.advertiseSID::
+	Boolean. When true, client and server processes will advertise their
+	trace2 session IDs to their remote counterpart. Defaults to false.

2.29.1.341.ge80a0c044ae-goog

I do not think placeing this in the trace2 hierarchy is a good idea.

It is not like concept of "session" belongs to trace2; each operation we perform inherently is done on behalf of a session. The trace2 subsystem may have been the first to externalize the concept, but even after trace2 gets superseded, we would want to key our log records with some "session ID". After we introduce an improved mechanism that is successor to trace2, we still would want to exchange some session ID if the advertiseSID option the users define in their repository today (well, maybe in 3 months after this series lands in a released version and widely deployed), no?

We are not exposing the session ID anywhere but the transports, so how about calling it transport.advertiseSID, perhaps?

We also may want to call that just "session ID", not "trace2 session ID" in the description.

Thanks.

Yes this makes sense. Do you think it's worthwhile to move all the session ID implementation out of trace2? Right now there are some user-facing bits (environment variables for parent/child SID hierarchy) that specifically mention trace2, and I believe that the repo tool is using it to tie together logs produced by a single repo invocation.

Yeah, this sounds good to me. Will change in V3.

If somebody other than trace2 starts using session ID, or if we introduce a mechanism that allows a session ID assigned without enabling the rest of the trace2 machinery, such a separation may make sense at the implementation level, but until then, I do not think it is worth doing.

Hi Junio,

Yep, the closest we came to having another user of the session ID was Matthew DeVore's git xl effort, but that effort seems to have gone cold
(https://lore.kernel.org/git/[email protected]/#r is the latest message in that thread that I could find).

Ciao,
Dscho

Add a public wrapper, trace2_session_id(), around tr2_sid_get(), which is intended to be private trace2 implementation.

---
 trace2.c | 5 +++++
 trace2.h | 2 ++
 2 files changed, 7 insertions(+)

index 2c6b570077..256120c7fd 100644
@@ -792,3 +792,8 @@ void trace2_printf(const char *fmt, ...)
 	va_end(ap);
 }
 #endif
+
+const char *trace2_session_id(void)
+{
+	return tr2_sid_get();
+}

index b18bc5529c..ede18c2e06 100644
@@ -500,4 +500,6 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason);
 	} while (0)
 #endif
 
+const char *trace2_session_id(void);
+
 #endif /* TRACE2_H */

2.29.1.341.ge80a0c044ae-goog

When trace2 is enabled and trace2.advertiseSID is true, advertise upload-pack's trace2 session ID via the new trace2-sid capability.

---
 upload-pack.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

index 3b858eb457..3bb01c5427 100644
@@ -110,6 +110,7 @@ struct upload_pack_data {
 	unsigned done : 1;					/* v2 only */
 	unsigned allow_ref_in_want : 1;				/* v2 only */
 	unsigned allow_sideband_all : 1;			/* v2 only */
+	unsigned advertise_trace2_sid : 1;
 };
 
 static void upload_pack_data_init(struct upload_pack_data *data)

@@ -141,6 +142,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	packet_writer_init(&data->writer, 1);
 
 	data->keepalive = 5;
+	data->advertise_trace2_sid = 0;
 }
 
 static void upload_pack_data_clear(struct upload_pack_data *data)

@@ -1178,6 +1180,11 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)
 		strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util);
 }
 
+static void format_trace2_info(struct strbuf *buf, struct upload_pack_data *d) {
+	if (d->advertise_trace2_sid && trace2_is_enabled())
+		strbuf_addf(buf, " trace2-sid=%s", trace2_session_id());
+}
+
 static int send_ref(const char *refname, const struct object_id *oid,
 		    int flag, void *cb_data)
 {

@@ -1193,9 +1200,11 @@ static int send_ref(const char *refname, const struct object_id *oid,
 
 	if (capabilities) {
 		struct strbuf symref_info = STRBUF_INIT;
+		struct strbuf trace2_info = STRBUF_INIT;
 
 		format_symref_info(&symref_info, &data->symref);
-		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s object-format=%s agent=%s\n",
+		format_trace2_info(&trace2_info, data);
+		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n",
 			     oid_to_hex(oid), refname_nons,
 			     0, capabilities,
 			     (data->allow_uor & ALLOW_TIP_SHA1) ?

@@ -1205,9 +1214,11 @@ static int send_ref(const char *refname, const struct object_id *oid,
 			     data->stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     data->allow_filter ? " filter" : "",
+			     trace2_info.buf,
 			     the_hash_algo->name,
 			     git_user_agent_sanitized());
 		strbuf_release(&symref_info);
+		strbuf_release(&trace2_info);
 	} else {
 		packet_write_fmt(1, "%s %s\n", oid_to_hex(oid), refname_nons);
 	}

@@ -1299,6 +1310,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
 		data->allow_sideband_all = git_config_bool(var, value);
 	} else if (!strcmp("core.precomposeunicode", var)) {
 		precomposed_unicode = git_config_bool(var, value);
+	} else if (!strcmp("trace2.advertisesid", var)) {
+		data->advertise_trace2_sid = git_config_bool(var, value);
 	}
 
 	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&

2.29.1.341.ge80a0c044ae-goog

I would have imagined when advertiseSID is enabled, trace2, at least the part that allocates and assigns the session ID, ought to be enabled automatically.

But the above goes in a different direction and requires both to be enabled. Any compelling reason behind the choice?

Does the documentation added by this series make it clear that asking for advertiseSID does NOT automatically enable allocation of session IDs (even if it does not explain why it does not happen)?

Thanks.

My reasoning was that by advertising the capability, you are telling the remote side "I have definitely produced a log using this session ID. If you need it later, you can find it with this key". If we advertise a session ID even when trace2 is not enabled, the remote side can't be as sure that the received session ID actually points to any useful logs on the other side.

Of course, this is a weak guarantee since a client could send whatever it likes regardless of whether anything was logged, or one side could delete or lose its logs before the other decides it needs to view them.

I think your idea in a different subthread about having a general session ID not tied to trace2 is interesting, and would also be a point in favor of changing the current behavior here, but I have some thoughts on that point that I'll add in the other subthread.

I'm still leaning towards advertising a session ID only if we actually produced logs locally, but I'm open to further discussion.

In V3 I'll update the docs to call out whichever decision we reach on this point.

When trace2 is enabled and trace2.advertiseSID is true, advertise receive-pack's trace2 session ID via the new trace2-sid capability.

---
 builtin/receive-pack.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index bb9909c52e..2fe5a8b4fd 100644
@@ -54,6 +54,7 @@ static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int advertise_atomic_push = 1;
 static int advertise_push_options;
+static int advertise_trace2_sid;
 static int unpack_limit = 100;
 static off_t max_input_size;
 static int report_status;

@@ -248,6 +249,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "trace2.advertisesid") == 0) {
+		advertise_trace2_sid = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 

@@ -268,6 +274,8 @@ static void show_ref(const char *path, const struct object_id *oid)
 			strbuf_addf(&cap, " push-cert=%s", push_cert_nonce);
 		if (advertise_push_options)
 			strbuf_addstr(&cap, " push-options");
+		if (advertise_trace2_sid && trace2_is_enabled())
+			strbuf_addf(&cap, " trace2-sid=%s", trace2_session_id());
 		strbuf_addf(&cap, " object-format=%s", the_hash_algo->name);
 		strbuf_addf(&cap, " agent=%s", git_user_agent_sanitized());
 		packet_write_fmt(1, "%s %s%c%s\n",

2.29.1.341.ge80a0c044ae-goog

When trace2 is enabled and trace2.advertiseSID is true, advertise the server's trace2 session ID for all protocol v2 connections via the new trace2-sid capability.

---
 serve.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index f6341206c4..b40d7aad34 100644
@@ -8,6 +8,8 @@
 #include "serve.h"
 #include "upload-pack.h"
 
+static int advertise_trace2_sid;
+
 static int always_advertise(struct repository *r,
 			    struct strbuf *value)
 {

@@ -30,6 +32,15 @@ static int object_format_advertise(struct repository *r,
 	return 1;
 }
 
+static int trace2_advertise(struct repository *r, struct strbuf *value)
+{
+	if (!advertise_trace2_sid || !trace2_is_enabled())
+		return 0;
+	if (value)
+		strbuf_addstr(value, trace2_session_id());
+	return 1;
+}
+
 struct protocol_capability {
 	/*
 	 * The name of the capability.  The server uses this name when

@@ -66,6 +77,7 @@ static struct protocol_capability capabilities[] = {
 	{ "fetch", upload_pack_advertise, upload_pack_v2 },
 	{ "server-option", always_advertise, NULL },
 	{ "object-format", object_format_advertise, NULL },
+	{ "trace2-sid", trace2_advertise, NULL },
 };
 
 static void advertise_capabilities(void)

@@ -261,6 +273,8 @@ static int process_request(void)
 /* Main serve loop for protocol version 2 */
 void serve(struct serve_options *options)
 {
+	git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid);
+
 	if (options->advertise_capabilities || !options->stateless_rpc) {
 		/* serve by default supports v2 */
 		packet_write_fmt(1, "version 2\n");

2.29.1.341.ge80a0c044ae-goog

When a client receives a trace2-sid capability from a protocol v0, v1, or v2 server, log the received session ID via a trace2 data event.

---
 t/t5705-trace2-sid-in-capabilities.sh | 64 +++++++++++++++++++++++++++
 transport.c                           | 10 +++++
 2 files changed, 74 insertions(+)
 create mode 100755 t/t5705-trace2-sid-in-capabilities.sh

new file mode 100755
index 0000000000..0870e78f7c
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='trace2 SID in capabilities'
+
+. ./test-lib.sh
+
+REPO="$(pwd)/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for trace2 SID capability tests' '
+	git init "$REPO" &&
+	test_commit -C "$REPO" a &&
+	git clone "file://$REPO" "$LOCAL_PRISTINE" &&
+	test_commit -C "$REPO" b
+'
+
+for PROTO in 0 1 2
+do
+	test_expect_success "trace2 session IDs not advertised by default (fetch v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local fetch origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+	'
+
+	test_expect_success "trace2 session IDs not advertised by default (push v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		git -C local pull --no-rebase origin &&
+		GIT_TRACE2_EVENT_NESTING=5 \
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local push origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+	'
+done
+
+test_expect_success 'enable SID advertisement' '
+	git -C "$REPO" config trace2.advertiseSID true &&
+	git -C "$LOCAL_PRISTINE" config trace2.advertiseSID true
+'
+
+for PROTO in 0 1 2
+do
+	test_expect_success "trace2 session IDs advertised (fetch v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local fetch origin &&
+		grep \"key\":\"server-sid\" tr2-client-events
+	'
+
+	test_expect_success "trace2 session IDs advertised (push v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		git -C local pull --no-rebase origin &&
+		GIT_TRACE2_EVENT_NESTING=5 \
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local push origin &&
+		grep \"key\":\"server-sid\" tr2-client-events
+	'
+done
+
+test_done

index 47da955e4f..d16be597bd 100644
@@ -286,6 +286,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
 	struct git_transport_data *data = transport->data;
 	struct ref *refs = NULL;
 	struct packet_reader reader;
+	int sid_len;
+	const char *server_trace2_sid;
 
 	connect_setup(transport, for_push);
 

@@ -297,6 +299,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
 	data->version = discover_version(&reader);
 	switch (data->version) {
 	case protocol_v2:
+		if (server_feature_v2("trace2-sid", &server_trace2_sid))
+			trace2_data_string("trace2", NULL, "server-sid", server_trace2_sid);
 		if (must_list_refs)
 			get_remote_refs(data->fd[1], &reader, &refs, for_push,
 					ref_prefixes,

@@ -310,6 +314,12 @@ static struct ref *handshake(struct transport *transport, int for_push,
 				 for_push ? REF_NORMAL : 0,
 				 &data->extra_have,
 				 &data->shallow);
+		server_trace2_sid = server_feature_value("trace2-sid", &sid_len);
+		if (server_trace2_sid) {
+			char *server_sid = xstrndup(server_trace2_sid, sid_len);
+			trace2_data_string("trace2", NULL, "server-sid", server_sid);
+			free(server_sid);
+		}
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");

2.29.1.341.ge80a0c044ae-goog

Would this pose a new security threat surface? Just wondering if we want to ignore the capability if it is not enabled on our end with the configuration.

Thanks.

As Jeff pointed out, the json-writer handles string escapes, so I don't think we could cause any trouble with the trace2 "event" target. The "normal" target ignores data events, so this would not show up in a normal trace2 log. The "perf" target doesn't seem to do any escaping, but it's intended to be human readable rather than parseable, so I'm not sure if we need to worry there. Jeff, any thoughts?

Only the "event" target prints the SID and it is JSON quoted there.

Neither the "perf" nor "normal" target print them. The "perf" target does print the SID "depth" parameter (which is the number of slashes in the complete SID).

My earlier concerns were about whitespace, CL/LF and other non-printables in the wire protocol and etc.

Jeff

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.

---
 fetch-pack.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

index b10c432315..7fbefa7b8e 100644
@@ -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;
 

2.29.1.341.ge80a0c044ae-goog

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.

When upload-pack (protocol v0/v1) or a protocol v2 server receives a trace2-sid capability from a client, log the received session ID via a trace2 data event.

---
 serve.c                               |  4 ++++
 t/t5705-trace2-sid-in-capabilities.sh | 19 +++++++++++++------
 upload-pack.c                         |  7 +++++++
 3 files changed, 24 insertions(+), 6 deletions(-)

index b40d7aad34..ffe5ba3f8a 100644
@@ -201,6 +201,7 @@ static int process_request(void)
 	struct packet_reader reader;
 	struct strvec keys = STRVEC_INIT;
 	struct protocol_capability *command = NULL;
+	const char *client_sid;
 
 	packet_reader_init(&reader, 0, NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |

@@ -264,6 +265,9 @@ static int process_request(void)
 
 	check_algorithm(the_repository, &keys);
 
+	if (has_capability(&keys, "trace2-sid", &client_sid))
+		trace2_data_string("trace2", NULL, "client-sid", client_sid);
+
 	command->command(the_repository, &keys, &reader);
 
 	strvec_clear(&keys);

index 0870e78f7c..3fad9462d3 100755
@@ -17,11 +17,14 @@ test_expect_success 'setup repos for trace2 SID capability tests' '
 for PROTO in 0 1 2
 do
 	test_expect_success "trace2 session IDs not advertised by default (fetch v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local fetch origin &&
-		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+		git -c protocol.version=$PROTO -C local fetch \
+			--upload-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-upload-pack" \
+			origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)" &&
+		test -z "$(grep \"key\":\"client-sid\" tr2-server-events)"
 	'
 
 	test_expect_success "trace2 session IDs not advertised by default (push v${PROTO})" '

@@ -43,11 +46,15 @@ test_expect_success 'enable SID advertisement' '
 for PROTO in 0 1 2
 do
 	test_expect_success "trace2 session IDs advertised (fetch v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
 		cp -r "$LOCAL_PRISTINE" local &&
+		git -C local config trace2.advertiseSID true &&
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local fetch origin &&
-		grep \"key\":\"server-sid\" tr2-client-events
+		git -c protocol.version=$PROTO -C local fetch \
+			--upload-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-upload-pack" \
+			origin &&
+		grep \"key\":\"server-sid\" tr2-client-events &&
+		grep \"key\":\"client-sid\" tr2-server-events
 	'
 
 	test_expect_success "trace2 session IDs advertised (push v${PROTO})" '

index 3bb01c5427..d938603c97 100644
@@ -1058,6 +1058,7 @@ static void receive_needs(struct upload_pack_data *data,
 		const char *features;
 		struct object_id oid_buf;
 		const char *arg;
+		int feature_len;
 
 		reset_timeout(data->timeout);
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL)

@@ -1109,6 +1110,12 @@ static void receive_needs(struct upload_pack_data *data,
 		if (data->allow_filter &&
 		    parse_feature_request(features, "filter"))
 			data->filter_capability_requested = 1;
+		if ((arg = parse_feature_value(features, "trace2-sid", &feature_len, NULL)))
+		{
+			char *client_sid = xstrndup(arg, feature_len);
+			trace2_data_string("trace2", NULL, "client-sid", client_sid);
+			free(client_sid);
+		}
 
 		o = parse_object(the_repository, &oid_buf);
 		if (!o) {

2.29.1.341.ge80a0c044ae-goog

Will fix in V3.

When trace2 is enabled, the server sent a trace2-sid capability, and trace2.advertiseSID is true, advertise send-pack's own session ID back to the server.

---
 send-pack.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index eb4a44270b..7478a3f7cc 100644
@@ -424,6 +424,8 @@ int send_pack(struct send_pack_args *args,
 	int use_sideband = 0;
 	int quiet_supported = 0;
 	int agent_supported = 0;
+	int server_sent_trace2_sid = 0;
+	int advertise_trace2_sid = 0;
 	int use_atomic = 0;
 	int atomic_supported = 0;
 	int use_push_options = 0;

@@ -435,6 +437,8 @@ int send_pack(struct send_pack_args *args,
 	const char *push_cert_nonce = NULL;
 	struct packet_reader reader;
 
+	git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid);
+
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status-v2"))
 		status_report = 2;

@@ -450,6 +454,8 @@ int send_pack(struct send_pack_args *args,
 		quiet_supported = 1;
 	if (server_supports("agent"))
 		agent_supported = 1;
+	if (server_supports("trace2-sid"))
+		server_sent_trace2_sid = 1;
 	if (server_supports("no-thin"))
 		args->use_thin_pack = 0;
 	if (server_supports("atomic"))

@@ -506,6 +512,8 @@ int send_pack(struct send_pack_args *args,
 		strbuf_addf(&cap_buf, " object-format=%s", the_hash_algo->name);
 	if (agent_supported)
 		strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
+	if (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())
+		strbuf_addf(&cap_buf, " trace2-sid=%s", trace2_session_id());
 
 	/*
 	 * NEEDSWORK: why does delete-refs have to be so specific to

2.29.1.341.ge80a0c044ae-goog

When receive-pack receives a trace2-sid capability from the client, log the received session ID via a trace2 data event.

---
 builtin/receive-pack.c                |  7 +++++++
 t/t5705-trace2-sid-in-capabilities.sh | 20 ++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

index 2fe5a8b4fd..f780a66a52 100644
@@ -2039,6 +2039,7 @@ static struct command *read_head_info(struct packet_reader *reader,
 		if (linelen < reader->pktlen) {
 			const char *feature_list = reader->line + linelen + 1;
 			const char *hash = NULL;
+			const char *client_trace2_sid;
 			int len = 0;
 			if (parse_feature_request(feature_list, "report-status"))
 				report_status = 1;

@@ -2061,6 +2062,12 @@ static struct command *read_head_info(struct packet_reader *reader,
 			}
 			if (xstrncmpz(the_hash_algo->name, hash, len))
 				die("error: unsupported object format '%s'", hash);
+			client_trace2_sid = parse_feature_value(feature_list, "trace2-sid", &len, NULL);
+			if (client_trace2_sid) {
+				char *client_sid = xstrndup(client_trace2_sid, len);
+				trace2_data_string("trace2", NULL, "client-sid", client_sid);
+				free(client_sid);
+			}
 		}
 
 		if (!strcmp(reader->line, "push-cert")) {

index 3fad9462d3..15f78cb7d2 100755
@@ -28,13 +28,17 @@ do
 	'
 
 	test_expect_success "trace2 session IDs not advertised by default (push v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
+		test_when_finished "git -C local push --delete origin new-branch" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		git -C local pull --no-rebase origin &&
 		GIT_TRACE2_EVENT_NESTING=5 \
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local push origin &&
-		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+		git -c protocol.version=$PROTO -C local push \
+			--receive-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-receive-pack" \
+			origin HEAD:new-branch &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)" &&
+		test -z "$(grep \"key\":\"client-sid\" tr2-server-events)"
 	'
 done
 

@@ -58,13 +62,17 @@ do
 	'
 
 	test_expect_success "trace2 session IDs advertised (push v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
+		test_when_finished "git -C local push --delete origin new-branch" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		git -C local pull --no-rebase origin &&
 		GIT_TRACE2_EVENT_NESTING=5 \
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local push origin &&
-		grep \"key\":\"server-sid\" tr2-client-events
+		git -c protocol.version=$PROTO -C local push \
+			--receive-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-receive-pack" \
+			origin HEAD:new-branch &&
+		grep \"key\":\"server-sid\" tr2-client-events &&
+		grep \"key\":\"client-sid\" tr2-server-events
 	'
 done
 

2.29.1.341.ge80a0c044ae-goog

Thanks; will replace.

In order to more easily debug remote operations, it is useful to be able to identify sessions on either side of the connection. This series allows clients and servers to provide a unique session ID via a new "session-id" protocol capability. This session ID can be logged on each side of the connection, allowing logs to be joined during debugging sessions.

Changes from V2:
* De-emphasize connection to trace2
* Renamed capability from "trace2-sid" to "session-id"
* Noted (lack of) session ID structure in capability docs
* Advertise SID regardless of whether trace2 is enabled
* Simplify conditionals
* Style cleanups

Changes since V1:
* Added a public trace2_session_id() function to trace2.{h,c}. Used this in place of tr2_sid_get().
* Fixed a style issue regarding using NULL rather than 0.

Josh Steadmon (11):
docs: new capability to advertise session IDs
docs: new transfer.advertiseSID option
trace2: add a public function for getting the SID
upload-pack: advertise session ID in v0 capabilities
receive-pack: advertise session ID in v0 capabilities
serve: advertise session ID in v2 capabilities
transport: log received server session ID
fetch-pack: advertise session ID in capabilities upload-pack, serve: log received client session ID
send-pack: advertise session ID in capabilities
receive-pack: log received client session ID

 Documentation/config/transfer.txt             |  4 +
 .../technical/protocol-capabilities.txt       | 17 +++-
 Documentation/technical/protocol-v2.txt       | 13 ++++
 builtin/receive-pack.c                        | 15 ++++
 fetch-pack.c                                  |  9 +++
 send-pack.c                                   |  7 ++
 serve.c                                       | 18 +++++
 t/t5705-session-id-in-capabilities.sh         | 78 +++++++++++++++++++
 trace2.c                                      |  5 ++
 trace2.h                                      |  2 +
 transport.c                                   | 10 +++
 upload-pack.c                                 | 23 +++++-
 12 files changed, 198 insertions(+), 3 deletions(-)
 create mode 100755 t/t5705-session-id-in-capabilities.sh

Range-diff against v2:

 1:  d04028c3c7 <  -:  ---------- docs: new capability to advertise trace2 SIDs
 2:  5d5097b671 <  -:  ---------- docs: new trace2.advertiseSID option
 -:  ---------- >  1:  184cabb6f5 docs: new capability to advertise session IDs
 -:  ---------- >  2:  937534065a docs: new transfer.advertiseSID option
 3:  7f42aacd2b =  3:  227c709ba5 trace2: add a public function for getting the SID
 4:  4912af5f2b !  4:  612957b9d5 upload-pack: advertise trace2 SID in v0 capabilities
    @@ Metadata
     Author: Josh Steadmon <[email protected]>
     
      ## Commit message ##
    -    upload-pack: advertise trace2 SID in v0 capabilities
    +    upload-pack: advertise session ID in v0 capabilities
     
    -    When trace2 is enabled and trace2.advertiseSID is true, advertise
    -    upload-pack's trace2 session ID via the new trace2-sid capability.
    +    When transfer.advertiseSID is true, advertise upload-pack's session ID
    +    via the new session-id capability.
     
     
    @@ upload-pack.c: struct upload_pack_data {
      	unsigned done : 1;					/* v2 only */
      	unsigned allow_ref_in_want : 1;				/* v2 only */
      	unsigned allow_sideband_all : 1;			/* v2 only */
    -+	unsigned advertise_trace2_sid : 1;
    ++	unsigned advertise_sid : 1;
      };
      
      static void upload_pack_data_init(struct upload_pack_data *data)
    @@ upload-pack.c: static void upload_pack_data_init(struct upload_pack_data *data)
      	packet_writer_init(&data->writer, 1);
      
      	data->keepalive = 5;
    -+	data->advertise_trace2_sid = 0;
    ++	data->advertise_sid = 0;
      }
      
      static void upload_pack_data_clear(struct upload_pack_data *data)
    @@ upload-pack.c: static void format_symref_info(struct strbuf *buf, struct string_
      		strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util);
      }
      
    -+static void format_trace2_info(struct strbuf *buf, struct upload_pack_data *d) {
    -+	if (d->advertise_trace2_sid && trace2_is_enabled())
    -+		strbuf_addf(buf, " trace2-sid=%s", trace2_session_id());
    ++static void format_session_id(struct strbuf *buf, struct upload_pack_data *d) {
    ++	if (d->advertise_sid)
    ++		strbuf_addf(buf, " session-id=%s", trace2_session_id());
     +}
     +
      static int send_ref(const char *refname, const struct object_id *oid,
    @@ upload-pack.c: static int send_ref(const char *refname, const struct object_id *
      
      	if (capabilities) {
      		struct strbuf symref_info = STRBUF_INIT;
    -+		struct strbuf trace2_info = STRBUF_INIT;
    ++		struct strbuf session_id = STRBUF_INIT;
      
      		format_symref_info(&symref_info, &data->symref);
     -		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s object-format=%s agent=%s\n",
    -+		format_trace2_info(&trace2_info, data);
    ++		format_session_id(&session_id, data);
     +		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n",
      			     oid_to_hex(oid), refname_nons,
      			     0, capabilities,
    @@ upload-pack.c: static int send_ref(const char *refname, const struct object_id *
      			     data->stateless_rpc ? " no-done" : "",
      			     symref_info.buf,
      			     data->allow_filter ? " filter" : "",
    -+			     trace2_info.buf,
    ++			     session_id.buf,
      			     the_hash_algo->name,
      			     git_user_agent_sanitized());
      		strbuf_release(&symref_info);
    -+		strbuf_release(&trace2_info);
    ++		strbuf_release(&session_id);
      	} else {
      		packet_write_fmt(1, "%s %s\n", oid_to_hex(oid), refname_nons);
      	}
    @@ upload-pack.c: static int upload_pack_config(const char *var, const char *value,
      		data->allow_sideband_all = git_config_bool(var, value);
      	} else if (!strcmp("core.precomposeunicode", var)) {
      		precomposed_unicode = git_config_bool(var, value);
    -+	} else if (!strcmp("trace2.advertisesid", var)) {
    -+		data->advertise_trace2_sid = git_config_bool(var, value);
    ++	} else if (!strcmp("transfer.advertisesid", var)) {
    ++		data->advertise_sid = git_config_bool(var, value);
      	}
      
      	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
 5:  7003189c81 !  5:  32fe78f3e9 receive-pack: advertise trace2 SID in v0 capabilities
    @@ Metadata
     Author: Josh Steadmon <[email protected]>
     
      ## Commit message ##
    -    receive-pack: advertise trace2 SID in v0 capabilities
    +    receive-pack: advertise session ID in v0 capabilities
     
    -    When trace2 is enabled and trace2.advertiseSID is true, advertise
    -    receive-pack's trace2 session ID via the new trace2-sid capability.
    +    When transfer.advertiseSID is true, advertise receive-pack's session ID
    +    via the new session-id capability.
     
     
    @@ builtin/receive-pack.c: static int receive_unpack_limit = -1;
      static int transfer_unpack_limit = -1;
      static int advertise_atomic_push = 1;
      static int advertise_push_options;
    -+static int advertise_trace2_sid;
    ++static int advertise_sid;
      static int unpack_limit = 100;
      static off_t max_input_size;
      static int report_status;
    @@ builtin/receive-pack.c: static int receive_pack_config(const char *var, const ch
      		return 0;
      	}
      
    -+	if (strcmp(var, "trace2.advertisesid") == 0) {
    -+		advertise_trace2_sid = git_config_bool(var, value);
    ++	if (strcmp(var, "transfer.advertisesid") == 0) {
    ++		advertise_sid = git_config_bool(var, value);
     +		return 0;
     +	}
     +
    @@ builtin/receive-pack.c: static void show_ref(const char *path, const struct obje
      			strbuf_addf(&cap, " push-cert=%s", push_cert_nonce);
      		if (advertise_push_options)
      			strbuf_addstr(&cap, " push-options");
    -+		if (advertise_trace2_sid && trace2_is_enabled())
    -+			strbuf_addf(&cap, " trace2-sid=%s", trace2_session_id());
    ++		if (advertise_sid)
    ++			strbuf_addf(&cap, " session-id=%s", trace2_session_id());
      		strbuf_addf(&cap, " object-format=%s", the_hash_algo->name);
      		strbuf_addf(&cap, " agent=%s", git_user_agent_sanitized());
      		packet_write_fmt(1, "%s %s%c%s\n",
 6:  9573428cc0 !  6:  fe6731cc09 serve: advertise trace2 SID in v2 capabilities
    @@ Metadata
     Author: Josh Steadmon <[email protected]>
     
      ## Commit message ##
    -    serve: advertise trace2 SID in v2 capabilities
    +    serve: advertise session ID in v2 capabilities
     
    -    When trace2 is enabled and trace2.advertiseSID is true, advertise the
    -    server's trace2 session ID for all protocol v2 connections via the new
    -    trace2-sid capability.
    +    When transfer.advertiseSID is true, advertise the server's session ID
    +    for all protocol v2 connections via the new session-id capability.
     
     
    @@ serve.c
      #include "serve.h"
      #include "upload-pack.h"
      
    -+static int advertise_trace2_sid;
    ++static int advertise_sid;
     +
      static int always_advertise(struct repository *r,
      			    struct strbuf *value)
    @@ serve.c: static int object_format_advertise(struct repository *r,
      	return 1;
      }
      
    -+static int trace2_advertise(struct repository *r, struct strbuf *value)
    ++static int session_id_advertise(struct repository *r, struct strbuf *value)
     +{
    -+	if (!advertise_trace2_sid || !trace2_is_enabled())
    ++	if (!advertise_sid)
     +		return 0;
     +	if (value)
     +		strbuf_addstr(value, trace2_session_id());
    @@ serve.c: static struct protocol_capability capabilities[] = {
      	{ "fetch", upload_pack_advertise, upload_pack_v2 },
      	{ "server-option", always_advertise, NULL },
      	{ "object-format", object_format_advertise, NULL },
    -+	{ "trace2-sid", trace2_advertise, NULL },
    ++	{ "session-id", session_id_advertise, NULL },
      };
      
      static void advertise_capabilities(void)
    @@ serve.c: static int process_request(void)
      /* Main serve loop for protocol version 2 */
      void serve(struct serve_options *options)
      {
    -+	git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid);
    ++	git_config_get_bool("transfer.advertisesid", &advertise_sid);
     +
      	if (options->advertise_capabilities || !options->stateless_rpc) {
      		/* serve by default supports v2 */
 7:  21bdbf23f3 !  7:  014cae8dc1 transport: log received server trace2 SID
    @@ Metadata
     Author: Josh Steadmon <[email protected]>
     
      ## Commit message ##
    -    transport: log received server trace2 SID
    +    transport: log received server session ID
     
    -    When a client receives a trace2-sid capability from a protocol v0, v1,
    +    When a client receives a session-id capability from a protocol v0, v1,
         or v2 server, log the received session ID via a trace2 data event.
     
     
    - ## t/t5705-trace2-sid-in-capabilities.sh (new) ##
    + ## t/t5705-session-id-in-capabilities.sh (new) ##
     @@
     +#!/bin/sh
     +
    -+test_description='trace2 SID in capabilities'
    ++test_description='session ID in capabilities'
     +
     +. ./test-lib.sh
     +
     +REPO="$(pwd)/repo"
     +LOCAL_PRISTINE="$(pwd)/local_pristine"
     +
    -+test_expect_success 'setup repos for trace2 SID capability tests' '
    ++test_expect_success 'setup repos for session ID capability tests' '
     +	git init "$REPO" &&
     +	test_commit -C "$REPO" a &&
     +	git clone "file://$REPO" "$LOCAL_PRISTINE" &&
    @@ t/t5705-trace2-sid-in-capabilities.sh (new)
     +
     +for PROTO in 0 1 2
     +do
    -+	test_expect_success "trace2 session IDs not advertised by default (fetch v${PROTO})" '
    ++	test_expect_success "session IDs not advertised by default (fetch v${PROTO})" '
     +		test_when_finished "rm -rf local tr2-client-events" &&
     +		cp -r "$LOCAL_PRISTINE" local &&
     +		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
    @@ t/t5705-trace2-sid-in-capabilities.sh (new)
     +		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
     +	'
     +
    -+	test_expect_success "trace2 session IDs not advertised by default (push v${PROTO})" '
    ++	test_expect_success "session IDs not advertised by default (push v${PROTO})" '
     +		test_when_finished "rm -rf local tr2-client-events" &&
     +		cp -r "$LOCAL_PRISTINE" local &&
     +		git -C local pull --no-rebase origin &&
    @@ t/t5705-trace2-sid-in-capabilities.sh (new)
     +done
     +
     +test_expect_success 'enable SID advertisement' '
    -+	git -C "$REPO" config trace2.advertiseSID true &&
    -+	git -C "$LOCAL_PRISTINE" config trace2.advertiseSID true
    ++	git -C "$REPO" config transfer.advertiseSID true &&
    ++	git -C "$LOCAL_PRISTINE" config transfer.advertiseSID true
     +'
     +
     +for PROTO in 0 1 2
     +do
    -+	test_expect_success "trace2 session IDs advertised (fetch v${PROTO})" '
    ++	test_expect_success "session IDs advertised (fetch v${PROTO})" '
     +		test_when_finished "rm -rf local tr2-client-events" &&
     +		cp -r "$LOCAL_PRISTINE" local &&
     +		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
    @@ t/t5705-trace2-sid-in-capabilities.sh (new)
     +		grep \"key\":\"server-sid\" tr2-client-events
     +	'
     +
    -+	test_expect_success "trace2 session IDs advertised (push v${PROTO})" '
    ++	test_expect_success "session IDs advertised (push v${PROTO})" '
     +		test_when_finished "rm -rf local tr2-client-events" &&
     +		cp -r "$LOCAL_PRISTINE" local &&
     +		git -C local pull --no-rebase origin &&
    @@ transport.c: static struct ref *handshake(struct transport *transport, int for_p
      	struct ref *refs = NULL;
      	struct packet_reader reader;
     +	int sid_len;
    -+	const char *server_trace2_sid;
    ++	const char *server_sid;
      
      	connect_setup(transport, for_push);
      
    @@ transport.c: static struct ref *handshake(struct transport *transport, int for_p
      	data->version = discover_version(&reader);
      	switch (data->version) {
      	case protocol_v2:
    -+		if (server_feature_v2("trace2-sid", &server_trace2_sid))
    -+			trace2_data_string("trace2", NULL, "server-sid", server_trace2_sid);
    ++		if (server_feature_v2("session-id", &server_sid))
    ++			trace2_data_string("transfer", NULL, "server-sid", server_sid);
      		if (must_list_refs)
      			get_remote_refs(data->fd[1], &reader, &refs, for_push,
      					ref_prefixes,
    @@ transport.c: static struct ref *handshake(struct transport *transport, int for_p
      				 for_push ? REF_NORMAL : 0,
      				 &data->extra_have,
      				 &data->shallow);
    -+		server_trace2_sid = server_feature_value("trace2-sid", &sid_len);
    -+		if (server_trace2_sid) {
    -+			char *server_sid = xstrndup(server_trace2_sid, sid_len);
    -+			trace2_data_string("trace2", NULL, "server-sid", server_sid);
    -+			free(server_sid);
    ++		server_sid = server_feature_value("session-id", &sid_len);
    ++		if (server_sid) {
    ++			char *sid = xstrndup(server_sid, sid_len);
    ++			trace2_data_string("transfer", NULL, "server-sid", sid);
    ++			free(sid);
     +		}
      		break;
      	case protocol_unknown_version:
 8:  11b5b1b54f !  8:  fc9f6c9286 fetch-pack: advertise trace2 SID in capabilities
    @@ Metadata
     Author: Josh Steadmon <[email protected]>
     
      ## Commit message ##
    -    fetch-pack: advertise trace2 SID in capabilities
    +    fetch-pack: advertise session ID in capabilities
     
    -    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.
    +    When the server sent a session-id capability and transfer.advertiseSID
    +    is true, advertise fetch-pack's own session ID back to the server.
     
     
    @@ fetch-pack.c: 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 int advertise_sid;
      static struct shallow_lock shallow_lock;
      static const char *alternate_shallow_file;
      static struct strbuf fsck_msg_types = STRBUF_INIT;
    @@ fetch-pack.c: 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 (advertise_sid)
    ++				strbuf_addf(&c, " session-id=%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);
    @@ fetch-pack.c: 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("session-id"))
    ++		advertise_sid = 0;
     +
      	if (server_supports("shallow"))
      		print_verbose(args, _("Server supports %s"), "shallow");
    @@ fetch-pack.c: static int send_fetch_request(struct fetch_negotiator *negotiator,
      		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 (advertise_sid && server_supports_v2("session-id", 0))
    ++		packet_buf_write(&req_buf, "session-id=%s", trace2_session_id());
      	if (args->server_options && args->server_options->nr &&
      	    server_supports_v2("server-option", 1)) {
      		int i;
    @@ fetch-pack.c: 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);
    ++	git_config_get_bool("transfer.advertisesid", &advertise_sid);
      	if (!uri_protocols.nr) {
      		char *str;

9: 23f44bc904 ! 9: bde9c1d97a upload-pack, serve: log received client trace2 SID

    @@ Metadata
     Author: Josh Steadmon <[email protected]>
     
      ## Commit message ##
    -    upload-pack, serve: log received client trace2 SID
    +    upload-pack, serve: log received client session ID
     
         When upload-pack (protocol v0/v1) or a protocol v2 server receives a
    -    trace2-sid capability from a client, log the received session ID via a
    +    session-id capability from a client, log the received session ID via a
         trace2 data event.
     
    @@ serve.c: static int process_request(void)
      
      	check_algorithm(the_repository, &keys);
      
    -+	if (has_capability(&keys, "trace2-sid", &client_sid))
    -+		trace2_data_string("trace2", NULL, "client-sid", client_sid);
    ++	if (has_capability(&keys, "session-id", &client_sid))
    ++		trace2_data_string("transfer", NULL, "client-sid", client_sid);
     +
      	command->command(the_repository, &keys, &reader);
      
      	strvec_clear(&keys);
     
    - ## t/t5705-trace2-sid-in-capabilities.sh ##
    -@@ t/t5705-trace2-sid-in-capabilities.sh: test_expect_success 'setup repos for trace2 SID capability tests' '
    + ## t/t5705-session-id-in-capabilities.sh ##
    +@@ t/t5705-session-id-in-capabilities.sh: test_expect_success 'setup repos for session ID capability tests' '
      for PROTO in 0 1 2
      do
    - 	test_expect_success "trace2 session IDs not advertised by default (fetch v${PROTO})" '
    + 	test_expect_success "session IDs not advertised by default (fetch v${PROTO})" '
     -		test_when_finished "rm -rf local tr2-client-events" &&
     +		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
      		cp -r "$LOCAL_PRISTINE" local &&
    @@ t/t5705-trace2-sid-in-capabilities.sh: test_expect_success 'setup repos for trac
     +		test -z "$(grep \"key\":\"client-sid\" tr2-server-events)"
      	'
      
    - 	test_expect_success "trace2 session IDs not advertised by default (push v${PROTO})" '
    -@@ t/t5705-trace2-sid-in-capabilities.sh: test_expect_success 'enable SID advertisement' '
    + 	test_expect_success "session IDs not advertised by default (push v${PROTO})" '
    +@@ t/t5705-session-id-in-capabilities.sh: test_expect_success 'enable SID advertisement' '
      for PROTO in 0 1 2
      do
    - 	test_expect_success "trace2 session IDs advertised (fetch v${PROTO})" '
    + 	test_expect_success "session IDs advertised (fetch v${PROTO})" '
     -		test_when_finished "rm -rf local tr2-client-events" &&
     +		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
      		cp -r "$LOCAL_PRISTINE" local &&
    -+		git -C local config trace2.advertiseSID true &&
      		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
     -		git -c protocol.version=$PROTO -C local fetch origin &&
     -		grep \"key\":\"server-sid\" tr2-client-events
    @@ t/t5705-trace2-sid-in-capabilities.sh: test_expect_success 'enable SID advertise
     +		grep \"key\":\"client-sid\" tr2-server-events
      	'
      
    - 	test_expect_success "trace2 session IDs advertised (push v${PROTO})" '
    + 	test_expect_success "session IDs advertised (push v${PROTO})" '
     
      ## upload-pack.c ##
     @@ upload-pack.c: static void receive_needs(struct upload_pack_data *data,
    @@ upload-pack.c: static void receive_needs(struct upload_pack_data *data,
      		reset_timeout(data->timeout);
      		if (packet_reader_read(reader) != PACKET_READ_NORMAL)
     @@ upload-pack.c: static void receive_needs(struct upload_pack_data *data,
    - 		if (data->allow_filter &&
      		    parse_feature_request(features, "filter"))
      			data->filter_capability_requested = 1;
    -+		if ((arg = parse_feature_value(features, "trace2-sid", &feature_len, NULL)))
    -+		{
    + 
    ++		arg = parse_feature_value(features, "session-id", &feature_len, NULL);
    ++		if (arg) {
     +			char *client_sid = xstrndup(arg, feature_len);
    -+			trace2_data_string("trace2", NULL, "client-sid", client_sid);
    ++			trace2_data_string("transfer", NULL, "client-sid", client_sid);
     +			free(client_sid);
     +		}
    - 
    ++
      		o = parse_object(the_repository, &oid_buf);
      		if (!o) {
    + 			packet_writer_error(&data->writer,
10:  c0b1ffc6d8 ! 10:  012949e7da send-pack: advertise trace2 SID in capabilities
    @@ Metadata
     Author: Josh Steadmon <[email protected]>
     
      ## Commit message ##
    -    send-pack: advertise trace2 SID in capabilities
    +    send-pack: advertise session ID in capabilities
     
    -    When trace2 is enabled, the server sent a trace2-sid capability, and
    -    trace2.advertiseSID is true, advertise send-pack's own session ID back
    -    to the server.
    +    When the server sent a session-id capability and transfer.advertiseSID
    +    is true, advertise send-pack's own session ID back to the server.
     
     
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      	int use_sideband = 0;
      	int quiet_supported = 0;
      	int agent_supported = 0;
    -+	int server_sent_trace2_sid = 0;
    -+	int advertise_trace2_sid = 0;
    ++	int advertise_sid = 0;
      	int use_atomic = 0;
      	int atomic_supported = 0;
      	int use_push_options = 0;
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      	const char *push_cert_nonce = NULL;
      	struct packet_reader reader;
      
    -+	git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid);
    ++	git_config_get_bool("transfer.advertisesid", &advertise_sid);
     +
      	/* Does the other end support the reporting? */
      	if (server_supports("report-status-v2"))
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      		quiet_supported = 1;
      	if (server_supports("agent"))
      		agent_supported = 1;
    -+	if (server_supports("trace2-sid"))
    -+		server_sent_trace2_sid = 1;
    ++	if (!server_supports("session-id"))
    ++		advertise_sid = 0;
      	if (server_supports("no-thin"))
      		args->use_thin_pack = 0;
      	if (server_supports("atomic"))
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      		strbuf_addf(&cap_buf, " object-format=%s", the_hash_algo->name);
      	if (agent_supported)
      		strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
    -+	if (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())
    -+		strbuf_addf(&cap_buf, " trace2-sid=%s", trace2_session_id());
    ++	if (advertise_sid)
    ++		strbuf_addf(&cap_buf, " session-id=%s", trace2_session_id());
      
      	/*
      	 * NEEDSWORK: why does delete-refs have to be so specific to
11:  c47eddd9df ! 11:  ea2a318f2b receive-pack: log received client trace2 SID
    @@ Metadata
     Author: Josh Steadmon <[email protected]>
     
      ## Commit message ##
    -    receive-pack: log received client trace2 SID
    +    receive-pack: log received client session ID
     
    -    When receive-pack receives a trace2-sid capability from the client, log
    +    When receive-pack receives a session-id capability from the client, log
         the received session ID via a trace2 data event.
     
    @@ builtin/receive-pack.c: static struct command *read_head_info(struct packet_read
      		if (linelen < reader->pktlen) {
      			const char *feature_list = reader->line + linelen + 1;
      			const char *hash = NULL;
    -+			const char *client_trace2_sid;
    ++			const char *client_sid;
      			int len = 0;
      			if (parse_feature_request(feature_list, "report-status"))
      				report_status = 1;
    @@ builtin/receive-pack.c: static struct command *read_head_info(struct packet_read
      			}
      			if (xstrncmpz(the_hash_algo->name, hash, len))
      				die("error: unsupported object format '%s'", hash);
    -+			client_trace2_sid = parse_feature_value(feature_list, "trace2-sid", &len, NULL);
    -+			if (client_trace2_sid) {
    -+				char *client_sid = xstrndup(client_trace2_sid, len);
    -+				trace2_data_string("trace2", NULL, "client-sid", client_sid);
    -+				free(client_sid);
    ++			client_sid = parse_feature_value(feature_list, "session-id", &len, NULL);
    ++			if (client_sid) {
    ++				char *sid = xstrndup(client_sid, len);
    ++				trace2_data_string("transfer", NULL, "client-sid", client_sid);
    ++				free(sid);
     +			}
      		}
      
      		if (!strcmp(reader->line, "push-cert")) {
     
    - ## t/t5705-trace2-sid-in-capabilities.sh ##
    -@@ t/t5705-trace2-sid-in-capabilities.sh: do
    + ## t/t5705-session-id-in-capabilities.sh ##
    +@@ t/t5705-session-id-in-capabilities.sh: do
      	'
      
    - 	test_expect_success "trace2 session IDs not advertised by default (push v${PROTO})" '
    + 	test_expect_success "session IDs not advertised by default (push v${PROTO})" '
     -		test_when_finished "rm -rf local tr2-client-events" &&
     +		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
     +		test_when_finished "git -C local push --delete origin new-branch" &&
    @@ t/t5705-trace2-sid-in-capabilities.sh: do
      	'
      done
      
    -@@ t/t5705-trace2-sid-in-capabilities.sh: do
    +@@ t/t5705-session-id-in-capabilities.sh: do
      	'
      
    - 	test_expect_success "trace2 session IDs advertised (push v${PROTO})" '
    + 	test_expect_success "session IDs advertised (push v${PROTO})" '
     -		test_when_finished "rm -rf local tr2-client-events" &&
     +		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
     +		test_when_finished "git -C local push --delete origin new-branch" &&

2.29.2.222.g5d2a92d10f8-goog

In future patches, we will add the ability for Git servers and clients to advertise unique session IDs via protocol capabilities. This allows for easier debugging when both client and server logs are available.

---
 .../technical/protocol-capabilities.txt         | 17 +++++++++++++++--
 Documentation/technical/protocol-v2.txt         | 13 +++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

index ba869a7d36..9dfade930d 100644
@@ -27,8 +27,8 @@ and 'push-cert' capabilities are sent and recognized by the receive-pack
 (push to server) process.
 
 The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
-by both upload-pack and receive-pack protocols.  The 'agent' capability
-may optionally be sent in both protocols.
+by both upload-pack and receive-pack protocols.  The 'agent' and 'session-id'
+capabilities may optionally be sent in both protocols.
 
 All other capabilities are only recognized by the upload-pack (fetch
 from server) process.

@@ -365,3 +365,16 @@ If the upload-pack server advertises the 'filter' capability,
 fetch-pack may send "filter" commands to request a partial clone
 or partial fetch and request that the server omit various objects
 from the packfile.
+
+session-id=<session id>
+-----------------------
+
+The server may advertise a session ID that can be used to identify this process
+across multiple requests. The client may advertise its own session ID back to
+the server as well.
+
+Session IDs should be unique to a given process. They must fit within a
+packet-line, and must not contain non-printable or whitespace characters. The
+current implementation uses trace2 session IDs (see
+link:api-trace2.html[api-trace2] for details), but this may change and users of
+the session ID should not rely on this fact.

index e597b74da3..85daeb5d9e 100644
@@ -492,3 +492,16 @@ form `object-format=X`) to notify the client that the server is able to deal
 with objects using hash algorithm X.  If not specified, the server is assumed to
 only handle SHA-1.  If the client would like to use a hash algorithm other than
 SHA-1, it should specify its object-format string.
+
+session-id=<session id>
+~~~~~~~~~~~~~~~~~~~~~~~
+
+The server may advertise a session ID that can be used to identify this process
+across multiple requests. The client may advertise its own session ID back to
+the server as well.
+
+Session IDs should be unique to a given process. They must fit within a
+packet-line, and must not contain non-printable or whitespace characters. The
+current implementation uses trace2 session IDs (see
+link:api-trace2.html[api-trace2] for details), but this may change and users of
+the session ID should not rely on this fact.

2.29.2.222.g5d2a92d10f8-goog

Document a new config option that allows users to determine whether or not to advertise their session IDs to remote Git clients and servers.

---
 Documentation/config/transfer.txt | 4 ++++
 1 file changed, 4 insertions(+)

index f5b6245270..505126a780 100644
@@ -69,3 +69,7 @@ transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
 	not set, the value of this variable is used instead.
 	The default value is 100.
+
+transfer.advertiseSID::
+	Boolean. When true, client and server processes will advertise their
+	unique session IDs to their remote counterpart. Defaults to false.

2.29.2.222.g5d2a92d10f8-goog

Add a public wrapper, trace2_session_id(), around tr2_sid_get(), which is intended to be private trace2 implementation.

---
 trace2.c | 5 +++++
 trace2.h | 2 ++
 2 files changed, 7 insertions(+)

index 2c6b570077..256120c7fd 100644
@@ -792,3 +792,8 @@ void trace2_printf(const char *fmt, ...)
 	va_end(ap);
 }
 #endif
+
+const char *trace2_session_id(void)
+{
+	return tr2_sid_get();
+}

index b18bc5529c..ede18c2e06 100644
@@ -500,4 +500,6 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason);
 	} while (0)
 #endif
 
+const char *trace2_session_id(void);
+
 #endif /* TRACE2_H */

2.29.2.222.g5d2a92d10f8-goog

When transfer.advertiseSID is true, advertise upload-pack's session ID via the new session-id capability.

---
 upload-pack.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

index 3b858eb457..ebb4099268 100644
@@ -110,6 +110,7 @@ struct upload_pack_data {
 	unsigned done : 1;					/* v2 only */
 	unsigned allow_ref_in_want : 1;				/* v2 only */
 	unsigned allow_sideband_all : 1;			/* v2 only */
+	unsigned advertise_sid : 1;
 };
 
 static void upload_pack_data_init(struct upload_pack_data *data)

@@ -141,6 +142,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	packet_writer_init(&data->writer, 1);
 
 	data->keepalive = 5;
+	data->advertise_sid = 0;
 }
 
 static void upload_pack_data_clear(struct upload_pack_data *data)

@@ -1178,6 +1180,11 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)
 		strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util);
 }
 
+static void format_session_id(struct strbuf *buf, struct upload_pack_data *d) {
+	if (d->advertise_sid)
+		strbuf_addf(buf, " session-id=%s", trace2_session_id());
+}
+
 static int send_ref(const char *refname, const struct object_id *oid,
 		    int flag, void *cb_data)
 {

@@ -1193,9 +1200,11 @@ static int send_ref(const char *refname, const struct object_id *oid,
 
 	if (capabilities) {
 		struct strbuf symref_info = STRBUF_INIT;
+		struct strbuf session_id = STRBUF_INIT;
 
 		format_symref_info(&symref_info, &data->symref);
-		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s object-format=%s agent=%s\n",
+		format_session_id(&session_id, data);
+		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n",
 			     oid_to_hex(oid), refname_nons,
 			     0, capabilities,
 			     (data->allow_uor & ALLOW_TIP_SHA1) ?

@@ -1205,9 +1214,11 @@ static int send_ref(const char *refname, const struct object_id *oid,
 			     data->stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     data->allow_filter ? " filter" : "",
+			     session_id.buf,
 			     the_hash_algo->name,
 			     git_user_agent_sanitized());
 		strbuf_release(&symref_info);
+		strbuf_release(&session_id);
 	} else {
 		packet_write_fmt(1, "%s %s\n", oid_to_hex(oid), refname_nons);
 	}

@@ -1299,6 +1310,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
 		data->allow_sideband_all = git_config_bool(var, value);
 	} else if (!strcmp("core.precomposeunicode", var)) {
 		precomposed_unicode = git_config_bool(var, value);
+	} else if (!strcmp("transfer.advertisesid", var)) {
+		data->advertise_sid = git_config_bool(var, value);
 	}
 
 	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&

2.29.2.222.g5d2a92d10f8-goog

When transfer.advertiseSID is true, advertise receive-pack's session ID via the new session-id capability.

---
 builtin/receive-pack.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index bb9909c52e..6ed498b6c7 100644
@@ -54,6 +54,7 @@ static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int advertise_atomic_push = 1;
 static int advertise_push_options;
+static int advertise_sid;
 static int unpack_limit = 100;
 static off_t max_input_size;
 static int report_status;

@@ -248,6 +249,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "transfer.advertisesid") == 0) {
+		advertise_sid = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 

@@ -268,6 +274,8 @@ static void show_ref(const char *path, const struct object_id *oid)
 			strbuf_addf(&cap, " push-cert=%s", push_cert_nonce);
 		if (advertise_push_options)
 			strbuf_addstr(&cap, " push-options");
+		if (advertise_sid)
+			strbuf_addf(&cap, " session-id=%s", trace2_session_id());
 		strbuf_addf(&cap, " object-format=%s", the_hash_algo->name);
 		strbuf_addf(&cap, " agent=%s", git_user_agent_sanitized());
 		packet_write_fmt(1, "%s %s%c%s\n",

2.29.2.222.g5d2a92d10f8-goog

When transfer.advertiseSID is true, advertise the server's session ID for all protocol v2 connections via the new session-id capability.

---
 serve.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index f6341206c4..8c0bb84f37 100644
@@ -8,6 +8,8 @@
 #include "serve.h"
 #include "upload-pack.h"
 
+static int advertise_sid;
+
 static int always_advertise(struct repository *r,
 			    struct strbuf *value)
 {

@@ -30,6 +32,15 @@ static int object_format_advertise(struct repository *r,
 	return 1;
 }
 
+static int session_id_advertise(struct repository *r, struct strbuf *value)
+{
+	if (!advertise_sid)
+		return 0;
+	if (value)
+		strbuf_addstr(value, trace2_session_id());
+	return 1;
+}
+
 struct protocol_capability {
 	/*
 	 * The name of the capability.  The server uses this name when

@@ -66,6 +77,7 @@ static struct protocol_capability capabilities[] = {
 	{ "fetch", upload_pack_advertise, upload_pack_v2 },
 	{ "server-option", always_advertise, NULL },
 	{ "object-format", object_format_advertise, NULL },
+	{ "session-id", session_id_advertise, NULL },
 };
 
 static void advertise_capabilities(void)

@@ -261,6 +273,8 @@ static int process_request(void)
 /* Main serve loop for protocol version 2 */
 void serve(struct serve_options *options)
 {
+	git_config_get_bool("transfer.advertisesid", &advertise_sid);
+
 	if (options->advertise_capabilities || !options->stateless_rpc) {
 		/* serve by default supports v2 */
 		packet_write_fmt(1, "version 2\n");

2.29.2.222.g5d2a92d10f8-goog

When a client receives a session-id capability from a protocol v0, v1, or v2 server, log the received session ID via a trace2 data event.

---
 t/t5705-session-id-in-capabilities.sh | 64 +++++++++++++++++++++++++++
 transport.c                           | 10 +++++
 2 files changed, 74 insertions(+)
 create mode 100755 t/t5705-session-id-in-capabilities.sh

new file mode 100755
index 0000000000..9e782f4413
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='session ID in capabilities'
+
+. ./test-lib.sh
+
+REPO="$(pwd)/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for session ID capability tests' '
+	git init "$REPO" &&
+	test_commit -C "$REPO" a &&
+	git clone "file://$REPO" "$LOCAL_PRISTINE" &&
+	test_commit -C "$REPO" b
+'
+
+for PROTO in 0 1 2
+do
+	test_expect_success "session IDs not advertised by default (fetch v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local fetch origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+	'
+
+	test_expect_success "session IDs not advertised by default (push v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		git -C local pull --no-rebase origin &&
+		GIT_TRACE2_EVENT_NESTING=5 \
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local push origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+	'
+done
+
+test_expect_success 'enable SID advertisement' '
+	git -C "$REPO" config transfer.advertiseSID true &&
+	git -C "$LOCAL_PRISTINE" config transfer.advertiseSID true
+'
+
+for PROTO in 0 1 2
+do
+	test_expect_success "session IDs advertised (fetch v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local fetch origin &&
+		grep \"key\":\"server-sid\" tr2-client-events
+	'
+
+	test_expect_success "session IDs advertised (push v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		git -C local pull --no-rebase origin &&
+		GIT_TRACE2_EVENT_NESTING=5 \
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local push origin &&
+		grep \"key\":\"server-sid\" tr2-client-events
+	'
+done
+
+test_done

index 47da955e4f..679a35e7c1 100644
@@ -286,6 +286,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
 	struct git_transport_data *data = transport->data;
 	struct ref *refs = NULL;
 	struct packet_reader reader;
+	int sid_len;
+	const char *server_sid;
 
 	connect_setup(transport, for_push);
 

@@ -297,6 +299,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
 	data->version = discover_version(&reader);
 	switch (data->version) {
 	case protocol_v2:
+		if (server_feature_v2("session-id", &server_sid))
+			trace2_data_string("transfer", NULL, "server-sid", server_sid);
 		if (must_list_refs)
 			get_remote_refs(data->fd[1], &reader, &refs, for_push,
 					ref_prefixes,

@@ -310,6 +314,12 @@ static struct ref *handshake(struct transport *transport, int for_push,
 				 for_push ? REF_NORMAL : 0,
 				 &data->extra_have,
 				 &data->shallow);
+		server_sid = server_feature_value("session-id", &sid_len);
+		if (server_sid) {
+			char *sid = xstrndup(server_sid, sid_len);
+			trace2_data_string("transfer", NULL, "server-sid", sid);
+			free(sid);
+		}
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");

2.29.2.222.g5d2a92d10f8-goog

When the server sent a session-id capability and transfer.advertiseSID is true, advertise fetch-pack's own session ID back to the server.

---
 fetch-pack.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index b10c432315..23179b8dd0 100644
@@ -35,6 +35,7 @@ static int fetch_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
 static int agent_supported;
 static int server_supports_filtering;
+static int advertise_sid;
 static struct shallow_lock shallow_lock;
 static const char *alternate_shallow_file;
 static struct strbuf fsck_msg_types = STRBUF_INIT;

@@ -326,6 +327,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_sid)
+				strbuf_addf(&c, " session-id=%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 +982,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 				      agent_len, agent_feature);
 	}
 
+	if (!server_supports("session-id"))
+		advertise_sid = 0;
+
 	if (server_supports("shallow"))
 		print_verbose(args, _("Server supports %s"), "shallow");
 	else if (args->depth > 0 || is_repository_shallow(r))

@@ -1191,6 +1197,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_sid && server_supports_v2("session-id", 0))
+		packet_buf_write(&req_buf, "session-id=%s", trace2_session_id());
 	if (args->server_options && args->server_options->nr &&
 	    server_supports_v2("server-option", 1)) {
 		int i;

@@ -1711,6 +1719,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("transfer.advertisesid", &advertise_sid);
 	if (!uri_protocols.nr) {
 		char *str;
 

2.29.2.222.g5d2a92d10f8-goog

When upload-pack (protocol v0/v1) or a protocol v2 server receives a session-id capability from a client, log the received session ID via a trace2 data event.

---
 serve.c                               |  4 ++++
 t/t5705-session-id-in-capabilities.sh | 18 ++++++++++++------
 upload-pack.c                         |  8 ++++++++
 3 files changed, 24 insertions(+), 6 deletions(-)

index 8c0bb84f37..eec2fe6f29 100644
@@ -201,6 +201,7 @@ static int process_request(void)
 	struct packet_reader reader;
 	struct strvec keys = STRVEC_INIT;
 	struct protocol_capability *command = NULL;
+	const char *client_sid;
 
 	packet_reader_init(&reader, 0, NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |

@@ -264,6 +265,9 @@ static int process_request(void)
 
 	check_algorithm(the_repository, &keys);
 
+	if (has_capability(&keys, "session-id", &client_sid))
+		trace2_data_string("transfer", NULL, "client-sid", client_sid);
+
 	command->command(the_repository, &keys, &reader);
 
 	strvec_clear(&keys);

index 9e782f4413..afa2159657 100755
@@ -17,11 +17,14 @@ test_expect_success 'setup repos for session ID capability tests' '
 for PROTO in 0 1 2
 do
 	test_expect_success "session IDs not advertised by default (fetch v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local fetch origin &&
-		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+		git -c protocol.version=$PROTO -C local fetch \
+			--upload-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-upload-pack" \
+			origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)" &&
+		test -z "$(grep \"key\":\"client-sid\" tr2-server-events)"
 	'
 
 	test_expect_success "session IDs not advertised by default (push v${PROTO})" '

@@ -43,11 +46,14 @@ test_expect_success 'enable SID advertisement' '
 for PROTO in 0 1 2
 do
 	test_expect_success "session IDs advertised (fetch v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local fetch origin &&
-		grep \"key\":\"server-sid\" tr2-client-events
+		git -c protocol.version=$PROTO -C local fetch \
+			--upload-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-upload-pack" \
+			origin &&
+		grep \"key\":\"server-sid\" tr2-client-events &&
+		grep \"key\":\"client-sid\" tr2-server-events
 	'
 
 	test_expect_success "session IDs advertised (push v${PROTO})" '

index ebb4099268..dcd429dc01 100644
@@ -1058,6 +1058,7 @@ static void receive_needs(struct upload_pack_data *data,
 		const char *features;
 		struct object_id oid_buf;
 		const char *arg;
+		int feature_len;
 
 		reset_timeout(data->timeout);
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL)

@@ -1110,6 +1111,13 @@ static void receive_needs(struct upload_pack_data *data,
 		    parse_feature_request(features, "filter"))
 			data->filter_capability_requested = 1;
 
+		arg = parse_feature_value(features, "session-id", &feature_len, NULL);
+		if (arg) {
+			char *client_sid = xstrndup(arg, feature_len);
+			trace2_data_string("transfer", NULL, "client-sid", client_sid);
+			free(client_sid);
+		}
+
 		o = parse_object(the_repository, &oid_buf);
 		if (!o) {
 			packet_writer_error(&data->writer,

2.29.2.222.g5d2a92d10f8-goog

When the server sent a session-id capability and transfer.advertiseSID is true, advertise send-pack's own session ID back to the server.

---
 send-pack.c | 7 +++++++
 1 file changed, 7 insertions(+)

index eb4a44270b..bda65c98f9 100644
@@ -424,6 +424,7 @@ int send_pack(struct send_pack_args *args,
 	int use_sideband = 0;
 	int quiet_supported = 0;
 	int agent_supported = 0;
+	int advertise_sid = 0;
 	int use_atomic = 0;
 	int atomic_supported = 0;
 	int use_push_options = 0;

@@ -435,6 +436,8 @@ int send_pack(struct send_pack_args *args,
 	const char *push_cert_nonce = NULL;
 	struct packet_reader reader;
 
+	git_config_get_bool("transfer.advertisesid", &advertise_sid);
+
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status-v2"))
 		status_report = 2;

@@ -450,6 +453,8 @@ int send_pack(struct send_pack_args *args,
 		quiet_supported = 1;
 	if (server_supports("agent"))
 		agent_supported = 1;
+	if (!server_supports("session-id"))
+		advertise_sid = 0;
 	if (server_supports("no-thin"))
 		args->use_thin_pack = 0;
 	if (server_supports("atomic"))

@@ -506,6 +511,8 @@ int send_pack(struct send_pack_args *args,
 		strbuf_addf(&cap_buf, " object-format=%s", the_hash_algo->name);
 	if (agent_supported)
 		strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
+	if (advertise_sid)
+		strbuf_addf(&cap_buf, " session-id=%s", trace2_session_id());
 
 	/*
 	 * NEEDSWORK: why does delete-refs have to be so specific to

2.29.2.222.g5d2a92d10f8-goog

When receive-pack receives a session-id capability from the client, log the received session ID via a trace2 data event.

---
 builtin/receive-pack.c                |  7 +++++++
 t/t5705-session-id-in-capabilities.sh | 20 ++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

index 6ed498b6c7..deb5f859a9 100644
@@ -2039,6 +2039,7 @@ static struct command *read_head_info(struct packet_reader *reader,
 		if (linelen < reader->pktlen) {
 			const char *feature_list = reader->line + linelen + 1;
 			const char *hash = NULL;
+			const char *client_sid;
 			int len = 0;
 			if (parse_feature_request(feature_list, "report-status"))
 				report_status = 1;

@@ -2061,6 +2062,12 @@ static struct command *read_head_info(struct packet_reader *reader,
 			}
 			if (xstrncmpz(the_hash_algo->name, hash, len))
 				die("error: unsupported object format '%s'", hash);
+			client_sid = parse_feature_value(feature_list, "session-id", &len, NULL);
+			if (client_sid) {
+				char *sid = xstrndup(client_sid, len);
+				trace2_data_string("transfer", NULL, "client-sid", client_sid);
+				free(sid);
+			}
 		}
 
 		if (!strcmp(reader->line, "push-cert")) {

index afa2159657..f1d189d5bc 100755
@@ -28,13 +28,17 @@ do
 	'
 
 	test_expect_success "session IDs not advertised by default (push v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
+		test_when_finished "git -C local push --delete origin new-branch" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		git -C local pull --no-rebase origin &&
 		GIT_TRACE2_EVENT_NESTING=5 \
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local push origin &&
-		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+		git -c protocol.version=$PROTO -C local push \
+			--receive-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-receive-pack" \
+			origin HEAD:new-branch &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)" &&
+		test -z "$(grep \"key\":\"client-sid\" tr2-server-events)"
 	'
 done
 

@@ -57,13 +61,17 @@ do
 	'
 
 	test_expect_success "session IDs advertised (push v${PROTO})" '
-		test_when_finished "rm -rf local tr2-client-events" &&
+		test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
+		test_when_finished "git -C local push --delete origin new-branch" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		git -C local pull --no-rebase origin &&
 		GIT_TRACE2_EVENT_NESTING=5 \
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
-		git -c protocol.version=$PROTO -C local push origin &&
-		grep \"key\":\"server-sid\" tr2-client-events
+		git -c protocol.version=$PROTO -C local push \
+			--receive-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-receive-pack" \
+			origin HEAD:new-branch &&
+		grep \"key\":\"server-sid\" tr2-client-events &&
+		grep \"key\":\"client-sid\" tr2-server-events
 	'
 done
 

2.29.2.222.g5d2a92d10f8-goog

This round didn't see any responses, but it has already been extensively reviewed in the previous two rounds and looking good.

Will mark to be merged to 'next', but please holler to stop me if needed (those in v2 review CC'ed).

Thanks.

I still intend to write some documentation to clarify the trace2 SID structure, but I think that can be done as a separate series. Thanks!