[PATCH 00/10] Advertise trace2 SID in protocol capabilities

[PATCH 00/10] Advertise trace2 SID in protocol capabilities

[Josh Steadmon]

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

[PATCH 01/10] docs: new capability to advertise trace2 SIDs

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--
 Documentation/technical/protocol-v2.txt           |  9 +++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index ba869a7d36..73d4ee7f27 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -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.
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index e597b74da3..a5b9ef04f6 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -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

[PATCH 02/10] docs: new trace2.advertiseSID option

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/config/trace2.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt
index 01d3afd8a8..3f2e3b4425 100644
--- a/Documentation/config/trace2.txt
+++ b/Documentation/config/trace2.txt
@@ -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

[PATCH 03/10] upload-pack: advertise trace2 SID in v0 capabilities

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 upload-pack.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/upload-pack.c b/upload-pack.c
index 3b858eb457..862656010c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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

[PATCH 04/10] receive-pack: advertise trace2 SID in v0 capabilities

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 builtin/receive-pack.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index bb9909c52e..1ff83c874b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -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

[PATCH 05/10] serve: advertise trace2 SID in v2 capabilities

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 serve.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/serve.c b/serve.c
index f6341206c4..6ad73d69ab 100644
--- a/serve.c
+++ b/serve.c
@@ -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

[PATCH 06/10] transport: log received server trace2 SID

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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

diff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh
new file mode 100755
index 0000000000..0870e78f7c
--- /dev/null
+++ b/t/t5705-trace2-sid-in-capabilities.sh
@@ -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
diff --git a/transport.c b/transport.c
index 47da955e4f..d16be597bd 100644
--- a/transport.c
+++ b/transport.c
@@ -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

[PATCH 07/10] fetch-pack: advertise trace2 SID in capabilities

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 fetch-pack.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fetch-pack.c b/fetch-pack.c
index b10c432315..40065fc7dd 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -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

[PATCH 08/10] upload-pack, serve: log received client trace2 SID

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 serve.c                               |  4 ++++
 t/t5705-trace2-sid-in-capabilities.sh | 19 +++++++++++++------
 upload-pack.c                         |  7 +++++++
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/serve.c b/serve.c
index 6ad73d69ab..6a4f0c64a9 100644
--- a/serve.c
+++ b/serve.c
@@ -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);
diff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh
index 0870e78f7c..3fad9462d3 100755
--- a/t/t5705-trace2-sid-in-capabilities.sh
+++ b/t/t5705-trace2-sid-in-capabilities.sh
@@ -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})" '
diff --git a/upload-pack.c b/upload-pack.c
index 862656010c..4238e06510 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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

[PATCH 09/10] send-pack: advertise trace2 SID in capabilities

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 send-pack.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/send-pack.c b/send-pack.c
index eb4a44270b..56c6740e65 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -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

[PATCH 10/10] receive-pack: log received client trace2 SID

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 builtin/receive-pack.c                |  7 +++++++
 t/t5705-trace2-sid-in-capabilities.sh | 20 ++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 1ff83c874b..34199e4933 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -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")) {
diff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh
index 3fad9462d3..15f78cb7d2 100755
--- a/t/t5705-trace2-sid-in-capabilities.sh
+++ b/t/t5705-trace2-sid-in-capabilities.sh
@@ -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

Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities

[Jeff Hostetler]

On 10/29/20 5:32 PM, Josh Steadmon wrote:
> 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.
> 

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


> 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?

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();
}


> 
> 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?

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


> 
> 
> 
> 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
> 

Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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?

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.

Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities

[Josh Steadmon]

On 2020.10.30 11:54, Jeff Hostetler wrote:
> 
> 
> On 10/29/20 5:32 PM, Josh Steadmon wrote:
> > 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.
> > 
> 
> Very nice!  This should be very helpful when matching up client and
> server commands.
> 
> 
> > 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?
> 
> 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();
> }

Done in V2, thanks.


> > 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?
> 
> 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 ?

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.


> 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.)

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,
> Jeff

Thanks for the review!


> > 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
> > 

[PATCH v2 00/11] Advertise trace2 SID in protocol capabilities

[Josh Steadmon]

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

[PATCH v2 01/11] docs: new capability to advertise trace2 SIDs

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--
 Documentation/technical/protocol-v2.txt           |  9 +++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index ba869a7d36..73d4ee7f27 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -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.
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index e597b74da3..a5b9ef04f6 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -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

[PATCH v2 02/11] docs: new trace2.advertiseSID option

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/config/trace2.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt
index 01d3afd8a8..3f2e3b4425 100644
--- a/Documentation/config/trace2.txt
+++ b/Documentation/config/trace2.txt
@@ -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

[PATCH v2 03/11] trace2: add a public function for getting the SID

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 trace2.c | 5 +++++
 trace2.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/trace2.c b/trace2.c
index 2c6b570077..256120c7fd 100644
--- a/trace2.c
+++ b/trace2.c
@@ -792,3 +792,8 @@ void trace2_printf(const char *fmt, ...)
 	va_end(ap);
 }
 #endif
+
+const char *trace2_session_id(void)
+{
+	return tr2_sid_get();
+}
diff --git a/trace2.h b/trace2.h
index b18bc5529c..ede18c2e06 100644
--- a/trace2.h
+++ b/trace2.h
@@ -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

[PATCH v2 04/11] upload-pack: advertise trace2 SID in v0 capabilities

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 upload-pack.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/upload-pack.c b/upload-pack.c
index 3b858eb457..3bb01c5427 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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

[PATCH v2 05/11] receive-pack: advertise trace2 SID in v0 capabilities

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 builtin/receive-pack.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index bb9909c52e..2fe5a8b4fd 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -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

[PATCH v2 06/11] serve: advertise trace2 SID in v2 capabilities

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 serve.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/serve.c b/serve.c
index f6341206c4..b40d7aad34 100644
--- a/serve.c
+++ b/serve.c
@@ -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

[PATCH v2 07/11] transport: log received server trace2 SID

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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

diff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh
new file mode 100755
index 0000000000..0870e78f7c
--- /dev/null
+++ b/t/t5705-trace2-sid-in-capabilities.sh
@@ -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
diff --git a/transport.c b/transport.c
index 47da955e4f..d16be597bd 100644
--- a/transport.c
+++ b/transport.c
@@ -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

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

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 fetch-pack.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fetch-pack.c b/fetch-pack.c
index b10c432315..7fbefa7b8e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -35,6 +35,8 @@ static int fetch_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
 static int agent_supported;
 static int server_supports_filtering;
+static int server_sent_trace2_sid;
+static int advertise_trace2_sid;
 static struct shallow_lock shallow_lock;
 static const char *alternate_shallow_file;
 static struct strbuf fsck_msg_types = STRBUF_INIT;
@@ -326,6 +328,8 @@ static int find_common(struct fetch_negotiator *negotiator,
 			if (deepen_not_ok)      strbuf_addstr(&c, " deepen-not");
 			if (agent_supported)    strbuf_addf(&c, " agent=%s",
 							    git_user_agent_sanitized());
+			if (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())
+				strbuf_addf(&c, " trace2-sid=%s", trace2_session_id());
 			if (args->filter_options.choice)
 				strbuf_addstr(&c, " filter");
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
@@ -979,6 +983,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 				      agent_len, agent_feature);
 	}
 
+	if (server_supports("trace2-sid"))
+		server_sent_trace2_sid = 1;
+
 	if (server_supports("shallow"))
 		print_verbose(args, _("Server supports %s"), "shallow");
 	else if (args->depth > 0 || is_repository_shallow(r))
@@ -1191,6 +1198,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 		packet_buf_write(&req_buf, "command=fetch");
 	if (server_supports_v2("agent", 0))
 		packet_buf_write(&req_buf, "agent=%s", git_user_agent_sanitized());
+	if (advertise_trace2_sid && server_supports_v2("trace2-sid", 0) && trace2_is_enabled())
+		packet_buf_write(&req_buf, "trace2-sid=%s", trace2_session_id());
 	if (args->server_options && args->server_options->nr &&
 	    server_supports_v2("server-option", 1)) {
 		int i;
@@ -1711,6 +1720,7 @@ static void fetch_pack_config(void)
 	git_config_get_bool("repack.usedeltabaseoffset", &prefer_ofs_delta);
 	git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects);
 	git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects);
+	git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid);
 	if (!uri_protocols.nr) {
 		char *str;
 
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH v2 09/11] upload-pack, serve: log received client trace2 SID

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 serve.c                               |  4 ++++
 t/t5705-trace2-sid-in-capabilities.sh | 19 +++++++++++++------
 upload-pack.c                         |  7 +++++++
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/serve.c b/serve.c
index b40d7aad34..ffe5ba3f8a 100644
--- a/serve.c
+++ b/serve.c
@@ -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);
diff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh
index 0870e78f7c..3fad9462d3 100755
--- a/t/t5705-trace2-sid-in-capabilities.sh
+++ b/t/t5705-trace2-sid-in-capabilities.sh
@@ -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})" '
diff --git a/upload-pack.c b/upload-pack.c
index 3bb01c5427..d938603c97 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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

[PATCH v2 10/11] send-pack: advertise trace2 SID in capabilities

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 send-pack.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/send-pack.c b/send-pack.c
index eb4a44270b..7478a3f7cc 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -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

[PATCH v2 11/11] receive-pack: log received client trace2 SID

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 builtin/receive-pack.c                |  7 +++++++
 t/t5705-trace2-sid-in-capabilities.sh | 20 ++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 2fe5a8b4fd..f780a66a52 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -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")) {
diff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh
index 3fad9462d3..15f78cb7d2 100755
--- a/t/t5705-trace2-sid-in-capabilities.sh
+++ b/t/t5705-trace2-sid-in-capabilities.sh
@@ -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

Re: [PATCH v2 00/11] Advertise trace2 SID in protocol capabilities

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

Thanks; will replace.

Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

>> 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 ?
>
> 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.

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.

Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> +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.
> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
> index e597b74da3..a5b9ef04f6 100644
> --- a/Documentation/technical/protocol-v2.txt
> +++ b/Documentation/technical/protocol-v2.txt
> @@ -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.

Have we documented what a session-id should look like anywhere in
the documentation?  This document is to help third-party to write
implementations of the protocol, but the above description leaves
things "implementation defined" a bit too much, I am afraid.

For example, as this must fit on a single pkt-line as an advertised
capability, there would be some length limit.  Are there other
inherent limitations due to our protocol?  Are there some artificial
limitations that we may want to impose to make it easier to harden
implementations against common mistakes?  For example are bytes in
<session-id> allowed to contain LF, CR, NUL, etc.?

Re: [PATCH v2 02/11] docs: new trace2.advertiseSID option

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

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.

> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  Documentation/config/trace2.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt
> index 01d3afd8a8..3f2e3b4425 100644
> --- a/Documentation/config/trace2.txt
> +++ b/Documentation/config/trace2.txt
> @@ -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.

Re: [PATCH v2 04/11] upload-pack: advertise trace2 SID in v0 capabilities

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

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

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.

Re: [PATCH v2 06/11] serve: advertise trace2 SID in v2 capabilities

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  serve.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/serve.c b/serve.c
> index f6341206c4..b40d7aad34 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -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;
> +}

To both 05/11 and 06/11, the same "if the user instructs us to
advertise, shouldn't we arrange the thing we were told to advertise
to be automatically available by at least allocating a session ID,
if not enabling the entire tracing?" applies.

>  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");

Re: [PATCH v2 07/11] transport: log received server trace2 SID

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

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.

> diff --git a/transport.c b/transport.c
> index 47da955e4f..d16be597bd 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -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");

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

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

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

The same comment as 05/11 and 06/11 applies to this repeated
appearance of this boolean expression:

	advertise_trace2_sid && trace2_is_enabled()

If we are committed to stick to the "even if we were told to
advertise, do not alllocate a session ID" design, perhaps it is
cleaner to clear advertise_trace2_sid at the very beginning,
immediately after we learn that the tracing is disabled and the
advertiseSID configuration is read.  That way, everybody needs to
only care about advertise_trace2_sid variable.

Incidentally, if we decide to change the semantics to auto allocate
the session ID if advertiseSID configuration asks us to advertise
(it is OK if we do not enable the full trace2 suite), we can still
make the code only check advertise_trace2_sid variable, without
adding repeated check of trace2_is_enabled() everywhere at all.

Re: [PATCH v2 09/11] upload-pack, serve: log received client trace2 SID

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> diff --git a/upload-pack.c b/upload-pack.c
> index 3bb01c5427..d938603c97 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -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)))
> +		{

Style.  '{' block is opened on the same line as "if (condition)"
ends, e.g.

		arg = parse_feature_value(...);
		if (arg) {

Avoid assignment in the condition part of if/while unless there is a
compelling reason to do so.  e.g.

		if ((arg = ...) && further_check_on(arg)) {
			...

might be justifiable, but here, I do not see any such reason.

> +			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) {

Thanks.

Re: [PATCH v2 04/11] upload-pack: advertise trace2 SID in v0 capabilities

[Josh Steadmon]

On 2020.11.03 13:48, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > When trace2 is enabled and trace2.advertiseSID is true, advertise
> > upload-pack's trace2 session ID via the new trace2-sid capability.
> 
> 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?

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.

> 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)?

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

> Thanks.
> 
> 

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

[Josh Steadmon]

On 2020.11.04 13:22, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > When trace2 is enabled, the server sent a trace2-sid capability, and
> > trace2.advertiseSID is true, advertise fetch-pack's own session ID back
> > to the server.
> >
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> >  fetch-pack.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index b10c432315..7fbefa7b8e 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -35,6 +35,8 @@ static int fetch_fsck_objects = -1;
> >  static int transfer_fsck_objects = -1;
> >  static int agent_supported;
> >  static int server_supports_filtering;
> > +static int server_sent_trace2_sid;
> > +static int advertise_trace2_sid;
> >  static struct shallow_lock shallow_lock;
> >  static const char *alternate_shallow_file;
> >  static struct strbuf fsck_msg_types = STRBUF_INIT;
> > @@ -326,6 +328,8 @@ static int find_common(struct fetch_negotiator *negotiator,
> >  			if (deepen_not_ok)      strbuf_addstr(&c, " deepen-not");
> >  			if (agent_supported)    strbuf_addf(&c, " agent=%s",
> >  							    git_user_agent_sanitized());
> > +			if (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())
> > +				strbuf_addf(&c, " trace2-sid=%s", trace2_session_id());
> >  			if (args->filter_options.choice)
> >  				strbuf_addstr(&c, " filter");
> >  			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
> > @@ -979,6 +983,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
> >  				      agent_len, agent_feature);
> >  	}
> >  
> > +	if (server_supports("trace2-sid"))
> > +		server_sent_trace2_sid = 1;
> > +
> >  	if (server_supports("shallow"))
> >  		print_verbose(args, _("Server supports %s"), "shallow");
> >  	else if (args->depth > 0 || is_repository_shallow(r))
> > @@ -1191,6 +1198,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
> >  		packet_buf_write(&req_buf, "command=fetch");
> >  	if (server_supports_v2("agent", 0))
> >  		packet_buf_write(&req_buf, "agent=%s", git_user_agent_sanitized());
> > +	if (advertise_trace2_sid && server_supports_v2("trace2-sid", 0) && trace2_is_enabled())
> > +		packet_buf_write(&req_buf, "trace2-sid=%s", trace2_session_id());
> >  	if (args->server_options && args->server_options->nr &&
> >  	    server_supports_v2("server-option", 1)) {
> >  		int i;
> > @@ -1711,6 +1720,7 @@ static void fetch_pack_config(void)
> >  	git_config_get_bool("repack.usedeltabaseoffset", &prefer_ofs_delta);
> >  	git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects);
> >  	git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects);
> > +	git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid);
> >  	if (!uri_protocols.nr) {
> >  		char *str;
> 
> The same comment as 05/11 and 06/11 applies to this repeated
> appearance of this boolean expression:
> 
> 	advertise_trace2_sid && trace2_is_enabled()
> 
> If we are committed to stick to the "even if we were told to
> advertise, do not alllocate a session ID" design, perhaps it is
> cleaner to clear advertise_trace2_sid at the very beginning,
> immediately after we learn that the tracing is disabled and the
> advertiseSID configuration is read.  That way, everybody needs to
> only care about advertise_trace2_sid variable.
> 
> Incidentally, if we decide to change the semantics to auto allocate
> the session ID if advertiseSID configuration asks us to advertise
> (it is OK if we do not enable the full trace2 suite), we can still
> make the code only check advertise_trace2_sid variable, without
> adding repeated check of trace2_is_enabled() everywhere at all.

Good point. Once we settle on whether or not to advertise when tracing
is enabled, I'll update these conditionals in V3.

Re: [PATCH v2 09/11] upload-pack, serve: log received client trace2 SID

[Josh Steadmon]

On 2020.11.04 13:26, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > diff --git a/upload-pack.c b/upload-pack.c
> > index 3bb01c5427..d938603c97 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -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)))
> > +		{
> 
> Style.  '{' block is opened on the same line as "if (condition)"
> ends, e.g.
> 
> 		arg = parse_feature_value(...);
> 		if (arg) {
> 
> Avoid assignment in the condition part of if/while unless there is a
> compelling reason to do so.  e.g.
> 
> 		if ((arg = ...) && further_check_on(arg)) {
> 			...
> 
> might be justifiable, but here, I do not see any such reason.

Will fix in V3.


> > +			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) {
> 
> Thanks.

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

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

>> The same comment as 05/11 and 06/11 applies to this repeated
>> appearance of this boolean expression:
>> 
>> 	advertise_trace2_sid && trace2_is_enabled()
>> 
>> If we are committed to stick to the "even if we were told to
>> advertise, do not alllocate a session ID" design, perhaps it is
>> cleaner to clear advertise_trace2_sid at the very beginning,
>> immediately after we learn that the tracing is disabled and the
>> advertiseSID configuration is read.  That way, everybody needs to
>> only care about advertise_trace2_sid variable.
>> 
>> Incidentally, if we decide to change the semantics to auto allocate
>> the session ID if advertiseSID configuration asks us to advertise
>> (it is OK if we do not enable the full trace2 suite), we can still
>> make the code only check advertise_trace2_sid variable, without
>> adding repeated check of trace2_is_enabled() everywhere at all.
>
> Good point. Once we settle on whether or not to advertise when tracing
> is enabled, I'll update these conditionals in V3.

Well, we can update these conditionals _before_ deciding that, and
that is the whole point of the part of my message you are responding
to.

Whether the advertise_trace2_sid means 

 (1) config asked and tracing enabled, or

 (2) config asked and we do not care whether tracing is enabled or not

it makes it easier to flip between (1) and (2) later if you clean up
the conditional first.

Thanks.

Re: [PATCH v2 02/11] docs: new trace2.advertiseSID option

[Josh Steadmon]

On 2020.11.03 13:42, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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.
> 
> 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?

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.

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

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

> We also may want to call that just "session ID", not "trace2
> session ID" in the description.
> 
> Thanks.
> 
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> >  Documentation/config/trace2.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt
> > index 01d3afd8a8..3f2e3b4425 100644
> > --- a/Documentation/config/trace2.txt
> > +++ b/Documentation/config/trace2.txt
> > @@ -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.

Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs

[Jeff Hostetler]

On 11/3/20 4:33 PM, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
>> +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.
>> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
>> index e597b74da3..a5b9ef04f6 100644
>> --- a/Documentation/technical/protocol-v2.txt
>> +++ b/Documentation/technical/protocol-v2.txt
>> @@ -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.
> 
> Have we documented what a session-id should look like anywhere in
> the documentation?  This document is to help third-party to write
> implementations of the protocol, but the above description leaves
> things "implementation defined" a bit too much, I am afraid.
> 
> For example, as this must fit on a single pkt-line as an advertised
> capability, there would be some length limit.  Are there other
> inherent limitations due to our protocol?  Are there some artificial
> limitations that we may want to impose to make it easier to harden
> implementations against common mistakes?  For example are bytes in
> <session-id> allowed to contain LF, CR, NUL, etc.?
> 

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

Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities

[Jeff Hostetler]

On 11/3/20 4:22 PM, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
>>> 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 ?
>>
>> 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.
> 
> 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

Re: [PATCH v2 02/11] docs: new trace2.advertiseSID option

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

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.

Re: [PATCH v2 02/11] docs: new trace2.advertiseSID option

[Johannes Schindelin]

Hi Junio,

On Thu, 5 Nov 2020, Junio C Hamano wrote:

> Josh Steadmon <steadmon@google.com> writes:
>
> > 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.
>
> 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.

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/20200207013918.GA459@comcast.net/#r is the
latest message in that thread that I could find).

Ciao,
Dscho

Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities

[Josh Steadmon]

On 2020.11.03 13:22, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> >> 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 ?
> >
> > 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.
> 
> 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.

Done in V3.

Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs

[Josh Steadmon]

On 2020.11.03 13:33, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > +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.
> > diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
> > index e597b74da3..a5b9ef04f6 100644
> > --- a/Documentation/technical/protocol-v2.txt
> > +++ b/Documentation/technical/protocol-v2.txt
> > @@ -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.
> 
> Have we documented what a session-id should look like anywhere in
> the documentation?  This document is to help third-party to write
> implementations of the protocol, but the above description leaves
> things "implementation defined" a bit too much, I am afraid.
> 
> For example, as this must fit on a single pkt-line as an advertised
> capability, there would be some length limit.  Are there other
> inherent limitations due to our protocol?  Are there some artificial
> limitations that we may want to impose to make it easier to harden
> implementations against common mistakes?  For example are bytes in
> <session-id> allowed to contain LF, CR, NUL, etc.?

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.

Re: [PATCH v2 07/11] transport: log received server trace2 SID

[Josh Steadmon]

On 2020.11.04 13:14, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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.
> 
> 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?

[PATCH v3 00/11] Advertise session ID in protocol capabilities

[Josh Steadmon]

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 <steadmon@google.com>
     
      ## 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 <steadmon@google.com>
     
      ## 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 <steadmon@google.com>
     
      ## 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 <steadmon@google.com>
     
      ## 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 <steadmon@google.com>
     
      ## 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 <steadmon@google.com>
     
      ## 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 <steadmon@google.com>
     
      ## 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 <steadmon@google.com>
     
      ## 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

[PATCH v3 01/11] docs: new capability to advertise session IDs

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 .../technical/protocol-capabilities.txt         | 17 +++++++++++++++--
 Documentation/technical/protocol-v2.txt         | 13 +++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index ba869a7d36..9dfade930d 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -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.
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index e597b74da3..85daeb5d9e 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -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

[PATCH v3 02/11] docs: new transfer.advertiseSID option

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/config/transfer.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt
index f5b6245270..505126a780 100644
--- a/Documentation/config/transfer.txt
+++ b/Documentation/config/transfer.txt
@@ -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

[PATCH v3 03/11] trace2: add a public function for getting the SID

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 trace2.c | 5 +++++
 trace2.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/trace2.c b/trace2.c
index 2c6b570077..256120c7fd 100644
--- a/trace2.c
+++ b/trace2.c
@@ -792,3 +792,8 @@ void trace2_printf(const char *fmt, ...)
 	va_end(ap);
 }
 #endif
+
+const char *trace2_session_id(void)
+{
+	return tr2_sid_get();
+}
diff --git a/trace2.h b/trace2.h
index b18bc5529c..ede18c2e06 100644
--- a/trace2.h
+++ b/trace2.h
@@ -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

[PATCH v3 04/11] upload-pack: advertise session ID in v0 capabilities

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 upload-pack.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/upload-pack.c b/upload-pack.c
index 3b858eb457..ebb4099268 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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

[PATCH v3 05/11] receive-pack: advertise session ID in v0 capabilities

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 builtin/receive-pack.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index bb9909c52e..6ed498b6c7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -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

[PATCH v3 06/11] serve: advertise session ID in v2 capabilities

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 serve.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/serve.c b/serve.c
index f6341206c4..8c0bb84f37 100644
--- a/serve.c
+++ b/serve.c
@@ -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

[PATCH v3 07/11] transport: log received server session ID

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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

diff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh
new file mode 100755
index 0000000000..9e782f4413
--- /dev/null
+++ b/t/t5705-session-id-in-capabilities.sh
@@ -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
diff --git a/transport.c b/transport.c
index 47da955e4f..679a35e7c1 100644
--- a/transport.c
+++ b/transport.c
@@ -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

[PATCH v3 08/11] fetch-pack: advertise session ID in capabilities

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 fetch-pack.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fetch-pack.c b/fetch-pack.c
index b10c432315..23179b8dd0 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -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

[PATCH v3 09/11] upload-pack, serve: log received client session ID

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 serve.c                               |  4 ++++
 t/t5705-session-id-in-capabilities.sh | 18 ++++++++++++------
 upload-pack.c                         |  8 ++++++++
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/serve.c b/serve.c
index 8c0bb84f37..eec2fe6f29 100644
--- a/serve.c
+++ b/serve.c
@@ -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);
diff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh
index 9e782f4413..afa2159657 100755
--- a/t/t5705-session-id-in-capabilities.sh
+++ b/t/t5705-session-id-in-capabilities.sh
@@ -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})" '
diff --git a/upload-pack.c b/upload-pack.c
index ebb4099268..dcd429dc01 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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

[PATCH v3 10/11] send-pack: advertise session ID in capabilities

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 send-pack.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/send-pack.c b/send-pack.c
index eb4a44270b..bda65c98f9 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -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

[PATCH v3 11/11] receive-pack: log received client session ID

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 builtin/receive-pack.c                |  7 +++++++
 t/t5705-session-id-in-capabilities.sh | 20 ++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 6ed498b6c7..deb5f859a9 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -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")) {
diff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh
index afa2159657..f1d189d5bc 100755
--- a/t/t5705-session-id-in-capabilities.sh
+++ b/t/t5705-session-id-in-capabilities.sh
@@ -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

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

[Josh Steadmon]

On 2020.11.05 11:21, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> >> The same comment as 05/11 and 06/11 applies to this repeated
> >> appearance of this boolean expression:
> >> 
> >> 	advertise_trace2_sid && trace2_is_enabled()
> >> 
> >> If we are committed to stick to the "even if we were told to
> >> advertise, do not alllocate a session ID" design, perhaps it is
> >> cleaner to clear advertise_trace2_sid at the very beginning,
> >> immediately after we learn that the tracing is disabled and the
> >> advertiseSID configuration is read.  That way, everybody needs to
> >> only care about advertise_trace2_sid variable.
> >> 
> >> Incidentally, if we decide to change the semantics to auto allocate
> >> the session ID if advertiseSID configuration asks us to advertise
> >> (it is OK if we do not enable the full trace2 suite), we can still
> >> make the code only check advertise_trace2_sid variable, without
> >> adding repeated check of trace2_is_enabled() everywhere at all.
> >
> > Good point. Once we settle on whether or not to advertise when tracing
> > is enabled, I'll update these conditionals in V3.
> 
> Well, we can update these conditionals _before_ deciding that, and
> that is the whole point of the part of my message you are responding
> to.
> 
> Whether the advertise_trace2_sid means 
> 
>  (1) config asked and tracing enabled, or
> 
>  (2) config asked and we do not care whether tracing is enabled or not
> 
> it makes it easier to flip between (1) and (2) later if you clean up
> the conditional first.
> 
> Thanks.
> 

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.

Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 05 2020, Jeff Hostetler wrote:

> 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.)

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

Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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.

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"?

> 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

Re: [PATCH v2 07/11] transport: log received server trace2 SID

[Jeff Hostetler]

On 11/11/20 5:53 PM, Josh Steadmon wrote:
> On 2020.11.04 13:14, Junio C Hamano wrote:
>> Josh Steadmon <steadmon@google.com> writes:
>>
>>> 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.
>>
>> 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

Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs

[Jeff Hostetler]

On 11/12/20 12:32 PM, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
>> 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'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.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.
> 
> 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)?

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

> 
>   - 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"?

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.

> 
>> 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

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

Re: [PATCH v3 00/11] Advertise session ID in protocol capabilities

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

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.

Re: [PATCH v3 00/11] Advertise session ID in protocol capabilities

[Josh Steadmon]

On 2020.11.25 14:08, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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.
> 
> 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!