{"thread":{"id":"54527","subject":"[PATCH 00/10] Advertise trace2 SID in protocol capabilities","startedAt":"2020-10-29T21:32:46Z","lastAt":"2020-11-25T22:57:28Z","messageCount":66,"participants":["Josh Steadmon","Jeff Hostetler","Junio C Hamano","Johannes Schindelin","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"408667","messageId":"cover.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":null,"subject":"[PATCH 00/10] Advertise trace2 SID in protocol capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:30Z","receivedAt":"2020-10-29T21:32:46Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"In order to more easily debug remote operations, it is useful to be able\nto inspect both client-side and server-side traces. This series allows\nclients to record the server's trace2 session ID, and vice versa, by\nadvertising the SID in a new \"trace2-sid\" protocol capability.\n\nTwo questions in particular for reviewers:\n\n1) Is trace2/tr2_sid.h intended to be visible to the rest of the code,\n or is the trace2/ directory supposed to be opaque implementation\n detail? If the latter, would it be acceptable to move tr2_sid_get()\n into trace2.h?\n\n2) upload-pack generally takes configuration via flags rather than\n gitconfig. From offline discussions, it sounds like this is an\n intentional choice to limit potential vulnerability from malicious\n configs in local repositories accessed via the file:// URL scheme. Is\n it reasonable to load the trace2.announceSID option from config files\n in upload-pack, or should this be changed to a flag?\n\n\n\nJosh Steadmon (10):\n docs: new capability to advertise trace2 SIDs\n docs: new trace2.advertiseSID option\n upload-pack: advertise trace2 SID in v0 capabilities\n receive-pack: advertise trace2 SID in v0 capabilities\n serve: advertise trace2 SID in v2 capabilities\n transport: log received server trace2 SID\n fetch-pack: advertise trace2 SID in capabilities\n upload-pack, serve: log received client trace2 SID\n send-pack: advertise trace2 SID in capabilities\n receive-pack: log received client trace2 SID\n\n Documentation/config/trace2.txt | 4 +\n .../technical/protocol-capabilities.txt | 13 ++-\n Documentation/technical/protocol-v2.txt | 9 +++\n builtin/receive-pack.c | 16 ++++\n fetch-pack.c | 11 +++\n send-pack.c | 9 +++\n serve.c | 19 +++++\n t/t5705-trace2-sid-in-capabilities.sh | 79 +++++++++++++++++++\n transport.c | 10 +++\n upload-pack.c | 23 +++++-\n 10 files changed, 190 insertions(+), 3 deletions(-)\n create mode 100755 t/t5705-trace2-sid-in-capabilities.sh\n\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408668","messageId":"d04028c3c7574e3ca0f9c1b3d711192ca756158d.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH 01/10] docs: new capability to advertise trace2 SIDs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:31Z","receivedAt":"2020-10-29T21:32:52Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"In future patches, we will add the ability for Git servers and clients\nto advertise their trace2 session IDs via protocol capabilities. This\nallows for easier debugging when both client and server logs are\navailable.\n\nSigned-off-by: Josh Steadmon \n---\n Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--\n Documentation/technical/protocol-v2.txt | 9 +++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex ba869a7d36..73d4ee7f27 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -27,8 +27,8 @@ and 'push-cert' capabilities are sent and recognized by the receive-pack\n (push to server) process.\n \n The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized\n-by both upload-pack and receive-pack protocols. The 'agent' capability\n-may optionally be sent in both protocols.\n+by both upload-pack and receive-pack protocols. The 'agent' and 'trace2-sid'\n+capabilities may optionally be sent in both protocols.\n \n All other capabilities are only recognized by the upload-pack (fetch\n from server) process.\n@@ -365,3 +365,12 @@ If the upload-pack server advertises the 'filter' capability,\n fetch-pack may send \"filter\" commands to request a partial clone\n or partial fetch and request that the server omit various objects\n from the packfile.\n+\n+trace2-sid=\n+-----------------------\n+\n+If trace2 tracing is enabled on the server, it may advertise its session ID via\n+this capability. The client may choose to log the server's session ID in its\n+trace logs, and advertise its own session ID back to the server for it to log\n+as well. This allows for easier debugging of remote sessions when both client\n+and server logs are available.\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex e597b74da3..a5b9ef04f6 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -492,3 +492,12 @@ form `object-format=X`) to notify the client that the server is able to deal\n with objects using hash algorithm X. If not specified, the server is assumed to\n only handle SHA-1. If the client would like to use a hash algorithm other than\n SHA-1, it should specify its object-format string.\n+\n+trace2-sid=\n+~~~~~~~~~~~~~~~~~~~~~~~\n+\n+If trace2 tracing is enabled on the server, it may advertise its session ID via\n+this capability. The client may choose to log the server's session ID in its\n+trace logs, and advertise its own session ID back to the server for it to log\n+as well. This allows for easier debugging of remote sessions when both client\n+and server logs are available.\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408669","messageId":"5d5097b67109554e0763724633810ea616b5e2b2.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH 02/10] docs: new trace2.advertiseSID option","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:32Z","receivedAt":"2020-10-29T21:32:52Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Document a new config option that allows users to determine whether or\nnot to advertise their trace2 session IDs to remote Git clients and\nservers.\n\nSigned-off-by: Josh Steadmon \n---\n Documentation/config/trace2.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt\nindex 01d3afd8a8..3f2e3b4425 100644\n--- a/Documentation/config/trace2.txt\n+++ b/Documentation/config/trace2.txt\n@@ -69,3 +69,7 @@ trace2.maxFiles::\n \twrite additional traces if we would exceed this many files. Instead,\n \twrite a sentinel file that will block further tracing to this\n \tdirectory. Defaults to 0, which disables this check.\n+\n+trace2.advertiseSID::\n+\tBoolean. When true, client and server processes will advertise their\n+\ttrace2 session IDs to their remote counterpart. Defaults to false.\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408670","messageId":"c07bf8aede12f2626d2bb8eb109b9fb28dea0dc2.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH 03/10] upload-pack: advertise trace2 SID in v0 capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:33Z","receivedAt":"2020-10-29T21:32:53Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When trace2 is enabled and trace2.advertiseSID is true, advertise\nupload-pack's trace2 session ID via the new trace2-sid capability.\n\nSigned-off-by: Josh Steadmon \n---\n upload-pack.c | 16 +++++++++++++++-\n 1 file changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b858eb457..862656010c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -27,6 +27,7 @@\n #include \"commit-graph.h\"\n #include \"commit-reach.h\"\n #include \"shallow.h\"\n+#include \"trace2/tr2_sid.h\"\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -110,6 +111,7 @@ struct upload_pack_data {\n \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n+\tunsigned advertise_trace2_sid : 1;\n };\n \n static void upload_pack_data_init(struct upload_pack_data *data)\n@@ -141,6 +143,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tpacket_writer_init(&data->writer, 1);\n \n \tdata->keepalive = 5;\n+\tdata->advertise_trace2_sid = 0;\n }\n \n static void upload_pack_data_clear(struct upload_pack_data *data)\n@@ -1178,6 +1181,11 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)\n \t\tstrbuf_addf(buf, \" symref=%s:%s\", item->string, (char *)item->util);\n }\n \n+static void format_trace2_info(struct strbuf *buf, struct upload_pack_data *d) {\n+\tif (d->advertise_trace2_sid && trace2_is_enabled())\n+\t\tstrbuf_addf(buf, \" trace2-sid=%s\", tr2_sid_get());\n+}\n+\n static int send_ref(const char *refname, const struct object_id *oid,\n \t\t int flag, void *cb_data)\n {\n@@ -1193,9 +1201,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \n \tif (capabilities) {\n \t\tstruct strbuf symref_info = STRBUF_INIT;\n+\t\tstruct strbuf trace2_info = STRBUF_INIT;\n \n \t\tformat_symref_info(&symref_info, &data->symref);\n-\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n+\t\tformat_trace2_info(&trace2_info, data);\n+\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n \t\t\t (data->allow_uor & ALLOW_TIP_SHA1) ?\n@@ -1205,9 +1215,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\t\t data->stateless_rpc ? \" no-done\" : \"\",\n \t\t\t symref_info.buf,\n \t\t\t data->allow_filter ? \" filter\" : \"\",\n+\t\t\t trace2_info.buf,\n \t\t\t the_hash_algo->name,\n \t\t\t git_user_agent_sanitized());\n \t\tstrbuf_release(&symref_info);\n+\t\tstrbuf_release(&trace2_info);\n \t} else {\n \t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n@@ -1299,6 +1311,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)\n \t\tdata->allow_sideband_all = git_config_bool(var, value);\n \t} else if (!strcmp(\"core.precomposeunicode\", var)) {\n \t\tprecomposed_unicode = git_config_bool(var, value);\n+\t} else if (!strcmp(\"trace2.advertisesid\", var)) {\n+\t\tdata->advertise_trace2_sid = git_config_bool(var, value);\n \t}\n \n \tif (current_config_scope() != CONFIG_SCOPE_LOCAL &&\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408671","messageId":"ff80f70d33c394b251b7baeae13bc5aab34ba16f.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH 05/10] serve: advertise trace2 SID in v2 capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:35Z","receivedAt":"2020-10-29T21:32:56Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When trace2 is enabled and trace2.advertiseSID is true, advertise the\nserver's trace2 session ID for all protocol v2 connections via the new\ntrace2-sid capability.\n\nSigned-off-by: Josh Steadmon \n---\n serve.c | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/serve.c b/serve.c\nindex f6341206c4..6ad73d69ab 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -7,6 +7,9 @@\n #include \"ls-refs.h\"\n #include \"serve.h\"\n #include \"upload-pack.h\"\n+#include \"trace2/tr2_sid.h\"\n+\n+static int advertise_trace2_sid;\n \n static int always_advertise(struct repository *r,\n \t\t\t struct strbuf *value)\n@@ -30,6 +33,15 @@ static int object_format_advertise(struct repository *r,\n \treturn 1;\n }\n \n+static int trace2_advertise(struct repository *r, struct strbuf *value)\n+{\n+\tif (!advertise_trace2_sid || !trace2_is_enabled())\n+\t\treturn 0;\n+\tif (value)\n+\t\tstrbuf_addstr(value, tr2_sid_get());\n+\treturn 1;\n+}\n+\n struct protocol_capability {\n \t/*\n \t * The name of the capability. The server uses this name when\n@@ -66,6 +78,7 @@ static struct protocol_capability capabilities[] = {\n \t{ \"fetch\", upload_pack_advertise, upload_pack_v2 },\n \t{ \"server-option\", always_advertise, NULL },\n \t{ \"object-format\", object_format_advertise, NULL },\n+\t{ \"trace2-sid\", trace2_advertise, NULL },\n };\n \n static void advertise_capabilities(void)\n@@ -261,6 +274,8 @@ static int process_request(void)\n /* Main serve loop for protocol version 2 */\n void serve(struct serve_options *options)\n {\n+\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n+\n \tif (options->advertise_capabilities || !options->stateless_rpc) {\n \t\t/* serve by default supports v2 */\n \t\tpacket_write_fmt(1, \"version 2\\n\");\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408672","messageId":"d42118ac41d74870782104d13dad59523d3dbaa3.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH 04/10] receive-pack: advertise trace2 SID in v0 capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:34Z","receivedAt":"2020-10-29T21:32:56Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When trace2 is enabled and trace2.advertiseSID is true, advertise\nreceive-pack's trace2 session ID via the new trace2-sid capability.\n\nSigned-off-by: Josh Steadmon \n---\n builtin/receive-pack.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bb9909c52e..1ff83c874b 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -29,6 +29,7 @@\n #include \"commit-reach.h\"\n #include \"worktree.h\"\n #include \"shallow.h\"\n+#include \"trace2/tr2_sid.h\"\n \n static const char * const receive_pack_usage[] = {\n \tN_(\"git receive-pack \"),\n@@ -54,6 +55,7 @@ static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int advertise_atomic_push = 1;\n static int advertise_push_options;\n+static int advertise_trace2_sid;\n static int unpack_limit = 100;\n static off_t max_input_size;\n static int report_status;\n@@ -248,6 +250,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"trace2.advertisesid\") == 0) {\n+\t\tadvertise_trace2_sid = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -268,6 +275,8 @@ static void show_ref(const char *path, const struct object_id *oid)\n \t\t\tstrbuf_addf(&cap, \" push-cert=%s\", push_cert_nonce);\n \t\tif (advertise_push_options)\n \t\t\tstrbuf_addstr(&cap, \" push-options\");\n+\t\tif (advertise_trace2_sid && trace2_is_enabled())\n+\t\t\tstrbuf_addf(&cap, \" trace2-sid=%s\", tr2_sid_get());\n \t\tstrbuf_addf(&cap, \" object-format=%s\", the_hash_algo->name);\n \t\tstrbuf_addf(&cap, \" agent=%s\", git_user_agent_sanitized());\n \t\tpacket_write_fmt(1, \"%s %s%c%s\\n\",\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408673","messageId":"1519767eadc396fa8de66c8a60b163f005aac503.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH 06/10] transport: log received server trace2 SID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:36Z","receivedAt":"2020-10-29T21:32:59Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When a client receives a trace2-sid capability from a protocol v0, v1,\nor v2 server, log the received session ID via a trace2 data event.\n\nSigned-off-by: Josh Steadmon \n---\n t/t5705-trace2-sid-in-capabilities.sh | 64 +++++++++++++++++++++++++++\n transport.c | 10 +++++\n 2 files changed, 74 insertions(+)\n create mode 100755 t/t5705-trace2-sid-in-capabilities.sh\n\ndiff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh\nnew file mode 100755\nindex 0000000000..0870e78f7c\n--- /dev/null\n+++ b/t/t5705-trace2-sid-in-capabilities.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='trace2 SID in capabilities'\n+\n+. ./test-lib.sh\n+\n+REPO=\"$(pwd)/repo\"\n+LOCAL_PRISTINE=\"$(pwd)/local_pristine\"\n+\n+test_expect_success 'setup repos for trace2 SID capability tests' '\n+\tgit init \"$REPO\" &&\n+\ttest_commit -C \"$REPO\" a &&\n+\tgit clone \"file://$REPO\" \"$LOCAL_PRISTINE\" &&\n+\ttest_commit -C \"$REPO\" b\n+'\n+\n+for PROTO in 0 1 2\n+do\n+\ttest_expect_success \"trace2 session IDs not advertised by default (fetch v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t'\n+\n+\ttest_expect_success \"trace2 session IDs not advertised by default (push v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tgit -C local pull --no-rebase origin &&\n+\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local push origin &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t'\n+done\n+\n+test_expect_success 'enable SID advertisement' '\n+\tgit -C \"$REPO\" config trace2.advertiseSID true &&\n+\tgit -C \"$LOCAL_PRISTINE\" config trace2.advertiseSID true\n+'\n+\n+for PROTO in 0 1 2\n+do\n+\ttest_expect_success \"trace2 session IDs advertised (fetch v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t'\n+\n+\ttest_expect_success \"trace2 session IDs advertised (push v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tgit -C local pull --no-rebase origin &&\n+\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local push origin &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t'\n+done\n+\n+test_done\ndiff --git a/transport.c b/transport.c\nindex 47da955e4f..d16be597bd 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -286,6 +286,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \tstruct git_transport_data *data = transport->data;\n \tstruct ref *refs = NULL;\n \tstruct packet_reader reader;\n+\tint sid_len;\n+\tconst char *server_trace2_sid;\n \n \tconnect_setup(transport, for_push);\n \n@@ -297,6 +299,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \tdata->version = discover_version(&reader);\n \tswitch (data->version) {\n \tcase protocol_v2:\n+\t\tif (server_feature_v2(\"trace2-sid\", &server_trace2_sid))\n+\t\t\ttrace2_data_string(\"trace2\", NULL, \"server-sid\", server_trace2_sid);\n \t\tif (must_list_refs)\n \t\t\tget_remote_refs(data->fd[1], &reader, &refs, for_push,\n \t\t\t\t\tref_prefixes,\n@@ -310,6 +314,12 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \t\t\t\t for_push ? REF_NORMAL : 0,\n \t\t\t\t &data->extra_have,\n \t\t\t\t &data->shallow);\n+\t\tserver_trace2_sid = server_feature_value(\"trace2-sid\", &sid_len);\n+\t\tif (server_trace2_sid) {\n+\t\t\tchar *server_sid = xstrndup(server_trace2_sid, sid_len);\n+\t\t\ttrace2_data_string(\"trace2\", NULL, \"server-sid\", server_sid);\n+\t\t\tfree(server_sid);\n+\t\t}\n \t\tbreak;\n \tcase protocol_unknown_version:\n \t\tBUG(\"unknown protocol version\");\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408674","messageId":"ee4b3a7a55f6823d86f9df4e87d6318019401f38.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH 07/10] fetch-pack: advertise trace2 SID in capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:37Z","receivedAt":"2020-10-29T21:33:01Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When trace2 is enabled, the server sent a trace2-sid capability, and\ntrace2.advertiseSID is true, advertise fetch-pack's own session ID back\nto the server.\n\nSigned-off-by: Josh Steadmon \n---\n fetch-pack.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b10c432315..40065fc7dd 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -23,6 +23,7 @@\n #include \"fetch-negotiator.h\"\n #include \"fsck.h\"\n #include \"shallow.h\"\n+#include \"trace2/tr2_sid.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -35,6 +36,8 @@ static int fetch_fsck_objects = -1;\n static int transfer_fsck_objects = -1;\n static int agent_supported;\n static int server_supports_filtering;\n+static int server_sent_trace2_sid;\n+static int advertise_trace2_sid;\n static struct shallow_lock shallow_lock;\n static const char *alternate_shallow_file;\n static struct strbuf fsck_msg_types = STRBUF_INIT;\n@@ -326,6 +329,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\tif (deepen_not_ok) strbuf_addstr(&c, \" deepen-not\");\n \t\t\tif (agent_supported) strbuf_addf(&c, \" agent=%s\",\n \t\t\t\t\t\t\t git_user_agent_sanitized());\n+\t\t\tif (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())\n+\t\t\t\tstrbuf_addf(&c, \" trace2-sid=%s\", tr2_sid_get());\n \t\t\tif (args->filter_options.choice)\n \t\t\t\tstrbuf_addstr(&c, \" filter\");\n \t\t\tpacket_buf_write(&req_buf, \"want %s%s\\n\", remote_hex, c.buf);\n@@ -979,6 +984,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t agent_len, agent_feature);\n \t}\n \n+\tif (server_supports(\"trace2-sid\"))\n+\t\tserver_sent_trace2_sid = 1;\n+\n \tif (server_supports(\"shallow\"))\n \t\tprint_verbose(args, _(\"Server supports %s\"), \"shallow\");\n \telse if (args->depth > 0 || is_repository_shallow(r))\n@@ -1191,6 +1199,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tpacket_buf_write(&req_buf, \"command=fetch\");\n \tif (server_supports_v2(\"agent\", 0))\n \t\tpacket_buf_write(&req_buf, \"agent=%s\", git_user_agent_sanitized());\n+\tif (advertise_trace2_sid && server_supports_v2(\"trace2-sid\", 0) && trace2_is_enabled())\n+\t\tpacket_buf_write(&req_buf, \"trace2-sid=%s\", tr2_sid_get());\n \tif (args->server_options && args->server_options->nr &&\n \t server_supports_v2(\"server-option\", 1)) {\n \t\tint i;\n@@ -1711,6 +1721,7 @@ static void fetch_pack_config(void)\n \tgit_config_get_bool(\"repack.usedeltabaseoffset\", &prefer_ofs_delta);\n \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n+\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n \tif (!uri_protocols.nr) {\n \t\tchar *str;\n \n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408675","messageId":"b160941c65e633f60fe08d71110e1d30b079c40e.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH 08/10] upload-pack, serve: log received client trace2 SID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:38Z","receivedAt":"2020-10-29T21:33:02Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When upload-pack (protocol v0/v1) or a protocol v2 server receives a\ntrace2-sid capability from a client, log the received session ID via a\ntrace2 data event.\n\nSigned-off-by: Josh Steadmon \n---\n serve.c | 4 ++++\n t/t5705-trace2-sid-in-capabilities.sh | 19 +++++++++++++------\n upload-pack.c | 7 +++++++\n 3 files changed, 24 insertions(+), 6 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 6ad73d69ab..6a4f0c64a9 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -202,6 +202,7 @@ static int process_request(void)\n \tstruct packet_reader reader;\n \tstruct strvec keys = STRVEC_INIT;\n \tstruct protocol_capability *command = NULL;\n+\tconst char *client_sid;\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -265,6 +266,9 @@ static int process_request(void)\n \n \tcheck_algorithm(the_repository, &keys);\n \n+\tif (has_capability(&keys, \"trace2-sid\", &client_sid))\n+\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n+\n \tcommand->command(the_repository, &keys, &reader);\n \n \tstrvec_clear(&keys);\ndiff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh\nindex 0870e78f7c..3fad9462d3 100755\n--- a/t/t5705-trace2-sid-in-capabilities.sh\n+++ b/t/t5705-trace2-sid-in-capabilities.sh\n@@ -17,11 +17,14 @@ test_expect_success 'setup repos for trace2 SID capability tests' '\n for PROTO in 0 1 2\n do\n \ttest_expect_success \"trace2 session IDs not advertised by default (fetch v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n-\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t\tgit -c protocol.version=$PROTO -C local fetch \\\n+\t\t\t--upload-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-upload-pack\" \\\n+\t\t\torigin &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\" &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"client-sid\\\" tr2-server-events)\"\n \t'\n \n \ttest_expect_success \"trace2 session IDs not advertised by default (push v${PROTO})\" '\n@@ -43,11 +46,15 @@ test_expect_success 'enable SID advertisement' '\n for PROTO in 0 1 2\n do\n \ttest_expect_success \"trace2 session IDs advertised (fetch v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tgit -C local config trace2.advertiseSID true &&\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n-\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t\tgit -c protocol.version=$PROTO -C local fetch \\\n+\t\t\t--upload-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-upload-pack\" \\\n+\t\t\torigin &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events &&\n+\t\tgrep \\\"key\\\":\\\"client-sid\\\" tr2-server-events\n \t'\n \n \ttest_expect_success \"trace2 session IDs advertised (push v${PROTO})\" '\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 862656010c..4238e06510 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1059,6 +1059,7 @@ static void receive_needs(struct upload_pack_data *data,\n \t\tconst char *features;\n \t\tstruct object_id oid_buf;\n \t\tconst char *arg;\n+\t\tint feature_len;\n \n \t\treset_timeout(data->timeout);\n \t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n@@ -1110,6 +1111,12 @@ static void receive_needs(struct upload_pack_data *data,\n \t\tif (data->allow_filter &&\n \t\t parse_feature_request(features, \"filter\"))\n \t\t\tdata->filter_capability_requested = 1;\n+\t\tif ((arg = parse_feature_value(features, \"trace2-sid\", &feature_len, 0)))\n+\t\t{\n+\t\t\tchar *client_sid = xstrndup(arg, feature_len);\n+\t\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n+\t\t\tfree(client_sid);\n+\t\t}\n \n \t\to = parse_object(the_repository, &oid_buf);\n \t\tif (!o) {\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408676","messageId":"609958bd1a308c63cc1d5a526b6206745f390a45.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH 09/10] send-pack: advertise trace2 SID in capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:39Z","receivedAt":"2020-10-29T21:33:04Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When trace2 is enabled, the server sent a trace2-sid capability, and\ntrace2.advertiseSID is true, advertise send-pack's own session ID back\nto the server.\n\nSigned-off-by: Josh Steadmon \n---\n send-pack.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex eb4a44270b..56c6740e65 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -16,6 +16,7 @@\n #include \"gpg-interface.h\"\n #include \"cache.h\"\n #include \"shallow.h\"\n+#include \"trace2/tr2_sid.h\"\n \n int option_parse_push_signed(const struct option *opt,\n \t\t\t const char *arg, int unset)\n@@ -424,6 +425,8 @@ int send_pack(struct send_pack_args *args,\n \tint use_sideband = 0;\n \tint quiet_supported = 0;\n \tint agent_supported = 0;\n+\tint server_sent_trace2_sid = 0;\n+\tint advertise_trace2_sid = 0;\n \tint use_atomic = 0;\n \tint atomic_supported = 0;\n \tint use_push_options = 0;\n@@ -435,6 +438,8 @@ int send_pack(struct send_pack_args *args,\n \tconst char *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n \n+\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n+\n \t/* Does the other end support the reporting? */\n \tif (server_supports(\"report-status-v2\"))\n \t\tstatus_report = 2;\n@@ -450,6 +455,8 @@ int send_pack(struct send_pack_args *args,\n \t\tquiet_supported = 1;\n \tif (server_supports(\"agent\"))\n \t\tagent_supported = 1;\n+\tif (server_supports(\"trace2-sid\"))\n+\t\tserver_sent_trace2_sid = 1;\n \tif (server_supports(\"no-thin\"))\n \t\targs->use_thin_pack = 0;\n \tif (server_supports(\"atomic\"))\n@@ -506,6 +513,8 @@ int send_pack(struct send_pack_args *args,\n \t\tstrbuf_addf(&cap_buf, \" object-format=%s\", the_hash_algo->name);\n \tif (agent_supported)\n \t\tstrbuf_addf(&cap_buf, \" agent=%s\", git_user_agent_sanitized());\n+\tif (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())\n+\t\tstrbuf_addf(&cap_buf, \" trace2-sid=%s\", tr2_sid_get());\n \n \t/*\n \t * NEEDSWORK: why does delete-refs have to be so specific to\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408677","messageId":"3bc412fc390afd0af1d6700609f899064b6d2844.1604006121.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH 10/10] receive-pack: log received client trace2 SID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-10-29T21:32:40Z","receivedAt":"2020-10-29T21:33:06Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When receive-pack receives a trace2-sid capability from the client, log\nthe received session ID via a trace2 data event.\n\nSigned-off-by: Josh Steadmon \n---\n builtin/receive-pack.c | 7 +++++++\n t/t5705-trace2-sid-in-capabilities.sh | 20 ++++++++++++++------\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 1ff83c874b..34199e4933 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2040,6 +2040,7 @@ static struct command *read_head_info(struct packet_reader *reader,\n \t\tif (linelen < reader->pktlen) {\n \t\t\tconst char *feature_list = reader->line + linelen + 1;\n \t\t\tconst char *hash = NULL;\n+\t\t\tconst char *client_trace2_sid;\n \t\t\tint len = 0;\n \t\t\tif (parse_feature_request(feature_list, \"report-status\"))\n \t\t\t\treport_status = 1;\n@@ -2062,6 +2063,12 @@ static struct command *read_head_info(struct packet_reader *reader,\n \t\t\t}\n \t\t\tif (xstrncmpz(the_hash_algo->name, hash, len))\n \t\t\t\tdie(\"error: unsupported object format '%s'\", hash);\n+\t\t\tclient_trace2_sid = parse_feature_value(feature_list, \"trace2-sid\", &len, NULL);\n+\t\t\tif (client_trace2_sid) {\n+\t\t\t\tchar *client_sid = xstrndup(client_trace2_sid, len);\n+\t\t\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n+\t\t\t\tfree(client_sid);\n+\t\t\t}\n \t\t}\n \n \t\tif (!strcmp(reader->line, \"push-cert\")) {\ndiff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh\nindex 3fad9462d3..15f78cb7d2 100755\n--- a/t/t5705-trace2-sid-in-capabilities.sh\n+++ b/t/t5705-trace2-sid-in-capabilities.sh\n@@ -28,13 +28,17 @@ do\n \t'\n \n \ttest_expect_success \"trace2 session IDs not advertised by default (push v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n+\t\ttest_when_finished \"git -C local push --delete origin new-branch\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tgit -C local pull --no-rebase origin &&\n \t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local push origin &&\n-\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t\tgit -c protocol.version=$PROTO -C local push \\\n+\t\t\t--receive-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-receive-pack\" \\\n+\t\t\torigin HEAD:new-branch &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\" &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"client-sid\\\" tr2-server-events)\"\n \t'\n done\n \n@@ -58,13 +62,17 @@ do\n \t'\n \n \ttest_expect_success \"trace2 session IDs advertised (push v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n+\t\ttest_when_finished \"git -C local push --delete origin new-branch\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tgit -C local pull --no-rebase origin &&\n \t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local push origin &&\n-\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t\tgit -c protocol.version=$PROTO -C local push \\\n+\t\t\t--receive-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-receive-pack\" \\\n+\t\t\torigin HEAD:new-branch &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events &&\n+\t\tgrep \\\"key\\\":\\\"client-sid\\\" tr2-server-events\n \t'\n done\n \n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408755","messageId":"4f1a1bab-7ac7-b8dd-acb2-6aeb04be3171@jeffhostetler.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2020-10-30T15:54:41Z","receivedAt":"2020-10-30T15:54:45Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 10/29/20 5:32 PM, Josh Steadmon wrote:\n> In order to more easily debug remote operations, it is useful to be able\n> to inspect both client-side and server-side traces. This series allows\n> clients to record the server's trace2 session ID, and vice versa, by\n> advertising the SID in a new \"trace2-sid\" protocol capability.\n> \n\nVery nice! This should be very helpful when matching up client and\nserver commands.\n\n\n> Two questions in particular for reviewers:\n> \n> 1) Is trace2/tr2_sid.h intended to be visible to the rest of the code,\n> or is the trace2/ directory supposed to be opaque implementation\n> detail? If the latter, would it be acceptable to move tr2_sid_get()\n> into trace2.h?\n\nI put all the trace2-private stuff in that sub-directory and gave\neverything tr2_ prefixes to hide the details as much as I could\n(and as an alternative to the usual tendency of having everything\nbe static within a massive .c file).\n\nSo, yeah, my intent was to make all of it opaque.\nSo that just trace2.h contains the official API.\n\nPerhaps in trace2.c you could create:\n\nconst char *trace2_session_id(void)\n{\n return tr2_sid_get();\n}\n\n\n> \n> 2) upload-pack generally takes configuration via flags rather than\n> gitconfig. From offline discussions, it sounds like this is an\n> intentional choice to limit potential vulnerability from malicious\n> configs in local repositories accessed via the file:// URL scheme. Is\n> it reasonable to load the trace2.announceSID option from config files\n> in upload-pack, or should this be changed to a flag?\n\nI don't have the history to comment on this.\n\nOne thing to consider is that the SID for a Git process is built up\nfrom the SID of the parent and the unique data for the current process.\nSo for example, if `git fetch` has SID `` and it spawns\n`git upload-pack`, the child will have SID `/` and if that\nspawns `git index-pack`, that child will have `//`.\nThis is very helpful when tracking down parent/child relationships\nand perf hunting.\n\nThis SID inheritance is passed via an environment variable to\nthe child, which extends it and passes the longer version to its\nchildren.\n\nSo the value being passed between client and server over the\nprotocol may look like `//` rather than just a\nsingle `` term. For your purposes, do you want or care if\nyou get the single term or the full SID ?\n\nAlso, there's nothing to stop someone from seeding that environment\nvariable in their shell with some mischief before launching the\ntop-level Git command. So the above example might see the SID as\n`///`. I'm not sure if this could be\nabused when inserted into the V0/V1/V2 protocol or your logging\nfacility.\n\n $ GIT_TRACE2_EVENT=1 GIT_TRACE2_PARENT_SID=hello git version\n {\"event\":\"version\",\"sid\":\"hello/20201030T154143.660608Z-H86606a97- \n P00001d30\",...}\n ...\n\nSo maybe we want to have a public API to return a pointer to just\nthe final `` term ? (Then again, maybe I worry too much.)\n\n\nThanks,\nJeff\n\n\n> \n> \n> \n> Josh Steadmon (10):\n> docs: new capability to advertise trace2 SIDs\n> docs: new trace2.advertiseSID option\n> upload-pack: advertise trace2 SID in v0 capabilities\n> receive-pack: advertise trace2 SID in v0 capabilities\n> serve: advertise trace2 SID in v2 capabilities\n> transport: log received server trace2 SID\n> fetch-pack: advertise trace2 SID in capabilities\n> upload-pack, serve: log received client trace2 SID\n> send-pack: advertise trace2 SID in capabilities\n> receive-pack: log received client trace2 SID\n> \n> Documentation/config/trace2.txt | 4 +\n> .../technical/protocol-capabilities.txt | 13 ++-\n> Documentation/technical/protocol-v2.txt | 9 +++\n> builtin/receive-pack.c | 16 ++++\n> fetch-pack.c | 11 +++\n> send-pack.c | 9 +++\n> serve.c | 19 +++++\n> t/t5705-trace2-sid-in-capabilities.sh | 79 +++++++++++++++++++\n> transport.c | 10 +++\n> upload-pack.c | 23 +++++-\n> 10 files changed, 190 insertions(+), 3 deletions(-)\n> create mode 100755 t/t5705-trace2-sid-in-capabilities.sh\n> \n"},{"id":"408774","messageId":"xmqq4kmb5osi.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-30T22:31:25Z","receivedAt":"2020-10-30T22:31:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> 2) upload-pack generally takes configuration via flags rather than\n> gitconfig. From offline discussions, it sounds like this is an\n> intentional choice to limit potential vulnerability from malicious\n> configs in local repositories accessed via the file:// URL scheme. Is\n> it reasonable to load the trace2.announceSID option from config files\n> in upload-pack, or should this be changed to a flag?\n\nI do not know about your offline discussion, but it certainly would\nmake it easier to reason about the attack surface if we know it\nnever gets affected by any configuration files.\n\nHaving said that, upload-pack.c::upload_pack_config() already reads\na lot from the configuration file, many of these variables are named\n\"allowSomething\", so...\n\nIOW, I do not see why the announceSID (should it be in trace2.*\nhierarchy, though?) needs to be treated in any more sensitive than\nsay uploadpack.allowrefinwant or *.allowfilter variables.\n"},{"id":"408933","messageId":"20201102222020.GA1904687@google.com","threadId":"54527","inReplyTo":"4f1a1bab-7ac7-b8dd-acb2-6aeb04be3171@jeffhostetler.com","subject":"Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:20:20Z","receivedAt":"2020-11-02T22:20:29Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.10.30 11:54, Jeff Hostetler wrote:\n> \n> \n> On 10/29/20 5:32 PM, Josh Steadmon wrote:\n> > In order to more easily debug remote operations, it is useful to be able\n> > to inspect both client-side and server-side traces. This series allows\n> > clients to record the server's trace2 session ID, and vice versa, by\n> > advertising the SID in a new \"trace2-sid\" protocol capability.\n> > \n> \n> Very nice! This should be very helpful when matching up client and\n> server commands.\n> \n> \n> > Two questions in particular for reviewers:\n> > \n> > 1) Is trace2/tr2_sid.h intended to be visible to the rest of the code,\n> > or is the trace2/ directory supposed to be opaque implementation\n> > detail? If the latter, would it be acceptable to move tr2_sid_get()\n> > into trace2.h?\n> \n> I put all the trace2-private stuff in that sub-directory and gave\n> everything tr2_ prefixes to hide the details as much as I could\n> (and as an alternative to the usual tendency of having everything\n> be static within a massive .c file).\n> \n> So, yeah, my intent was to make all of it opaque.\n> So that just trace2.h contains the official API.\n> \n> Perhaps in trace2.c you could create:\n> \n> const char *trace2_session_id(void)\n> {\n> return tr2_sid_get();\n> }\n\nDone in V2, thanks.\n\n\n> > 2) upload-pack generally takes configuration via flags rather than\n> > gitconfig. From offline discussions, it sounds like this is an\n> > intentional choice to limit potential vulnerability from malicious\n> > configs in local repositories accessed via the file:// URL scheme. Is\n> > it reasonable to load the trace2.announceSID option from config files\n> > in upload-pack, or should this be changed to a flag?\n> \n> I don't have the history to comment on this.\n> \n> One thing to consider is that the SID for a Git process is built up\n> from the SID of the parent and the unique data for the current process.\n> So for example, if `git fetch` has SID `` and it spawns\n> `git upload-pack`, the child will have SID `/` and if that\n> spawns `git index-pack`, that child will have `//`.\n> This is very helpful when tracking down parent/child relationships\n> and perf hunting.\n> \n> This SID inheritance is passed via an environment variable to\n> the child, which extends it and passes the longer version to its\n> children.\n> \n> So the value being passed between client and server over the\n> protocol may look like `//` rather than just a\n> single `` term. For your purposes, do you want or care if\n> you get the single term or the full SID ?\n\nI'm not sure we care too much one way or the other. A single component\nof the SID should be enough to join client & server logs, but it's\neasier to just send the whole thing.\n\n\n> Also, there's nothing to stop someone from seeding that environment\n> variable in their shell with some mischief before launching the\n> top-level Git command. So the above example might see the SID as\n> `///`. I'm not sure if this could be\n> abused when inserted into the V0/V1/V2 protocol or your logging\n> facility.\n> \n> $ GIT_TRACE2_EVENT=1 GIT_TRACE2_PARENT_SID=hello git version\n> {\"event\":\"version\",\"sid\":\"hello/20201030T154143.660608Z-H86606a97-\n> P00001d30\",...}\n> ...\n> \n> So maybe we want to have a public API to return a pointer to just\n> the final `` term ? (Then again, maybe I worry too much.)\n\nYeah, it's certainly possible to muck with the SID as you describe, but\nI'm not sure I see any big problems that could be caused. If someone\npoints out an issue I've missed, I'll be happy to change this, though.\n\n> Thanks,\n> Jeff\n\nThanks for the review!\n\n\n> > Josh Steadmon (10):\n> > docs: new capability to advertise trace2 SIDs\n> > docs: new trace2.advertiseSID option\n> > upload-pack: advertise trace2 SID in v0 capabilities\n> > receive-pack: advertise trace2 SID in v0 capabilities\n> > serve: advertise trace2 SID in v2 capabilities\n> > transport: log received server trace2 SID\n> > fetch-pack: advertise trace2 SID in capabilities\n> > upload-pack, serve: log received client trace2 SID\n> > send-pack: advertise trace2 SID in capabilities\n> > receive-pack: log received client trace2 SID\n> > \n> > Documentation/config/trace2.txt | 4 +\n> > .../technical/protocol-capabilities.txt | 13 ++-\n> > Documentation/technical/protocol-v2.txt | 9 +++\n> > builtin/receive-pack.c | 16 ++++\n> > fetch-pack.c | 11 +++\n> > send-pack.c | 9 +++\n> > serve.c | 19 +++++\n> > t/t5705-trace2-sid-in-capabilities.sh | 79 +++++++++++++++++++\n> > transport.c | 10 +++\n> > upload-pack.c | 23 +++++-\n> > 10 files changed, 190 insertions(+), 3 deletions(-)\n> > create mode 100755 t/t5705-trace2-sid-in-capabilities.sh\n> > \n"},{"id":"408936","messageId":"cover.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH v2 00/11] Advertise trace2 SID in protocol capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:30:58Z","receivedAt":"2020-11-02T22:31:14Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"In order to more easily debug remote operations, it is useful to be able\nto inspect both client-side and server-side traces. This series allows\nclients to record the server's trace2 session ID, and vice versa, by\nadvertising the SID in a new \"trace2-sid\" protocol capability.\n\nChanges since V1:\n* Added a public trace2_session_id() function to trace2.{h,c}. Used this\n in place of tr2_sid_get().\n* Fixed a style issue regarding using NULL rather than 0.\n\nJosh Steadmon (11):\n docs: new capability to advertise trace2 SIDs\n docs: new trace2.advertiseSID option\n trace2: add a public function for getting the SID\n upload-pack: advertise trace2 SID in v0 capabilities\n receive-pack: advertise trace2 SID in v0 capabilities\n serve: advertise trace2 SID in v2 capabilities\n transport: log received server trace2 SID\n fetch-pack: advertise trace2 SID in capabilities\n upload-pack, serve: log received client trace2 SID\n send-pack: advertise trace2 SID in capabilities\n receive-pack: log received client trace2 SID\n\n Documentation/config/trace2.txt | 4 +\n .../technical/protocol-capabilities.txt | 13 ++-\n Documentation/technical/protocol-v2.txt | 9 +++\n builtin/receive-pack.c | 15 ++++\n fetch-pack.c | 10 +++\n send-pack.c | 8 ++\n serve.c | 18 +++++\n t/t5705-trace2-sid-in-capabilities.sh | 79 +++++++++++++++++++\n trace2.c | 5 ++\n trace2.h | 2 +\n transport.c | 10 +++\n upload-pack.c | 22 +++++-\n 12 files changed, 192 insertions(+), 3 deletions(-)\n create mode 100755 t/t5705-trace2-sid-in-capabilities.sh\n\nRange-diff against v1:\n -: ---------- > 1: d04028c3c7 docs: new capability to advertise trace2 SIDs\n -: ---------- > 2: 5d5097b671 docs: new trace2.advertiseSID option\n -: ---------- > 3: 7f42aacd2b trace2: add a public function for getting the SID\n 1: c07bf8aede ! 4: 4912af5f2b upload-pack: advertise trace2 SID in v0 capabilities\n @@ Commit message\n \n ## upload-pack.c ##\n -@@\n - #include \"commit-graph.h\"\n - #include \"commit-reach.h\"\n - #include \"shallow.h\"\n -+#include \"trace2/tr2_sid.h\"\n - \n - /* Remember to update object flag allocation in object.h */\n - #define THEY_HAVE\t(1u << 11)\n @@ upload-pack.c: struct upload_pack_data {\n \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n @@ upload-pack.c: static void format_symref_info(struct strbuf *buf, struct string_\n \n +static void format_trace2_info(struct strbuf *buf, struct upload_pack_data *d) {\n +\tif (d->advertise_trace2_sid && trace2_is_enabled())\n -+\t\tstrbuf_addf(buf, \" trace2-sid=%s\", tr2_sid_get());\n ++\t\tstrbuf_addf(buf, \" trace2-sid=%s\", trace2_session_id());\n +}\n +\n static int send_ref(const char *refname, const struct object_id *oid,\n 2: d42118ac41 ! 5: 7003189c81 receive-pack: advertise trace2 SID in v0 capabilities\n @@ Commit message\n \n ## builtin/receive-pack.c ##\n -@@\n - #include \"commit-reach.h\"\n - #include \"worktree.h\"\n - #include \"shallow.h\"\n -+#include \"trace2/tr2_sid.h\"\n - \n - static const char * const receive_pack_usage[] = {\n - \tN_(\"git receive-pack \"),\n @@ builtin/receive-pack.c: static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int advertise_atomic_push = 1;\n @@ builtin/receive-pack.c: static void show_ref(const char *path, const struct obje\n \t\tif (advertise_push_options)\n \t\t\tstrbuf_addstr(&cap, \" push-options\");\n +\t\tif (advertise_trace2_sid && trace2_is_enabled())\n -+\t\t\tstrbuf_addf(&cap, \" trace2-sid=%s\", tr2_sid_get());\n ++\t\t\tstrbuf_addf(&cap, \" trace2-sid=%s\", trace2_session_id());\n \t\tstrbuf_addf(&cap, \" object-format=%s\", the_hash_algo->name);\n \t\tstrbuf_addf(&cap, \" agent=%s\", git_user_agent_sanitized());\n \t\tpacket_write_fmt(1, \"%s %s%c%s\\n\",\n 3: ff80f70d33 ! 6: 9573428cc0 serve: advertise trace2 SID in v2 capabilities\n @@ Commit message\n \n ## serve.c ##\n @@\n - #include \"ls-refs.h\"\n #include \"serve.h\"\n #include \"upload-pack.h\"\n -+#include \"trace2/tr2_sid.h\"\n -+\n -+static int advertise_trace2_sid;\n \n ++static int advertise_trace2_sid;\n ++\n static int always_advertise(struct repository *r,\n \t\t\t struct strbuf *value)\n + {\n @@ serve.c: static int object_format_advertise(struct repository *r,\n \treturn 1;\n }\n @@ serve.c: static int object_format_advertise(struct repository *r,\n +\tif (!advertise_trace2_sid || !trace2_is_enabled())\n +\t\treturn 0;\n +\tif (value)\n -+\t\tstrbuf_addstr(value, tr2_sid_get());\n ++\t\tstrbuf_addstr(value, trace2_session_id());\n +\treturn 1;\n +}\n +\n 4: 1519767ead = 7: 21bdbf23f3 transport: log received server trace2 SID\n 5: ee4b3a7a55 ! 8: 11b5b1b54f fetch-pack: advertise trace2 SID in capabilities\n @@ Commit message\n \n ## fetch-pack.c ##\n -@@\n - #include \"fetch-negotiator.h\"\n - #include \"fsck.h\"\n - #include \"shallow.h\"\n -+#include \"trace2/tr2_sid.h\"\n - \n - static int transfer_unpack_limit = -1;\n - static int fetch_unpack_limit = -1;\n @@ fetch-pack.c: static int fetch_fsck_objects = -1;\n static int transfer_fsck_objects = -1;\n static int agent_supported;\n @@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,\n \t\t\tif (agent_supported) strbuf_addf(&c, \" agent=%s\",\n \t\t\t\t\t\t\t git_user_agent_sanitized());\n +\t\t\tif (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())\n -+\t\t\t\tstrbuf_addf(&c, \" trace2-sid=%s\", tr2_sid_get());\n ++\t\t\t\tstrbuf_addf(&c, \" trace2-sid=%s\", trace2_session_id());\n \t\t\tif (args->filter_options.choice)\n \t\t\t\tstrbuf_addstr(&c, \" filter\");\n \t\t\tpacket_buf_write(&req_buf, \"want %s%s\\n\", remote_hex, c.buf);\n @@ fetch-pack.c: static int send_fetch_request(struct fetch_negotiator *negotiator,\n \tif (server_supports_v2(\"agent\", 0))\n \t\tpacket_buf_write(&req_buf, \"agent=%s\", git_user_agent_sanitized());\n +\tif (advertise_trace2_sid && server_supports_v2(\"trace2-sid\", 0) && trace2_is_enabled())\n -+\t\tpacket_buf_write(&req_buf, \"trace2-sid=%s\", tr2_sid_get());\n ++\t\tpacket_buf_write(&req_buf, \"trace2-sid=%s\", trace2_session_id());\n \tif (args->server_options && args->server_options->nr &&\n \t server_supports_v2(\"server-option\", 1)) {\n \t\tint i;\n 6: b160941c65 ! 9: 23f44bc904 upload-pack, serve: log received client trace2 SID\n @@ upload-pack.c: static void receive_needs(struct upload_pack_data *data,\n \t\tif (data->allow_filter &&\n \t\t parse_feature_request(features, \"filter\"))\n \t\t\tdata->filter_capability_requested = 1;\n -+\t\tif ((arg = parse_feature_value(features, \"trace2-sid\", &feature_len, 0)))\n ++\t\tif ((arg = parse_feature_value(features, \"trace2-sid\", &feature_len, NULL)))\n +\t\t{\n +\t\t\tchar *client_sid = xstrndup(arg, feature_len);\n +\t\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n 7: 609958bd1a ! 10: c0b1ffc6d8 send-pack: advertise trace2 SID in capabilities\n @@ Commit message\n \n ## send-pack.c ##\n -@@\n - #include \"gpg-interface.h\"\n - #include \"cache.h\"\n - #include \"shallow.h\"\n -+#include \"trace2/tr2_sid.h\"\n - \n - int option_parse_push_signed(const struct option *opt,\n - \t\t\t const char *arg, int unset)\n @@ send-pack.c: int send_pack(struct send_pack_args *args,\n \tint use_sideband = 0;\n \tint quiet_supported = 0;\n @@ send-pack.c: int send_pack(struct send_pack_args *args,\n \tif (agent_supported)\n \t\tstrbuf_addf(&cap_buf, \" agent=%s\", git_user_agent_sanitized());\n +\tif (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())\n -+\t\tstrbuf_addf(&cap_buf, \" trace2-sid=%s\", tr2_sid_get());\n ++\t\tstrbuf_addf(&cap_buf, \" trace2-sid=%s\", trace2_session_id());\n \n \t/*\n \t * NEEDSWORK: why does delete-refs have to be so specific to\n 8: 3bc412fc39 = 11: c47eddd9df receive-pack: log received client trace2 SID\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408937","messageId":"5d5097b67109554e0763724633810ea616b5e2b2.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 02/11] docs: new trace2.advertiseSID option","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:31:00Z","receivedAt":"2020-11-02T22:31:18Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Document a new config option that allows users to determine whether or\nnot to advertise their trace2 session IDs to remote Git clients and\nservers.\n\nSigned-off-by: Josh Steadmon \n---\n Documentation/config/trace2.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt\nindex 01d3afd8a8..3f2e3b4425 100644\n--- a/Documentation/config/trace2.txt\n+++ b/Documentation/config/trace2.txt\n@@ -69,3 +69,7 @@ trace2.maxFiles::\n \twrite additional traces if we would exceed this many files. Instead,\n \twrite a sentinel file that will block further tracing to this\n \tdirectory. Defaults to 0, which disables this check.\n+\n+trace2.advertiseSID::\n+\tBoolean. When true, client and server processes will advertise their\n+\ttrace2 session IDs to their remote counterpart. Defaults to false.\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408938","messageId":"d04028c3c7574e3ca0f9c1b3d711192ca756158d.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 01/11] docs: new capability to advertise trace2 SIDs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:30:59Z","receivedAt":"2020-11-02T22:31:18Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"In future patches, we will add the ability for Git servers and clients\nto advertise their trace2 session IDs via protocol capabilities. This\nallows for easier debugging when both client and server logs are\navailable.\n\nSigned-off-by: Josh Steadmon \n---\n Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--\n Documentation/technical/protocol-v2.txt | 9 +++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex ba869a7d36..73d4ee7f27 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -27,8 +27,8 @@ and 'push-cert' capabilities are sent and recognized by the receive-pack\n (push to server) process.\n \n The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized\n-by both upload-pack and receive-pack protocols. The 'agent' capability\n-may optionally be sent in both protocols.\n+by both upload-pack and receive-pack protocols. The 'agent' and 'trace2-sid'\n+capabilities may optionally be sent in both protocols.\n \n All other capabilities are only recognized by the upload-pack (fetch\n from server) process.\n@@ -365,3 +365,12 @@ If the upload-pack server advertises the 'filter' capability,\n fetch-pack may send \"filter\" commands to request a partial clone\n or partial fetch and request that the server omit various objects\n from the packfile.\n+\n+trace2-sid=\n+-----------------------\n+\n+If trace2 tracing is enabled on the server, it may advertise its session ID via\n+this capability. The client may choose to log the server's session ID in its\n+trace logs, and advertise its own session ID back to the server for it to log\n+as well. This allows for easier debugging of remote sessions when both client\n+and server logs are available.\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex e597b74da3..a5b9ef04f6 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -492,3 +492,12 @@ form `object-format=X`) to notify the client that the server is able to deal\n with objects using hash algorithm X. If not specified, the server is assumed to\n only handle SHA-1. If the client would like to use a hash algorithm other than\n SHA-1, it should specify its object-format string.\n+\n+trace2-sid=\n+~~~~~~~~~~~~~~~~~~~~~~~\n+\n+If trace2 tracing is enabled on the server, it may advertise its session ID via\n+this capability. The client may choose to log the server's session ID in its\n+trace logs, and advertise its own session ID back to the server for it to log\n+as well. This allows for easier debugging of remote sessions when both client\n+and server logs are available.\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408939","messageId":"7f42aacd2bbfdfe40b46bb024ec2eed2e61d027a.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 03/11] trace2: add a public function for getting the SID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:31:01Z","receivedAt":"2020-11-02T22:31:22Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Add a public wrapper, trace2_session_id(), around tr2_sid_get(), which\nis intended to be private trace2 implementation.\n\nSigned-off-by: Josh Steadmon \n---\n trace2.c | 5 +++++\n trace2.h | 2 ++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/trace2.c b/trace2.c\nindex 2c6b570077..256120c7fd 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -792,3 +792,8 @@ void trace2_printf(const char *fmt, ...)\n \tva_end(ap);\n }\n #endif\n+\n+const char *trace2_session_id(void)\n+{\n+\treturn tr2_sid_get();\n+}\ndiff --git a/trace2.h b/trace2.h\nindex b18bc5529c..ede18c2e06 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -500,4 +500,6 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason);\n \t} while (0)\n #endif\n \n+const char *trace2_session_id(void);\n+\n #endif /* TRACE2_H */\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408940","messageId":"7003189c81b7642922eb73bac9b2a688c014a8bb.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 05/11] receive-pack: advertise trace2 SID in v0 capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:31:03Z","receivedAt":"2020-11-02T22:31:23Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When trace2 is enabled and trace2.advertiseSID is true, advertise\nreceive-pack's trace2 session ID via the new trace2-sid capability.\n\nSigned-off-by: Josh Steadmon \n---\n builtin/receive-pack.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bb9909c52e..2fe5a8b4fd 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -54,6 +54,7 @@ static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int advertise_atomic_push = 1;\n static int advertise_push_options;\n+static int advertise_trace2_sid;\n static int unpack_limit = 100;\n static off_t max_input_size;\n static int report_status;\n@@ -248,6 +249,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"trace2.advertisesid\") == 0) {\n+\t\tadvertise_trace2_sid = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -268,6 +274,8 @@ static void show_ref(const char *path, const struct object_id *oid)\n \t\t\tstrbuf_addf(&cap, \" push-cert=%s\", push_cert_nonce);\n \t\tif (advertise_push_options)\n \t\t\tstrbuf_addstr(&cap, \" push-options\");\n+\t\tif (advertise_trace2_sid && trace2_is_enabled())\n+\t\t\tstrbuf_addf(&cap, \" trace2-sid=%s\", trace2_session_id());\n \t\tstrbuf_addf(&cap, \" object-format=%s\", the_hash_algo->name);\n \t\tstrbuf_addf(&cap, \" agent=%s\", git_user_agent_sanitized());\n \t\tpacket_write_fmt(1, \"%s %s%c%s\\n\",\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408941","messageId":"4912af5f2b949b9944b37843a9ebabdd33e66215.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 04/11] upload-pack: advertise trace2 SID in v0 capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:31:02Z","receivedAt":"2020-11-02T22:31:25Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When trace2 is enabled and trace2.advertiseSID is true, advertise\nupload-pack's trace2 session ID via the new trace2-sid capability.\n\nSigned-off-by: Josh Steadmon \n---\n upload-pack.c | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b858eb457..3bb01c5427 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -110,6 +110,7 @@ struct upload_pack_data {\n \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n+\tunsigned advertise_trace2_sid : 1;\n };\n \n static void upload_pack_data_init(struct upload_pack_data *data)\n@@ -141,6 +142,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tpacket_writer_init(&data->writer, 1);\n \n \tdata->keepalive = 5;\n+\tdata->advertise_trace2_sid = 0;\n }\n \n static void upload_pack_data_clear(struct upload_pack_data *data)\n@@ -1178,6 +1180,11 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)\n \t\tstrbuf_addf(buf, \" symref=%s:%s\", item->string, (char *)item->util);\n }\n \n+static void format_trace2_info(struct strbuf *buf, struct upload_pack_data *d) {\n+\tif (d->advertise_trace2_sid && trace2_is_enabled())\n+\t\tstrbuf_addf(buf, \" trace2-sid=%s\", trace2_session_id());\n+}\n+\n static int send_ref(const char *refname, const struct object_id *oid,\n \t\t int flag, void *cb_data)\n {\n@@ -1193,9 +1200,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \n \tif (capabilities) {\n \t\tstruct strbuf symref_info = STRBUF_INIT;\n+\t\tstruct strbuf trace2_info = STRBUF_INIT;\n \n \t\tformat_symref_info(&symref_info, &data->symref);\n-\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n+\t\tformat_trace2_info(&trace2_info, data);\n+\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n \t\t\t (data->allow_uor & ALLOW_TIP_SHA1) ?\n@@ -1205,9 +1214,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\t\t data->stateless_rpc ? \" no-done\" : \"\",\n \t\t\t symref_info.buf,\n \t\t\t data->allow_filter ? \" filter\" : \"\",\n+\t\t\t trace2_info.buf,\n \t\t\t the_hash_algo->name,\n \t\t\t git_user_agent_sanitized());\n \t\tstrbuf_release(&symref_info);\n+\t\tstrbuf_release(&trace2_info);\n \t} else {\n \t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n@@ -1299,6 +1310,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)\n \t\tdata->allow_sideband_all = git_config_bool(var, value);\n \t} else if (!strcmp(\"core.precomposeunicode\", var)) {\n \t\tprecomposed_unicode = git_config_bool(var, value);\n+\t} else if (!strcmp(\"trace2.advertisesid\", var)) {\n+\t\tdata->advertise_trace2_sid = git_config_bool(var, value);\n \t}\n \n \tif (current_config_scope() != CONFIG_SCOPE_LOCAL &&\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408942","messageId":"9573428cc02111d3141ec2481bf679b31a5139e2.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 06/11] serve: advertise trace2 SID in v2 capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:31:04Z","receivedAt":"2020-11-02T22:31:26Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When trace2 is enabled and trace2.advertiseSID is true, advertise the\nserver's trace2 session ID for all protocol v2 connections via the new\ntrace2-sid capability.\n\nSigned-off-by: Josh Steadmon \n---\n serve.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/serve.c b/serve.c\nindex f6341206c4..b40d7aad34 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -8,6 +8,8 @@\n #include \"serve.h\"\n #include \"upload-pack.h\"\n \n+static int advertise_trace2_sid;\n+\n static int always_advertise(struct repository *r,\n \t\t\t struct strbuf *value)\n {\n@@ -30,6 +32,15 @@ static int object_format_advertise(struct repository *r,\n \treturn 1;\n }\n \n+static int trace2_advertise(struct repository *r, struct strbuf *value)\n+{\n+\tif (!advertise_trace2_sid || !trace2_is_enabled())\n+\t\treturn 0;\n+\tif (value)\n+\t\tstrbuf_addstr(value, trace2_session_id());\n+\treturn 1;\n+}\n+\n struct protocol_capability {\n \t/*\n \t * The name of the capability. The server uses this name when\n@@ -66,6 +77,7 @@ static struct protocol_capability capabilities[] = {\n \t{ \"fetch\", upload_pack_advertise, upload_pack_v2 },\n \t{ \"server-option\", always_advertise, NULL },\n \t{ \"object-format\", object_format_advertise, NULL },\n+\t{ \"trace2-sid\", trace2_advertise, NULL },\n };\n \n static void advertise_capabilities(void)\n@@ -261,6 +273,8 @@ static int process_request(void)\n /* Main serve loop for protocol version 2 */\n void serve(struct serve_options *options)\n {\n+\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n+\n \tif (options->advertise_capabilities || !options->stateless_rpc) {\n \t\t/* serve by default supports v2 */\n \t\tpacket_write_fmt(1, \"version 2\\n\");\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408943","messageId":"21bdbf23f39c800b1722c54b666df7a91b5879b5.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 07/11] transport: log received server trace2 SID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:31:05Z","receivedAt":"2020-11-02T22:31:28Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When a client receives a trace2-sid capability from a protocol v0, v1,\nor v2 server, log the received session ID via a trace2 data event.\n\nSigned-off-by: Josh Steadmon \n---\n t/t5705-trace2-sid-in-capabilities.sh | 64 +++++++++++++++++++++++++++\n transport.c | 10 +++++\n 2 files changed, 74 insertions(+)\n create mode 100755 t/t5705-trace2-sid-in-capabilities.sh\n\ndiff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh\nnew file mode 100755\nindex 0000000000..0870e78f7c\n--- /dev/null\n+++ b/t/t5705-trace2-sid-in-capabilities.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='trace2 SID in capabilities'\n+\n+. ./test-lib.sh\n+\n+REPO=\"$(pwd)/repo\"\n+LOCAL_PRISTINE=\"$(pwd)/local_pristine\"\n+\n+test_expect_success 'setup repos for trace2 SID capability tests' '\n+\tgit init \"$REPO\" &&\n+\ttest_commit -C \"$REPO\" a &&\n+\tgit clone \"file://$REPO\" \"$LOCAL_PRISTINE\" &&\n+\ttest_commit -C \"$REPO\" b\n+'\n+\n+for PROTO in 0 1 2\n+do\n+\ttest_expect_success \"trace2 session IDs not advertised by default (fetch v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t'\n+\n+\ttest_expect_success \"trace2 session IDs not advertised by default (push v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tgit -C local pull --no-rebase origin &&\n+\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local push origin &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t'\n+done\n+\n+test_expect_success 'enable SID advertisement' '\n+\tgit -C \"$REPO\" config trace2.advertiseSID true &&\n+\tgit -C \"$LOCAL_PRISTINE\" config trace2.advertiseSID true\n+'\n+\n+for PROTO in 0 1 2\n+do\n+\ttest_expect_success \"trace2 session IDs advertised (fetch v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t'\n+\n+\ttest_expect_success \"trace2 session IDs advertised (push v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tgit -C local pull --no-rebase origin &&\n+\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local push origin &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t'\n+done\n+\n+test_done\ndiff --git a/transport.c b/transport.c\nindex 47da955e4f..d16be597bd 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -286,6 +286,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \tstruct git_transport_data *data = transport->data;\n \tstruct ref *refs = NULL;\n \tstruct packet_reader reader;\n+\tint sid_len;\n+\tconst char *server_trace2_sid;\n \n \tconnect_setup(transport, for_push);\n \n@@ -297,6 +299,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \tdata->version = discover_version(&reader);\n \tswitch (data->version) {\n \tcase protocol_v2:\n+\t\tif (server_feature_v2(\"trace2-sid\", &server_trace2_sid))\n+\t\t\ttrace2_data_string(\"trace2\", NULL, \"server-sid\", server_trace2_sid);\n \t\tif (must_list_refs)\n \t\t\tget_remote_refs(data->fd[1], &reader, &refs, for_push,\n \t\t\t\t\tref_prefixes,\n@@ -310,6 +314,12 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \t\t\t\t for_push ? REF_NORMAL : 0,\n \t\t\t\t &data->extra_have,\n \t\t\t\t &data->shallow);\n+\t\tserver_trace2_sid = server_feature_value(\"trace2-sid\", &sid_len);\n+\t\tif (server_trace2_sid) {\n+\t\t\tchar *server_sid = xstrndup(server_trace2_sid, sid_len);\n+\t\t\ttrace2_data_string(\"trace2\", NULL, \"server-sid\", server_sid);\n+\t\t\tfree(server_sid);\n+\t\t}\n \t\tbreak;\n \tcase protocol_unknown_version:\n \t\tBUG(\"unknown protocol version\");\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408944","messageId":"11b5b1b54f14354f08c9eb230d5b4e6a3de1996b.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 08/11] fetch-pack: advertise trace2 SID in capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:31:06Z","receivedAt":"2020-11-02T22:31:30Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When trace2 is enabled, the server sent a trace2-sid capability, and\ntrace2.advertiseSID is true, advertise fetch-pack's own session ID back\nto the server.\n\nSigned-off-by: Josh Steadmon \n---\n fetch-pack.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b10c432315..7fbefa7b8e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -35,6 +35,8 @@ static int fetch_fsck_objects = -1;\n static int transfer_fsck_objects = -1;\n static int agent_supported;\n static int server_supports_filtering;\n+static int server_sent_trace2_sid;\n+static int advertise_trace2_sid;\n static struct shallow_lock shallow_lock;\n static const char *alternate_shallow_file;\n static struct strbuf fsck_msg_types = STRBUF_INIT;\n@@ -326,6 +328,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\tif (deepen_not_ok) strbuf_addstr(&c, \" deepen-not\");\n \t\t\tif (agent_supported) strbuf_addf(&c, \" agent=%s\",\n \t\t\t\t\t\t\t git_user_agent_sanitized());\n+\t\t\tif (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())\n+\t\t\t\tstrbuf_addf(&c, \" trace2-sid=%s\", trace2_session_id());\n \t\t\tif (args->filter_options.choice)\n \t\t\t\tstrbuf_addstr(&c, \" filter\");\n \t\t\tpacket_buf_write(&req_buf, \"want %s%s\\n\", remote_hex, c.buf);\n@@ -979,6 +983,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t agent_len, agent_feature);\n \t}\n \n+\tif (server_supports(\"trace2-sid\"))\n+\t\tserver_sent_trace2_sid = 1;\n+\n \tif (server_supports(\"shallow\"))\n \t\tprint_verbose(args, _(\"Server supports %s\"), \"shallow\");\n \telse if (args->depth > 0 || is_repository_shallow(r))\n@@ -1191,6 +1198,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tpacket_buf_write(&req_buf, \"command=fetch\");\n \tif (server_supports_v2(\"agent\", 0))\n \t\tpacket_buf_write(&req_buf, \"agent=%s\", git_user_agent_sanitized());\n+\tif (advertise_trace2_sid && server_supports_v2(\"trace2-sid\", 0) && trace2_is_enabled())\n+\t\tpacket_buf_write(&req_buf, \"trace2-sid=%s\", trace2_session_id());\n \tif (args->server_options && args->server_options->nr &&\n \t server_supports_v2(\"server-option\", 1)) {\n \t\tint i;\n@@ -1711,6 +1720,7 @@ static void fetch_pack_config(void)\n \tgit_config_get_bool(\"repack.usedeltabaseoffset\", &prefer_ofs_delta);\n \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n+\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n \tif (!uri_protocols.nr) {\n \t\tchar *str;\n \n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408945","messageId":"23f44bc904dfdd31f68a27c587b4b61df4bc0041.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 09/11] upload-pack, serve: log received client trace2 SID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:31:07Z","receivedAt":"2020-11-02T22:31:32Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When upload-pack (protocol v0/v1) or a protocol v2 server receives a\ntrace2-sid capability from a client, log the received session ID via a\ntrace2 data event.\n\nSigned-off-by: Josh Steadmon \n---\n serve.c | 4 ++++\n t/t5705-trace2-sid-in-capabilities.sh | 19 +++++++++++++------\n upload-pack.c | 7 +++++++\n 3 files changed, 24 insertions(+), 6 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex b40d7aad34..ffe5ba3f8a 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -201,6 +201,7 @@ static int process_request(void)\n \tstruct packet_reader reader;\n \tstruct strvec keys = STRVEC_INIT;\n \tstruct protocol_capability *command = NULL;\n+\tconst char *client_sid;\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -264,6 +265,9 @@ static int process_request(void)\n \n \tcheck_algorithm(the_repository, &keys);\n \n+\tif (has_capability(&keys, \"trace2-sid\", &client_sid))\n+\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n+\n \tcommand->command(the_repository, &keys, &reader);\n \n \tstrvec_clear(&keys);\ndiff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh\nindex 0870e78f7c..3fad9462d3 100755\n--- a/t/t5705-trace2-sid-in-capabilities.sh\n+++ b/t/t5705-trace2-sid-in-capabilities.sh\n@@ -17,11 +17,14 @@ test_expect_success 'setup repos for trace2 SID capability tests' '\n for PROTO in 0 1 2\n do\n \ttest_expect_success \"trace2 session IDs not advertised by default (fetch v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n-\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t\tgit -c protocol.version=$PROTO -C local fetch \\\n+\t\t\t--upload-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-upload-pack\" \\\n+\t\t\torigin &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\" &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"client-sid\\\" tr2-server-events)\"\n \t'\n \n \ttest_expect_success \"trace2 session IDs not advertised by default (push v${PROTO})\" '\n@@ -43,11 +46,15 @@ test_expect_success 'enable SID advertisement' '\n for PROTO in 0 1 2\n do\n \ttest_expect_success \"trace2 session IDs advertised (fetch v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tgit -C local config trace2.advertiseSID true &&\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n-\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t\tgit -c protocol.version=$PROTO -C local fetch \\\n+\t\t\t--upload-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-upload-pack\" \\\n+\t\t\torigin &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events &&\n+\t\tgrep \\\"key\\\":\\\"client-sid\\\" tr2-server-events\n \t'\n \n \ttest_expect_success \"trace2 session IDs advertised (push v${PROTO})\" '\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3bb01c5427..d938603c97 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1058,6 +1058,7 @@ static void receive_needs(struct upload_pack_data *data,\n \t\tconst char *features;\n \t\tstruct object_id oid_buf;\n \t\tconst char *arg;\n+\t\tint feature_len;\n \n \t\treset_timeout(data->timeout);\n \t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n@@ -1109,6 +1110,12 @@ static void receive_needs(struct upload_pack_data *data,\n \t\tif (data->allow_filter &&\n \t\t parse_feature_request(features, \"filter\"))\n \t\t\tdata->filter_capability_requested = 1;\n+\t\tif ((arg = parse_feature_value(features, \"trace2-sid\", &feature_len, NULL)))\n+\t\t{\n+\t\t\tchar *client_sid = xstrndup(arg, feature_len);\n+\t\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n+\t\t\tfree(client_sid);\n+\t\t}\n \n \t\to = parse_object(the_repository, &oid_buf);\n \t\tif (!o) {\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408946","messageId":"c0b1ffc6d8ba6be9906a573e7c4d88b29c90d9b4.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 10/11] send-pack: advertise trace2 SID in capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:31:08Z","receivedAt":"2020-11-02T22:31:35Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When trace2 is enabled, the server sent a trace2-sid capability, and\ntrace2.advertiseSID is true, advertise send-pack's own session ID back\nto the server.\n\nSigned-off-by: Josh Steadmon \n---\n send-pack.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex eb4a44270b..7478a3f7cc 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -424,6 +424,8 @@ int send_pack(struct send_pack_args *args,\n \tint use_sideband = 0;\n \tint quiet_supported = 0;\n \tint agent_supported = 0;\n+\tint server_sent_trace2_sid = 0;\n+\tint advertise_trace2_sid = 0;\n \tint use_atomic = 0;\n \tint atomic_supported = 0;\n \tint use_push_options = 0;\n@@ -435,6 +437,8 @@ int send_pack(struct send_pack_args *args,\n \tconst char *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n \n+\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n+\n \t/* Does the other end support the reporting? */\n \tif (server_supports(\"report-status-v2\"))\n \t\tstatus_report = 2;\n@@ -450,6 +454,8 @@ int send_pack(struct send_pack_args *args,\n \t\tquiet_supported = 1;\n \tif (server_supports(\"agent\"))\n \t\tagent_supported = 1;\n+\tif (server_supports(\"trace2-sid\"))\n+\t\tserver_sent_trace2_sid = 1;\n \tif (server_supports(\"no-thin\"))\n \t\targs->use_thin_pack = 0;\n \tif (server_supports(\"atomic\"))\n@@ -506,6 +512,8 @@ int send_pack(struct send_pack_args *args,\n \t\tstrbuf_addf(&cap_buf, \" object-format=%s\", the_hash_algo->name);\n \tif (agent_supported)\n \t\tstrbuf_addf(&cap_buf, \" agent=%s\", git_user_agent_sanitized());\n+\tif (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())\n+\t\tstrbuf_addf(&cap_buf, \" trace2-sid=%s\", trace2_session_id());\n \n \t/*\n \t * NEEDSWORK: why does delete-refs have to be so specific to\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408948","messageId":"c47eddd9df195920d6f8f36d038b79af4aabd293.1604355792.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"[PATCH v2 11/11] receive-pack: log received client trace2 SID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-02T22:31:09Z","receivedAt":"2020-11-02T22:31:38Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When receive-pack receives a trace2-sid capability from the client, log\nthe received session ID via a trace2 data event.\n\nSigned-off-by: Josh Steadmon \n---\n builtin/receive-pack.c | 7 +++++++\n t/t5705-trace2-sid-in-capabilities.sh | 20 ++++++++++++++------\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2fe5a8b4fd..f780a66a52 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2039,6 +2039,7 @@ static struct command *read_head_info(struct packet_reader *reader,\n \t\tif (linelen < reader->pktlen) {\n \t\t\tconst char *feature_list = reader->line + linelen + 1;\n \t\t\tconst char *hash = NULL;\n+\t\t\tconst char *client_trace2_sid;\n \t\t\tint len = 0;\n \t\t\tif (parse_feature_request(feature_list, \"report-status\"))\n \t\t\t\treport_status = 1;\n@@ -2061,6 +2062,12 @@ static struct command *read_head_info(struct packet_reader *reader,\n \t\t\t}\n \t\t\tif (xstrncmpz(the_hash_algo->name, hash, len))\n \t\t\t\tdie(\"error: unsupported object format '%s'\", hash);\n+\t\t\tclient_trace2_sid = parse_feature_value(feature_list, \"trace2-sid\", &len, NULL);\n+\t\t\tif (client_trace2_sid) {\n+\t\t\t\tchar *client_sid = xstrndup(client_trace2_sid, len);\n+\t\t\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n+\t\t\t\tfree(client_sid);\n+\t\t\t}\n \t\t}\n \n \t\tif (!strcmp(reader->line, \"push-cert\")) {\ndiff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh\nindex 3fad9462d3..15f78cb7d2 100755\n--- a/t/t5705-trace2-sid-in-capabilities.sh\n+++ b/t/t5705-trace2-sid-in-capabilities.sh\n@@ -28,13 +28,17 @@ do\n \t'\n \n \ttest_expect_success \"trace2 session IDs not advertised by default (push v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n+\t\ttest_when_finished \"git -C local push --delete origin new-branch\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tgit -C local pull --no-rebase origin &&\n \t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local push origin &&\n-\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t\tgit -c protocol.version=$PROTO -C local push \\\n+\t\t\t--receive-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-receive-pack\" \\\n+\t\t\torigin HEAD:new-branch &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\" &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"client-sid\\\" tr2-server-events)\"\n \t'\n done\n \n@@ -58,13 +62,17 @@ do\n \t'\n \n \ttest_expect_success \"trace2 session IDs advertised (push v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n+\t\ttest_when_finished \"git -C local push --delete origin new-branch\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tgit -C local pull --no-rebase origin &&\n \t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local push origin &&\n-\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t\tgit -c protocol.version=$PROTO -C local push \\\n+\t\t\t--receive-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-receive-pack\" \\\n+\t\t\torigin HEAD:new-branch &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events &&\n+\t\tgrep \\\"key\\\":\\\"client-sid\\\" tr2-server-events\n \t'\n done\n \n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408972","messageId":"xmqqy2jjp80f.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"cover.1604355792.git.steadmon@google.com","subject":"Re: [PATCH v2 00/11] Advertise trace2 SID in protocol capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T01:02:40Z","receivedAt":"2020-11-03T01:02:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> In order to more easily debug remote operations, it is useful to be able\n> to inspect both client-side and server-side traces. This series allows\n> clients to record the server's trace2 session ID, and vice versa, by\n> advertising the SID in a new \"trace2-sid\" protocol capability.\n>\n> Changes since V1:\n> * Added a public trace2_session_id() function to trace2.{h,c}. Used this\n> in place of tr2_sid_get().\n> * Fixed a style issue regarding using NULL rather than 0.\n\nThanks; will replace.\n"},{"id":"409045","messageId":"xmqqa6vyqgoc.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"20201102222020.GA1904687@google.com","subject":"Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T21:22:27Z","receivedAt":"2020-11-03T21:22:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n>> So the value being passed between client and server over the\n>> protocol may look like `//` rather than just a\n>> single `` term. For your purposes, do you want or care if\n>> you get the single term or the full SID ?\n>\n> I'm not sure we care too much one way or the other. A single component\n> of the SID should be enough to join client & server logs, but it's\n> easier to just send the whole thing.\n\nIt may be worth documenting this design decision in the protocol\ndoc; even though protocol doc may say this should be treated as an\nopaque token, people may assume certain structure.\n"},{"id":"409048","messageId":"xmqq5z6mqg60.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"d04028c3c7574e3ca0f9c1b3d711192ca756158d.1604355792.git.steadmon@google.com","subject":"Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T21:33:27Z","receivedAt":"2020-11-03T21:33:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> +trace2-sid=\n> +-----------------------\n> +\n> +If trace2 tracing is enabled on the server, it may advertise its session ID via\n> +this capability. The client may choose to log the server's session ID in its\n> +trace logs, and advertise its own session ID back to the server for it to log\n> +as well. This allows for easier debugging of remote sessions when both client\n> +and server logs are available.\n> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> index e597b74da3..a5b9ef04f6 100644\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -492,3 +492,12 @@ form `object-format=X`) to notify the client that the server is able to deal\n> with objects using hash algorithm X. If not specified, the server is assumed to\n> only handle SHA-1. If the client would like to use a hash algorithm other than\n> SHA-1, it should specify its object-format string.\n> +\n> +trace2-sid=\n> +~~~~~~~~~~~~~~~~~~~~~~~\n> +\n> +If trace2 tracing is enabled on the server, it may advertise its session ID via\n> +this capability. The client may choose to log the server's session ID in its\n> +trace logs, and advertise its own session ID back to the server for it to log\n> +as well. This allows for easier debugging of remote sessions when both client\n> +and server logs are available.\n\nHave we documented what a session-id should look like anywhere in\nthe documentation? This document is to help third-party to write\nimplementations of the protocol, but the above description leaves\nthings \"implementation defined\" a bit too much, I am afraid.\n\nFor example, as this must fit on a single pkt-line as an advertised\ncapability, there would be some length limit. Are there other\ninherent limitations due to our protocol? Are there some artificial\nlimitations that we may want to impose to make it easier to harden\nimplementations against common mistakes? For example are bytes in\n allowed to contain LF, CR, NUL, etc.?\n"},{"id":"409049","messageId":"xmqq1rhaqfqg.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"5d5097b67109554e0763724633810ea616b5e2b2.1604355792.git.steadmon@google.com","subject":"Re: [PATCH v2 02/11] docs: new trace2.advertiseSID option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T21:42:47Z","receivedAt":"2020-11-03T21:42:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Document a new config option that allows users to determine whether or\n> not to advertise their trace2 session IDs to remote Git clients and\n> servers.\n\nI do not think placeing this in the trace2 hierarchy is a good idea.\n\nIt is not like concept of \"session\" belongs to trace2; each\noperation we perform inherently is done on behalf of a session.\nThe trace2 subsystem may have been the first to externalize the\nconcept, but even after trace2 gets superseded, we would want to\nkey our log records with some \"session ID\". After we introduce\nan improved mechanism that is successor to trace2, we still would\nwant to exchange some session ID if the advertiseSID option the\nusers define in their repository today (well, maybe in 3 months\nafter this series lands in a released version and widely\ndeployed), no?\n\nWe are not exposing the session ID anywhere but the transports, so\nhow about calling it transport.advertiseSID, perhaps?\n\nWe also may want to call that just \"session ID\", not \"trace2\nsession ID\" in the description.\n\nThanks.\n\n> Signed-off-by: Josh Steadmon \n> ---\n> Documentation/config/trace2.txt | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt\n> index 01d3afd8a8..3f2e3b4425 100644\n> --- a/Documentation/config/trace2.txt\n> +++ b/Documentation/config/trace2.txt\n> @@ -69,3 +69,7 @@ trace2.maxFiles::\n> \twrite additional traces if we would exceed this many files. Instead,\n> \twrite a sentinel file that will block further tracing to this\n> \tdirectory. Defaults to 0, which disables this check.\n> +\n> +trace2.advertiseSID::\n> +\tBoolean. When true, client and server processes will advertise their\n> +\ttrace2 session IDs to their remote counterpart. Defaults to false.\n"},{"id":"409050","messageId":"xmqqwnz2p0wo.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"4912af5f2b949b9944b37843a9ebabdd33e66215.1604355792.git.steadmon@google.com","subject":"Re: [PATCH v2 04/11] upload-pack: advertise trace2 SID in v0 capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T21:48:23Z","receivedAt":"2020-11-03T21:48:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> When trace2 is enabled and trace2.advertiseSID is true, advertise\n> upload-pack's trace2 session ID via the new trace2-sid capability.\n\nI would have imagined when advertiseSID is enabled, trace2, at least\nthe part that allocates and assigns the session ID, ought to be\nenabled automatically.\n\nBut the above goes in a different direction and requires both to be\nenabled. Any compelling reason behind the choice?\n\nDoes the documentation added by this series make it clear that\nasking for advertiseSID does NOT automatically enable allocation of\nsession IDs (even if it does not explain why it does not happen)?\n\nThanks.\n\n\n"},{"id":"409167","messageId":"xmqqtuu4ltd9.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"9573428cc02111d3141ec2481bf679b31a5139e2.1604355792.git.steadmon@google.com","subject":"Re: [PATCH v2 06/11] serve: advertise trace2 SID in v2 capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T21:11:46Z","receivedAt":"2020-11-04T21:11:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> When trace2 is enabled and trace2.advertiseSID is true, advertise the\n> server's trace2 session ID for all protocol v2 connections via the new\n> trace2-sid capability.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> serve.c | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n>\n> diff --git a/serve.c b/serve.c\n> index f6341206c4..b40d7aad34 100644\n> --- a/serve.c\n> +++ b/serve.c\n> @@ -8,6 +8,8 @@\n> #include \"serve.h\"\n> #include \"upload-pack.h\"\n> \n> +static int advertise_trace2_sid;\n> +\n> static int always_advertise(struct repository *r,\n> \t\t\t struct strbuf *value)\n> {\n> @@ -30,6 +32,15 @@ static int object_format_advertise(struct repository *r,\n> \treturn 1;\n> }\n> \n> +static int trace2_advertise(struct repository *r, struct strbuf *value)\n> +{\n> +\tif (!advertise_trace2_sid || !trace2_is_enabled())\n> +\t\treturn 0;\n> +\tif (value)\n> +\t\tstrbuf_addstr(value, trace2_session_id());\n> +\treturn 1;\n> +}\n\nTo both 05/11 and 06/11, the same \"if the user instructs us to\nadvertise, shouldn't we arrange the thing we were told to advertise\nto be automatically available by at least allocating a session ID,\nif not enabling the entire tracing?\" applies.\n\n> struct protocol_capability {\n> \t/*\n> \t * The name of the capability. The server uses this name when\n> @@ -66,6 +77,7 @@ static struct protocol_capability capabilities[] = {\n> \t{ \"fetch\", upload_pack_advertise, upload_pack_v2 },\n> \t{ \"server-option\", always_advertise, NULL },\n> \t{ \"object-format\", object_format_advertise, NULL },\n> +\t{ \"trace2-sid\", trace2_advertise, NULL },\n> };\n> \n> static void advertise_capabilities(void)\n> @@ -261,6 +273,8 @@ static int process_request(void)\n> /* Main serve loop for protocol version 2 */\n> void serve(struct serve_options *options)\n> {\n> +\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n> +\n> \tif (options->advertise_capabilities || !options->stateless_rpc) {\n> \t\t/* serve by default supports v2 */\n> \t\tpacket_write_fmt(1, \"version 2\\n\");\n"},{"id":"409168","messageId":"xmqqpn4slt8o.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"21bdbf23f39c800b1722c54b666df7a91b5879b5.1604355792.git.steadmon@google.com","subject":"Re: [PATCH v2 07/11] transport: log received server trace2 SID","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T21:14:31Z","receivedAt":"2020-11-04T21:14:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> When a client receives a trace2-sid capability from a protocol v0, v1,\n> or v2 server, log the received session ID via a trace2 data event.\n\nWould this pose a new security threat surface? Just wondering if we\nwant to ignore the capability if it is not enabled on our end with\nthe configuration.\n\nThanks.\n\n> diff --git a/transport.c b/transport.c\n> index 47da955e4f..d16be597bd 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -286,6 +286,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n> \tstruct git_transport_data *data = transport->data;\n> \tstruct ref *refs = NULL;\n> \tstruct packet_reader reader;\n> +\tint sid_len;\n> +\tconst char *server_trace2_sid;\n> \n> \tconnect_setup(transport, for_push);\n> \n> @@ -297,6 +299,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n> \tdata->version = discover_version(&reader);\n> \tswitch (data->version) {\n> \tcase protocol_v2:\n> +\t\tif (server_feature_v2(\"trace2-sid\", &server_trace2_sid))\n> +\t\t\ttrace2_data_string(\"trace2\", NULL, \"server-sid\", server_trace2_sid);\n> \t\tif (must_list_refs)\n> \t\t\tget_remote_refs(data->fd[1], &reader, &refs, for_push,\n> \t\t\t\t\tref_prefixes,\n> @@ -310,6 +314,12 @@ static struct ref *handshake(struct transport *transport, int for_push,\n> \t\t\t\t for_push ? REF_NORMAL : 0,\n> \t\t\t\t &data->extra_have,\n> \t\t\t\t &data->shallow);\n> +\t\tserver_trace2_sid = server_feature_value(\"trace2-sid\", &sid_len);\n> +\t\tif (server_trace2_sid) {\n> +\t\t\tchar *server_sid = xstrndup(server_trace2_sid, sid_len);\n> +\t\t\ttrace2_data_string(\"trace2\", NULL, \"server-sid\", server_sid);\n> +\t\t\tfree(server_sid);\n> +\t\t}\n> \t\tbreak;\n> \tcase protocol_unknown_version:\n> \t\tBUG(\"unknown protocol version\");\n"},{"id":"409169","messageId":"xmqqimaklsvg.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"11b5b1b54f14354f08c9eb230d5b4e6a3de1996b.1604355792.git.steadmon@google.com","subject":"Re: [PATCH v2 08/11] fetch-pack: advertise trace2 SID in capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T21:22:27Z","receivedAt":"2020-11-04T21:22:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> When trace2 is enabled, the server sent a trace2-sid capability, and\n> trace2.advertiseSID is true, advertise fetch-pack's own session ID back\n> to the server.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> fetch-pack.c | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index b10c432315..7fbefa7b8e 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -35,6 +35,8 @@ static int fetch_fsck_objects = -1;\n> static int transfer_fsck_objects = -1;\n> static int agent_supported;\n> static int server_supports_filtering;\n> +static int server_sent_trace2_sid;\n> +static int advertise_trace2_sid;\n> static struct shallow_lock shallow_lock;\n> static const char *alternate_shallow_file;\n> static struct strbuf fsck_msg_types = STRBUF_INIT;\n> @@ -326,6 +328,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n> \t\t\tif (deepen_not_ok) strbuf_addstr(&c, \" deepen-not\");\n> \t\t\tif (agent_supported) strbuf_addf(&c, \" agent=%s\",\n> \t\t\t\t\t\t\t git_user_agent_sanitized());\n> +\t\t\tif (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())\n> +\t\t\t\tstrbuf_addf(&c, \" trace2-sid=%s\", trace2_session_id());\n> \t\t\tif (args->filter_options.choice)\n> \t\t\t\tstrbuf_addstr(&c, \" filter\");\n> \t\t\tpacket_buf_write(&req_buf, \"want %s%s\\n\", remote_hex, c.buf);\n> @@ -979,6 +983,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> \t\t\t\t agent_len, agent_feature);\n> \t}\n> \n> +\tif (server_supports(\"trace2-sid\"))\n> +\t\tserver_sent_trace2_sid = 1;\n> +\n> \tif (server_supports(\"shallow\"))\n> \t\tprint_verbose(args, _(\"Server supports %s\"), \"shallow\");\n> \telse if (args->depth > 0 || is_repository_shallow(r))\n> @@ -1191,6 +1198,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n> \t\tpacket_buf_write(&req_buf, \"command=fetch\");\n> \tif (server_supports_v2(\"agent\", 0))\n> \t\tpacket_buf_write(&req_buf, \"agent=%s\", git_user_agent_sanitized());\n> +\tif (advertise_trace2_sid && server_supports_v2(\"trace2-sid\", 0) && trace2_is_enabled())\n> +\t\tpacket_buf_write(&req_buf, \"trace2-sid=%s\", trace2_session_id());\n> \tif (args->server_options && args->server_options->nr &&\n> \t server_supports_v2(\"server-option\", 1)) {\n> \t\tint i;\n> @@ -1711,6 +1720,7 @@ static void fetch_pack_config(void)\n> \tgit_config_get_bool(\"repack.usedeltabaseoffset\", &prefer_ofs_delta);\n> \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n> \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n> +\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n> \tif (!uri_protocols.nr) {\n> \t\tchar *str;\n\nThe same comment as 05/11 and 06/11 applies to this repeated\nappearance of this boolean expression:\n\n\tadvertise_trace2_sid && trace2_is_enabled()\n\nIf we are committed to stick to the \"even if we were told to\nadvertise, do not alllocate a session ID\" design, perhaps it is\ncleaner to clear advertise_trace2_sid at the very beginning,\nimmediately after we learn that the tracing is disabled and the\nadvertiseSID configuration is read. That way, everybody needs to\nonly care about advertise_trace2_sid variable.\n\nIncidentally, if we decide to change the semantics to auto allocate\nthe session ID if advertiseSID configuration asks us to advertise\n(it is OK if we do not enable the full trace2 suite), we can still\nmake the code only check advertise_trace2_sid variable, without\nadding repeated check of trace2_is_enabled() everywhere at all.\n\n\n"},{"id":"409170","messageId":"xmqqeel8lso1.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"23f44bc904dfdd31f68a27c587b4b61df4bc0041.1604355792.git.steadmon@google.com","subject":"Re: [PATCH v2 09/11] upload-pack, serve: log received client trace2 SID","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T21:26:54Z","receivedAt":"2020-11-04T21:27:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 3bb01c5427..d938603c97 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1058,6 +1058,7 @@ static void receive_needs(struct upload_pack_data *data,\n> \t\tconst char *features;\n> \t\tstruct object_id oid_buf;\n> \t\tconst char *arg;\n> +\t\tint feature_len;\n> \n> \t\treset_timeout(data->timeout);\n> \t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n> @@ -1109,6 +1110,12 @@ static void receive_needs(struct upload_pack_data *data,\n> \t\tif (data->allow_filter &&\n> \t\t parse_feature_request(features, \"filter\"))\n> \t\t\tdata->filter_capability_requested = 1;\n> +\t\tif ((arg = parse_feature_value(features, \"trace2-sid\", &feature_len, NULL)))\n> +\t\t{\n\nStyle. '{' block is opened on the same line as \"if (condition)\"\nends, e.g.\n\n\t\targ = parse_feature_value(...);\n\t\tif (arg) {\n\nAvoid assignment in the condition part of if/while unless there is a\ncompelling reason to do so. e.g.\n\n\t\tif ((arg = ...) && further_check_on(arg)) {\n\t\t\t...\n\nmight be justifiable, but here, I do not see any such reason.\n\n> +\t\t\tchar *client_sid = xstrndup(arg, feature_len);\n> +\t\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n> +\t\t\tfree(client_sid);\n> +\t\t}\n> \n> \t\to = parse_object(the_repository, &oid_buf);\n> \t\tif (!o) {\n\nThanks.\n"},{"id":"409207","messageId":"20201105184455.GA36751@google.com","threadId":"54527","inReplyTo":"xmqqwnz2p0wo.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 04/11] upload-pack: advertise trace2 SID in v0 capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-05T18:44:55Z","receivedAt":"2020-11-05T18:45:04Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.11.03 13:48, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > When trace2 is enabled and trace2.advertiseSID is true, advertise\n> > upload-pack's trace2 session ID via the new trace2-sid capability.\n> \n> I would have imagined when advertiseSID is enabled, trace2, at least\n> the part that allocates and assigns the session ID, ought to be\n> enabled automatically.\n> \n> But the above goes in a different direction and requires both to be\n> enabled. Any compelling reason behind the choice?\n\nMy reasoning was that by advertising the capability, you are telling the\nremote side \"I have definitely produced a log using this session ID. If\nyou need it later, you can find it with this key\". If we advertise a\nsession ID even when trace2 is not enabled, the remote side can't be as\nsure that the received session ID actually points to any useful logs on\nthe other side.\n\nOf course, this is a weak guarantee since a client could send whatever\nit likes regardless of whether anything was logged, or one side could\ndelete or lose its logs before the other decides it needs to view them.\n\nI think your idea in a different subthread about having a general\nsession ID not tied to trace2 is interesting, and would also be a point\nin favor of changing the current behavior here, but I have some thoughts\non that point that I'll add in the other subthread.\n\nI'm still leaning towards advertising a session ID only if we actually\nproduced logs locally, but I'm open to further discussion.\n\n> Does the documentation added by this series make it clear that\n> asking for advertiseSID does NOT automatically enable allocation of\n> session IDs (even if it does not explain why it does not happen)?\n\nIn V3 I'll update the docs to call out whichever decision we reach on\nthis point.\n\n> Thanks.\n> \n> \n"},{"id":"409208","messageId":"20201105185855.GB36751@google.com","threadId":"54527","inReplyTo":"xmqqimaklsvg.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 08/11] fetch-pack: advertise trace2 SID in capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-05T18:58:55Z","receivedAt":"2020-11-05T18:59:04Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.11.04 13:22, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > When trace2 is enabled, the server sent a trace2-sid capability, and\n> > trace2.advertiseSID is true, advertise fetch-pack's own session ID back\n> > to the server.\n> >\n> > Signed-off-by: Josh Steadmon \n> > ---\n> > fetch-pack.c | 10 ++++++++++\n> > 1 file changed, 10 insertions(+)\n> >\n> > diff --git a/fetch-pack.c b/fetch-pack.c\n> > index b10c432315..7fbefa7b8e 100644\n> > --- a/fetch-pack.c\n> > +++ b/fetch-pack.c\n> > @@ -35,6 +35,8 @@ static int fetch_fsck_objects = -1;\n> > static int transfer_fsck_objects = -1;\n> > static int agent_supported;\n> > static int server_supports_filtering;\n> > +static int server_sent_trace2_sid;\n> > +static int advertise_trace2_sid;\n> > static struct shallow_lock shallow_lock;\n> > static const char *alternate_shallow_file;\n> > static struct strbuf fsck_msg_types = STRBUF_INIT;\n> > @@ -326,6 +328,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n> > \t\t\tif (deepen_not_ok) strbuf_addstr(&c, \" deepen-not\");\n> > \t\t\tif (agent_supported) strbuf_addf(&c, \" agent=%s\",\n> > \t\t\t\t\t\t\t git_user_agent_sanitized());\n> > +\t\t\tif (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())\n> > +\t\t\t\tstrbuf_addf(&c, \" trace2-sid=%s\", trace2_session_id());\n> > \t\t\tif (args->filter_options.choice)\n> > \t\t\t\tstrbuf_addstr(&c, \" filter\");\n> > \t\t\tpacket_buf_write(&req_buf, \"want %s%s\\n\", remote_hex, c.buf);\n> > @@ -979,6 +983,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> > \t\t\t\t agent_len, agent_feature);\n> > \t}\n> > \n> > +\tif (server_supports(\"trace2-sid\"))\n> > +\t\tserver_sent_trace2_sid = 1;\n> > +\n> > \tif (server_supports(\"shallow\"))\n> > \t\tprint_verbose(args, _(\"Server supports %s\"), \"shallow\");\n> > \telse if (args->depth > 0 || is_repository_shallow(r))\n> > @@ -1191,6 +1198,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n> > \t\tpacket_buf_write(&req_buf, \"command=fetch\");\n> > \tif (server_supports_v2(\"agent\", 0))\n> > \t\tpacket_buf_write(&req_buf, \"agent=%s\", git_user_agent_sanitized());\n> > +\tif (advertise_trace2_sid && server_supports_v2(\"trace2-sid\", 0) && trace2_is_enabled())\n> > +\t\tpacket_buf_write(&req_buf, \"trace2-sid=%s\", trace2_session_id());\n> > \tif (args->server_options && args->server_options->nr &&\n> > \t server_supports_v2(\"server-option\", 1)) {\n> > \t\tint i;\n> > @@ -1711,6 +1720,7 @@ static void fetch_pack_config(void)\n> > \tgit_config_get_bool(\"repack.usedeltabaseoffset\", &prefer_ofs_delta);\n> > \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n> > \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n> > +\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n> > \tif (!uri_protocols.nr) {\n> > \t\tchar *str;\n> \n> The same comment as 05/11 and 06/11 applies to this repeated\n> appearance of this boolean expression:\n> \n> \tadvertise_trace2_sid && trace2_is_enabled()\n> \n> If we are committed to stick to the \"even if we were told to\n> advertise, do not alllocate a session ID\" design, perhaps it is\n> cleaner to clear advertise_trace2_sid at the very beginning,\n> immediately after we learn that the tracing is disabled and the\n> advertiseSID configuration is read. That way, everybody needs to\n> only care about advertise_trace2_sid variable.\n> \n> Incidentally, if we decide to change the semantics to auto allocate\n> the session ID if advertiseSID configuration asks us to advertise\n> (it is OK if we do not enable the full trace2 suite), we can still\n> make the code only check advertise_trace2_sid variable, without\n> adding repeated check of trace2_is_enabled() everywhere at all.\n\nGood point. Once we settle on whether or not to advertise when tracing\nis enabled, I'll update these conditionals in V3.\n"},{"id":"409209","messageId":"20201105191235.GC36751@google.com","threadId":"54527","inReplyTo":"xmqqeel8lso1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 09/11] upload-pack, serve: log received client trace2 SID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-05T19:12:35Z","receivedAt":"2020-11-05T19:12:44Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.11.04 13:26, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index 3bb01c5427..d938603c97 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -1058,6 +1058,7 @@ static void receive_needs(struct upload_pack_data *data,\n> > \t\tconst char *features;\n> > \t\tstruct object_id oid_buf;\n> > \t\tconst char *arg;\n> > +\t\tint feature_len;\n> > \n> > \t\treset_timeout(data->timeout);\n> > \t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n> > @@ -1109,6 +1110,12 @@ static void receive_needs(struct upload_pack_data *data,\n> > \t\tif (data->allow_filter &&\n> > \t\t parse_feature_request(features, \"filter\"))\n> > \t\t\tdata->filter_capability_requested = 1;\n> > +\t\tif ((arg = parse_feature_value(features, \"trace2-sid\", &feature_len, NULL)))\n> > +\t\t{\n> \n> Style. '{' block is opened on the same line as \"if (condition)\"\n> ends, e.g.\n> \n> \t\targ = parse_feature_value(...);\n> \t\tif (arg) {\n> \n> Avoid assignment in the condition part of if/while unless there is a\n> compelling reason to do so. e.g.\n> \n> \t\tif ((arg = ...) && further_check_on(arg)) {\n> \t\t\t...\n> \n> might be justifiable, but here, I do not see any such reason.\n\nWill fix in V3.\n\n\n> > +\t\t\tchar *client_sid = xstrndup(arg, feature_len);\n> > +\t\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n> > +\t\t\tfree(client_sid);\n> > +\t\t}\n> > \n> > \t\to = parse_object(the_repository, &oid_buf);\n> > \t\tif (!o) {\n> \n> Thanks.\n"},{"id":"409211","messageId":"xmqqd00rk3t6.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"20201105185855.GB36751@google.com","subject":"Re: [PATCH v2 08/11] fetch-pack: advertise trace2 SID in capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-05T19:21:25Z","receivedAt":"2020-11-05T19:21:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n>> The same comment as 05/11 and 06/11 applies to this repeated\n>> appearance of this boolean expression:\n>> \n>> \tadvertise_trace2_sid && trace2_is_enabled()\n>> \n>> If we are committed to stick to the \"even if we were told to\n>> advertise, do not alllocate a session ID\" design, perhaps it is\n>> cleaner to clear advertise_trace2_sid at the very beginning,\n>> immediately after we learn that the tracing is disabled and the\n>> advertiseSID configuration is read. That way, everybody needs to\n>> only care about advertise_trace2_sid variable.\n>> \n>> Incidentally, if we decide to change the semantics to auto allocate\n>> the session ID if advertiseSID configuration asks us to advertise\n>> (it is OK if we do not enable the full trace2 suite), we can still\n>> make the code only check advertise_trace2_sid variable, without\n>> adding repeated check of trace2_is_enabled() everywhere at all.\n>\n> Good point. Once we settle on whether or not to advertise when tracing\n> is enabled, I'll update these conditionals in V3.\n\nWell, we can update these conditionals _before_ deciding that, and\nthat is the whole point of the part of my message you are responding\nto.\n\nWhether the advertise_trace2_sid means \n\n (1) config asked and tracing enabled, or\n\n (2) config asked and we do not care whether tracing is enabled or not\n\nit makes it easier to flip between (1) and (2) later if you clean up\nthe conditional first.\n\nThanks.\n\n"},{"id":"409212","messageId":"20201105192847.GD36751@google.com","threadId":"54527","inReplyTo":"xmqq1rhaqfqg.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 02/11] docs: new trace2.advertiseSID option","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-05T19:28:47Z","receivedAt":"2020-11-05T19:28:55Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.11.03 13:42, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > Document a new config option that allows users to determine whether or\n> > not to advertise their trace2 session IDs to remote Git clients and\n> > servers.\n> \n> I do not think placeing this in the trace2 hierarchy is a good idea.\n> \n> It is not like concept of \"session\" belongs to trace2; each\n> operation we perform inherently is done on behalf of a session.\n> The trace2 subsystem may have been the first to externalize the\n> concept, but even after trace2 gets superseded, we would want to\n> key our log records with some \"session ID\". After we introduce\n> an improved mechanism that is successor to trace2, we still would\n> want to exchange some session ID if the advertiseSID option the\n> users define in their repository today (well, maybe in 3 months\n> after this series lands in a released version and widely\n> deployed), no?\n\nYes this makes sense. Do you think it's worthwhile to move all the\nsession ID implementation out of trace2? Right now there are some\nuser-facing bits (environment variables for parent/child SID hierarchy)\nthat specifically mention trace2, and I believe that the repo tool is\nusing it to tie together logs produced by a single repo invocation.\n\n> We are not exposing the session ID anywhere but the transports, so\n> how about calling it transport.advertiseSID, perhaps?\n\nYeah, this sounds good to me. Will change in V3.\n\n> We also may want to call that just \"session ID\", not \"trace2\n> session ID\" in the description.\n> \n> Thanks.\n> \n> > Signed-off-by: Josh Steadmon \n> > ---\n> > Documentation/config/trace2.txt | 4 ++++\n> > 1 file changed, 4 insertions(+)\n> >\n> > diff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt\n> > index 01d3afd8a8..3f2e3b4425 100644\n> > --- a/Documentation/config/trace2.txt\n> > +++ b/Documentation/config/trace2.txt\n> > @@ -69,3 +69,7 @@ trace2.maxFiles::\n> > \twrite additional traces if we would exceed this many files. Instead,\n> > \twrite a sentinel file that will block further tracing to this\n> > \tdirectory. Defaults to 0, which disables this check.\n> > +\n> > +trace2.advertiseSID::\n> > +\tBoolean. When true, client and server processes will advertise their\n> > +\ttrace2 session IDs to their remote counterpart. Defaults to false.\n"},{"id":"409219","messageId":"634cf106-7bc2-e8fa-5745-5e2d26b50e14@jeffhostetler.com","threadId":"54527","inReplyTo":"xmqq5z6mqg60.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2020-11-05T21:00:46Z","receivedAt":"2020-11-05T21:00:51Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 11/3/20 4:33 PM, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n>> +trace2-sid=\n>> +-----------------------\n>> +\n>> +If trace2 tracing is enabled on the server, it may advertise its session ID via\n>> +this capability. The client may choose to log the server's session ID in its\n>> +trace logs, and advertise its own session ID back to the server for it to log\n>> +as well. This allows for easier debugging of remote sessions when both client\n>> +and server logs are available.\n>> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n>> index e597b74da3..a5b9ef04f6 100644\n>> --- a/Documentation/technical/protocol-v2.txt\n>> +++ b/Documentation/technical/protocol-v2.txt\n>> @@ -492,3 +492,12 @@ form `object-format=X`) to notify the client that the server is able to deal\n>> with objects using hash algorithm X. If not specified, the server is assumed to\n>> only handle SHA-1. If the client would like to use a hash algorithm other than\n>> SHA-1, it should specify its object-format string.\n>> +\n>> +trace2-sid=\n>> +~~~~~~~~~~~~~~~~~~~~~~~\n>> +\n>> +If trace2 tracing is enabled on the server, it may advertise its session ID via\n>> +this capability. The client may choose to log the server's session ID in its\n>> +trace logs, and advertise its own session ID back to the server for it to log\n>> +as well. This allows for easier debugging of remote sessions when both client\n>> +and server logs are available.\n> \n> Have we documented what a session-id should look like anywhere in\n> the documentation? This document is to help third-party to write\n> implementations of the protocol, but the above description leaves\n> things \"implementation defined\" a bit too much, I am afraid.\n> \n> For example, as this must fit on a single pkt-line as an advertised\n> capability, there would be some length limit. Are there other\n> inherent limitations due to our protocol? Are there some artificial\n> limitations that we may want to impose to make it easier to harden\n> implementations against common mistakes? For example are bytes in\n> allowed to contain LF, CR, NUL, etc.?\n> \n\nThe computed part of trace2 SIDs are relatively safe (both for length\nand funky characters). And funky characters are protected by JSON\nencoding rules when writing to the GIT_TRACE2_EVENT target.\n\nAnd I think the computed part would be safe in this context.\nI've not seen commands that spawn more than about 6 levels of\nchild processes, and those would be fine here.\n\nHowever, the opportunity to introduce a prefix as I suggested earlier\ndoes offer the opportunity to introduce funky chars that would not\nhave any protection, so you may want to c-quote the string when\ninserting it into the wire protocol.\n\n $ GIT_TRACE2_EVENT=1 GIT_TRACE2_PARENT_SID=hello git version\n {\"event\":\"version\",\"sid\":\"hello/20201030T154143.660608Z-H86606a97- \nP00001d30\",...}\n ...\n\n(Allowing the user to insert a prefix like that has been useful for\ntracking control/experiment testing, so I wouldn't want to disable\nit.)\n\nJeff\n"},{"id":"409221","messageId":"f7dfc2ee-673f-8386-299c-ddfbe984b04f@jeffhostetler.com","threadId":"54527","inReplyTo":"xmqqa6vyqgoc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2020-11-05T21:01:32Z","receivedAt":"2020-11-05T21:01:35Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 11/3/20 4:22 PM, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n>>> So the value being passed between client and server over the\n>>> protocol may look like `//` rather than just a\n>>> single `` term. For your purposes, do you want or care if\n>>> you get the single term or the full SID ?\n>>\n>> I'm not sure we care too much one way or the other. A single component\n>> of the SID should be enough to join client & server logs, but it's\n>> easier to just send the whole thing.\n> \n> It may be worth documenting this design decision in the protocol\n> doc; even though protocol doc may say this should be treated as an\n> opaque token, people may assume certain structure.\n> \n\ngood point. let me make a note to revisit that text.\n\nthanks\nJeff\n"},{"id":"409233","messageId":"xmqqeel7ijk0.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"20201105192847.GD36751@google.com","subject":"Re: [PATCH v2 02/11] docs: new trace2.advertiseSID option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-05T21:24:15Z","receivedAt":"2020-11-05T21:24:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Yes this makes sense. Do you think it's worthwhile to move all the\n> session ID implementation out of trace2? Right now there are some\n> user-facing bits (environment variables for parent/child SID hierarchy)\n> that specifically mention trace2, and I believe that the repo tool is\n> using it to tie together logs produced by a single repo invocation.\n\nIf somebody other than trace2 starts using session ID, or if we\nintroduce a mechanism that allows a session ID assigned without\nenabling the rest of the trace2 machinery, such a separation may\nmake sense at the implementation level, but until then, I do not\nthink it is worth doing.\n\n"},{"id":"409272","messageId":"nycvar.QRO.7.76.6.2011061255080.18437@tvgsbejvaqbjf.bet","threadId":"54527","inReplyTo":"xmqqeel7ijk0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 02/11] docs: new trace2.advertiseSID option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-06T11:57:28Z","receivedAt":"2020-11-06T11:57:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 5 Nov 2020, Junio C Hamano wrote:\n\n> Josh Steadmon writes:\n>\n> > Yes this makes sense. Do you think it's worthwhile to move all the\n> > session ID implementation out of trace2? Right now there are some\n> > user-facing bits (environment variables for parent/child SID hierarchy)\n> > that specifically mention trace2, and I believe that the repo tool is\n> > using it to tie together logs produced by a single repo invocation.\n>\n> If somebody other than trace2 starts using session ID, or if we\n> introduce a mechanism that allows a session ID assigned without\n> enabling the rest of the trace2 machinery, such a separation may\n> make sense at the implementation level, but until then, I do not\n> think it is worth doing.\n\nYep, the closest we came to having another user of the session ID was\nMatthew DeVore's `git xl` effort, but that effort seems to have gone cold\n(https://lore.kernel.org/git/20200207013918.GA459@comcast.net/#r is the\nlatest message in that thread that I could find).\n\nCiao,\nDscho\n"},{"id":"409553","messageId":"20201110213732.GE36751@google.com","threadId":"54527","inReplyTo":"xmqqa6vyqgoc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-10T21:37:32Z","receivedAt":"2020-11-10T21:39:05Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.11.03 13:22, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> >> So the value being passed between client and server over the\n> >> protocol may look like `//` rather than just a\n> >> single `` term. For your purposes, do you want or care if\n> >> you get the single term or the full SID ?\n> >\n> > I'm not sure we care too much one way or the other. A single component\n> > of the SID should be enough to join client & server logs, but it's\n> > easier to just send the whole thing.\n> \n> It may be worth documenting this design decision in the protocol\n> doc; even though protocol doc may say this should be treated as an\n> opaque token, people may assume certain structure.\n\nDone in V3.\n"},{"id":"409729","messageId":"20201111224023.GF36751@google.com","threadId":"54527","inReplyTo":"xmqq5z6mqg60.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T22:40:23Z","receivedAt":"2020-11-11T22:42:39Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.11.03 13:33, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > +trace2-sid=\n> > +-----------------------\n> > +\n> > +If trace2 tracing is enabled on the server, it may advertise its session ID via\n> > +this capability. The client may choose to log the server's session ID in its\n> > +trace logs, and advertise its own session ID back to the server for it to log\n> > +as well. This allows for easier debugging of remote sessions when both client\n> > +and server logs are available.\n> > diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> > index e597b74da3..a5b9ef04f6 100644\n> > --- a/Documentation/technical/protocol-v2.txt\n> > +++ b/Documentation/technical/protocol-v2.txt\n> > @@ -492,3 +492,12 @@ form `object-format=X`) to notify the client that the server is able to deal\n> > with objects using hash algorithm X. If not specified, the server is assumed to\n> > only handle SHA-1. If the client would like to use a hash algorithm other than\n> > SHA-1, it should specify its object-format string.\n> > +\n> > +trace2-sid=\n> > +~~~~~~~~~~~~~~~~~~~~~~~\n> > +\n> > +If trace2 tracing is enabled on the server, it may advertise its session ID via\n> > +this capability. The client may choose to log the server's session ID in its\n> > +trace logs, and advertise its own session ID back to the server for it to log\n> > +as well. This allows for easier debugging of remote sessions when both client\n> > +and server logs are available.\n> \n> Have we documented what a session-id should look like anywhere in\n> the documentation? This document is to help third-party to write\n> implementations of the protocol, but the above description leaves\n> things \"implementation defined\" a bit too much, I am afraid.\n> \n> For example, as this must fit on a single pkt-line as an advertised\n> capability, there would be some length limit. Are there other\n> inherent limitations due to our protocol? Are there some artificial\n> limitations that we may want to impose to make it easier to harden\n> implementations against common mistakes? For example are bytes in\n> allowed to contain LF, CR, NUL, etc.?\n\nDocumented in V3. For argument's sake, I'm going to say that the tokens\nshould be limited to printable, non-whitespace characters, and should\nfit on a single pkt-line.\n"},{"id":"409731","messageId":"fe6731cc0908201f26cb04c5b91cd78a6c4f2602.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 06/11] serve: advertise session ID in v2 capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:29Z","receivedAt":"2020-11-12T01:32:35Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When transfer.advertiseSID is true, advertise the server's session ID\nfor all protocol v2 connections via the new session-id capability.\n\nSigned-off-by: Josh Steadmon \n---\n serve.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/serve.c b/serve.c\nindex f6341206c4..8c0bb84f37 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -8,6 +8,8 @@\n #include \"serve.h\"\n #include \"upload-pack.h\"\n \n+static int advertise_sid;\n+\n static int always_advertise(struct repository *r,\n \t\t\t struct strbuf *value)\n {\n@@ -30,6 +32,15 @@ static int object_format_advertise(struct repository *r,\n \treturn 1;\n }\n \n+static int session_id_advertise(struct repository *r, struct strbuf *value)\n+{\n+\tif (!advertise_sid)\n+\t\treturn 0;\n+\tif (value)\n+\t\tstrbuf_addstr(value, trace2_session_id());\n+\treturn 1;\n+}\n+\n struct protocol_capability {\n \t/*\n \t * The name of the capability. The server uses this name when\n@@ -66,6 +77,7 @@ static struct protocol_capability capabilities[] = {\n \t{ \"fetch\", upload_pack_advertise, upload_pack_v2 },\n \t{ \"server-option\", always_advertise, NULL },\n \t{ \"object-format\", object_format_advertise, NULL },\n+\t{ \"session-id\", session_id_advertise, NULL },\n };\n \n static void advertise_capabilities(void)\n@@ -261,6 +273,8 @@ static int process_request(void)\n /* Main serve loop for protocol version 2 */\n void serve(struct serve_options *options)\n {\n+\tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n+\n \tif (options->advertise_capabilities || !options->stateless_rpc) {\n \t\t/* serve by default supports v2 */\n \t\tpacket_write_fmt(1, \"version 2\\n\");\n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409732","messageId":"fc9f6c92863e36192a496b4a33e6c8bdecfee4ba.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 08/11] fetch-pack: advertise session ID in capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:31Z","receivedAt":"2020-11-12T01:32:35Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When the server sent a session-id capability and transfer.advertiseSID\nis true, advertise fetch-pack's own session ID back to the server.\n\nSigned-off-by: Josh Steadmon \n---\n fetch-pack.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b10c432315..23179b8dd0 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -35,6 +35,7 @@ static int fetch_fsck_objects = -1;\n static int transfer_fsck_objects = -1;\n static int agent_supported;\n static int server_supports_filtering;\n+static int advertise_sid;\n static struct shallow_lock shallow_lock;\n static const char *alternate_shallow_file;\n static struct strbuf fsck_msg_types = STRBUF_INIT;\n@@ -326,6 +327,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\tif (deepen_not_ok) strbuf_addstr(&c, \" deepen-not\");\n \t\t\tif (agent_supported) strbuf_addf(&c, \" agent=%s\",\n \t\t\t\t\t\t\t git_user_agent_sanitized());\n+\t\t\tif (advertise_sid)\n+\t\t\t\tstrbuf_addf(&c, \" session-id=%s\", trace2_session_id());\n \t\t\tif (args->filter_options.choice)\n \t\t\t\tstrbuf_addstr(&c, \" filter\");\n \t\t\tpacket_buf_write(&req_buf, \"want %s%s\\n\", remote_hex, c.buf);\n@@ -979,6 +982,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t agent_len, agent_feature);\n \t}\n \n+\tif (!server_supports(\"session-id\"))\n+\t\tadvertise_sid = 0;\n+\n \tif (server_supports(\"shallow\"))\n \t\tprint_verbose(args, _(\"Server supports %s\"), \"shallow\");\n \telse if (args->depth > 0 || is_repository_shallow(r))\n@@ -1191,6 +1197,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tpacket_buf_write(&req_buf, \"command=fetch\");\n \tif (server_supports_v2(\"agent\", 0))\n \t\tpacket_buf_write(&req_buf, \"agent=%s\", git_user_agent_sanitized());\n+\tif (advertise_sid && server_supports_v2(\"session-id\", 0))\n+\t\tpacket_buf_write(&req_buf, \"session-id=%s\", trace2_session_id());\n \tif (args->server_options && args->server_options->nr &&\n \t server_supports_v2(\"server-option\", 1)) {\n \t\tint i;\n@@ -1711,6 +1719,7 @@ static void fetch_pack_config(void)\n \tgit_config_get_bool(\"repack.usedeltabaseoffset\", &prefer_ofs_delta);\n \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n+\tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n \tif (!uri_protocols.nr) {\n \t\tchar *str;\n \n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409733","messageId":"014cae8dc1c3e03e149f8abc98f2f1a50bb0cb98.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 07/11] transport: log received server session ID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:30Z","receivedAt":"2020-11-12T01:32:37Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When a client receives a session-id capability from a protocol v0, v1,\nor v2 server, log the received session ID via a trace2 data event.\n\nSigned-off-by: Josh Steadmon \n---\n t/t5705-session-id-in-capabilities.sh | 64 +++++++++++++++++++++++++++\n transport.c | 10 +++++\n 2 files changed, 74 insertions(+)\n create mode 100755 t/t5705-session-id-in-capabilities.sh\n\ndiff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh\nnew file mode 100755\nindex 0000000000..9e782f4413\n--- /dev/null\n+++ b/t/t5705-session-id-in-capabilities.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='session ID in capabilities'\n+\n+. ./test-lib.sh\n+\n+REPO=\"$(pwd)/repo\"\n+LOCAL_PRISTINE=\"$(pwd)/local_pristine\"\n+\n+test_expect_success 'setup repos for session ID capability tests' '\n+\tgit init \"$REPO\" &&\n+\ttest_commit -C \"$REPO\" a &&\n+\tgit clone \"file://$REPO\" \"$LOCAL_PRISTINE\" &&\n+\ttest_commit -C \"$REPO\" b\n+'\n+\n+for PROTO in 0 1 2\n+do\n+\ttest_expect_success \"session IDs not advertised by default (fetch v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t'\n+\n+\ttest_expect_success \"session IDs not advertised by default (push v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tgit -C local pull --no-rebase origin &&\n+\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local push origin &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t'\n+done\n+\n+test_expect_success 'enable SID advertisement' '\n+\tgit -C \"$REPO\" config transfer.advertiseSID true &&\n+\tgit -C \"$LOCAL_PRISTINE\" config transfer.advertiseSID true\n+'\n+\n+for PROTO in 0 1 2\n+do\n+\ttest_expect_success \"session IDs advertised (fetch v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t'\n+\n+\ttest_expect_success \"session IDs advertised (push v${PROTO})\" '\n+\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n+\t\tgit -C local pull --no-rebase origin &&\n+\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n+\t\tgit -c protocol.version=$PROTO -C local push origin &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t'\n+done\n+\n+test_done\ndiff --git a/transport.c b/transport.c\nindex 47da955e4f..679a35e7c1 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -286,6 +286,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \tstruct git_transport_data *data = transport->data;\n \tstruct ref *refs = NULL;\n \tstruct packet_reader reader;\n+\tint sid_len;\n+\tconst char *server_sid;\n \n \tconnect_setup(transport, for_push);\n \n@@ -297,6 +299,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \tdata->version = discover_version(&reader);\n \tswitch (data->version) {\n \tcase protocol_v2:\n+\t\tif (server_feature_v2(\"session-id\", &server_sid))\n+\t\t\ttrace2_data_string(\"transfer\", NULL, \"server-sid\", server_sid);\n \t\tif (must_list_refs)\n \t\t\tget_remote_refs(data->fd[1], &reader, &refs, for_push,\n \t\t\t\t\tref_prefixes,\n@@ -310,6 +314,12 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \t\t\t\t for_push ? REF_NORMAL : 0,\n \t\t\t\t &data->extra_have,\n \t\t\t\t &data->shallow);\n+\t\tserver_sid = server_feature_value(\"session-id\", &sid_len);\n+\t\tif (server_sid) {\n+\t\t\tchar *sid = xstrndup(server_sid, sid_len);\n+\t\t\ttrace2_data_string(\"transfer\", NULL, \"server-sid\", sid);\n+\t\t\tfree(sid);\n+\t\t}\n \t\tbreak;\n \tcase protocol_unknown_version:\n \t\tBUG(\"unknown protocol version\");\n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409734","messageId":"012949e7da24a9f9b8bb7ac7c098f04a1d35d6f7.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 10/11] send-pack: advertise session ID in capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:33Z","receivedAt":"2020-11-12T01:32:40Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When the server sent a session-id capability and transfer.advertiseSID\nis true, advertise send-pack's own session ID back to the server.\n\nSigned-off-by: Josh Steadmon \n---\n send-pack.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex eb4a44270b..bda65c98f9 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -424,6 +424,7 @@ int send_pack(struct send_pack_args *args,\n \tint use_sideband = 0;\n \tint quiet_supported = 0;\n \tint agent_supported = 0;\n+\tint advertise_sid = 0;\n \tint use_atomic = 0;\n \tint atomic_supported = 0;\n \tint use_push_options = 0;\n@@ -435,6 +436,8 @@ int send_pack(struct send_pack_args *args,\n \tconst char *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n \n+\tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n+\n \t/* Does the other end support the reporting? */\n \tif (server_supports(\"report-status-v2\"))\n \t\tstatus_report = 2;\n@@ -450,6 +453,8 @@ int send_pack(struct send_pack_args *args,\n \t\tquiet_supported = 1;\n \tif (server_supports(\"agent\"))\n \t\tagent_supported = 1;\n+\tif (!server_supports(\"session-id\"))\n+\t\tadvertise_sid = 0;\n \tif (server_supports(\"no-thin\"))\n \t\targs->use_thin_pack = 0;\n \tif (server_supports(\"atomic\"))\n@@ -506,6 +511,8 @@ int send_pack(struct send_pack_args *args,\n \t\tstrbuf_addf(&cap_buf, \" object-format=%s\", the_hash_algo->name);\n \tif (agent_supported)\n \t\tstrbuf_addf(&cap_buf, \" agent=%s\", git_user_agent_sanitized());\n+\tif (advertise_sid)\n+\t\tstrbuf_addf(&cap_buf, \" session-id=%s\", trace2_session_id());\n \n \t/*\n \t * NEEDSWORK: why does delete-refs have to be so specific to\n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409735","messageId":"bde9c1d97a295d99d9da867410743b5c237bf7ae.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 09/11] upload-pack, serve: log received client session ID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:32Z","receivedAt":"2020-11-12T01:32:47Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When upload-pack (protocol v0/v1) or a protocol v2 server receives a\nsession-id capability from a client, log the received session ID via a\ntrace2 data event.\n\nSigned-off-by: Josh Steadmon \n---\n serve.c | 4 ++++\n t/t5705-session-id-in-capabilities.sh | 18 ++++++++++++------\n upload-pack.c | 8 ++++++++\n 3 files changed, 24 insertions(+), 6 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 8c0bb84f37..eec2fe6f29 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -201,6 +201,7 @@ static int process_request(void)\n \tstruct packet_reader reader;\n \tstruct strvec keys = STRVEC_INIT;\n \tstruct protocol_capability *command = NULL;\n+\tconst char *client_sid;\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -264,6 +265,9 @@ static int process_request(void)\n \n \tcheck_algorithm(the_repository, &keys);\n \n+\tif (has_capability(&keys, \"session-id\", &client_sid))\n+\t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n+\n \tcommand->command(the_repository, &keys, &reader);\n \n \tstrvec_clear(&keys);\ndiff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh\nindex 9e782f4413..afa2159657 100755\n--- a/t/t5705-session-id-in-capabilities.sh\n+++ b/t/t5705-session-id-in-capabilities.sh\n@@ -17,11 +17,14 @@ test_expect_success 'setup repos for session ID capability tests' '\n for PROTO in 0 1 2\n do\n \ttest_expect_success \"session IDs not advertised by default (fetch v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n-\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t\tgit -c protocol.version=$PROTO -C local fetch \\\n+\t\t\t--upload-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-upload-pack\" \\\n+\t\t\torigin &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\" &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"client-sid\\\" tr2-server-events)\"\n \t'\n \n \ttest_expect_success \"session IDs not advertised by default (push v${PROTO})\" '\n@@ -43,11 +46,14 @@ test_expect_success 'enable SID advertisement' '\n for PROTO in 0 1 2\n do\n \ttest_expect_success \"session IDs advertised (fetch v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n-\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t\tgit -c protocol.version=$PROTO -C local fetch \\\n+\t\t\t--upload-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-upload-pack\" \\\n+\t\t\torigin &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events &&\n+\t\tgrep \\\"key\\\":\\\"client-sid\\\" tr2-server-events\n \t'\n \n \ttest_expect_success \"session IDs advertised (push v${PROTO})\" '\ndiff --git a/upload-pack.c b/upload-pack.c\nindex ebb4099268..dcd429dc01 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1058,6 +1058,7 @@ static void receive_needs(struct upload_pack_data *data,\n \t\tconst char *features;\n \t\tstruct object_id oid_buf;\n \t\tconst char *arg;\n+\t\tint feature_len;\n \n \t\treset_timeout(data->timeout);\n \t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n@@ -1110,6 +1111,13 @@ static void receive_needs(struct upload_pack_data *data,\n \t\t parse_feature_request(features, \"filter\"))\n \t\t\tdata->filter_capability_requested = 1;\n \n+\t\targ = parse_feature_value(features, \"session-id\", &feature_len, NULL);\n+\t\tif (arg) {\n+\t\t\tchar *client_sid = xstrndup(arg, feature_len);\n+\t\t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n+\t\t\tfree(client_sid);\n+\t\t}\n+\n \t\to = parse_object(the_repository, &oid_buf);\n \t\tif (!o) {\n \t\t\tpacket_writer_error(&data->writer,\n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409736","messageId":"20201111233210.GH36751@google.com","threadId":"54527","inReplyTo":"xmqqd00rk3t6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 08/11] fetch-pack: advertise trace2 SID in capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:32:10Z","receivedAt":"2020-11-12T01:32:51Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.11.05 11:21, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> >> The same comment as 05/11 and 06/11 applies to this repeated\n> >> appearance of this boolean expression:\n> >> \n> >> \tadvertise_trace2_sid && trace2_is_enabled()\n> >> \n> >> If we are committed to stick to the \"even if we were told to\n> >> advertise, do not alllocate a session ID\" design, perhaps it is\n> >> cleaner to clear advertise_trace2_sid at the very beginning,\n> >> immediately after we learn that the tracing is disabled and the\n> >> advertiseSID configuration is read. That way, everybody needs to\n> >> only care about advertise_trace2_sid variable.\n> >> \n> >> Incidentally, if we decide to change the semantics to auto allocate\n> >> the session ID if advertiseSID configuration asks us to advertise\n> >> (it is OK if we do not enable the full trace2 suite), we can still\n> >> make the code only check advertise_trace2_sid variable, without\n> >> adding repeated check of trace2_is_enabled() everywhere at all.\n> >\n> > Good point. Once we settle on whether or not to advertise when tracing\n> > is enabled, I'll update these conditionals in V3.\n> \n> Well, we can update these conditionals _before_ deciding that, and\n> that is the whole point of the part of my message you are responding\n> to.\n> \n> Whether the advertise_trace2_sid means \n> \n> (1) config asked and tracing enabled, or\n> \n> (2) config asked and we do not care whether tracing is enabled or not\n> \n> it makes it easier to flip between (1) and (2) later if you clean up\n> the conditional first.\n> \n> Thanks.\n> \n\nDone in V3. After some additional offline discussions, I've been\nconvinced that it makes sense to advertise the session-id capability\neven when trace2 is not enabled. Specifically, it can be useful on the\nserver side to identify multiple connections that make up a single\nlarger operation, such as in a single `repo` invocation.\n"},{"id":"409739","messageId":"ea2a318f2bd46115f5eb76426e6546a325a1b62e.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 11/11] receive-pack: log received client session ID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:34Z","receivedAt":"2020-11-12T01:49:45Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When receive-pack receives a session-id capability from the client, log\nthe received session ID via a trace2 data event.\n\nSigned-off-by: Josh Steadmon \n---\n builtin/receive-pack.c | 7 +++++++\n t/t5705-session-id-in-capabilities.sh | 20 ++++++++++++++------\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 6ed498b6c7..deb5f859a9 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2039,6 +2039,7 @@ static struct command *read_head_info(struct packet_reader *reader,\n \t\tif (linelen < reader->pktlen) {\n \t\t\tconst char *feature_list = reader->line + linelen + 1;\n \t\t\tconst char *hash = NULL;\n+\t\t\tconst char *client_sid;\n \t\t\tint len = 0;\n \t\t\tif (parse_feature_request(feature_list, \"report-status\"))\n \t\t\t\treport_status = 1;\n@@ -2061,6 +2062,12 @@ static struct command *read_head_info(struct packet_reader *reader,\n \t\t\t}\n \t\t\tif (xstrncmpz(the_hash_algo->name, hash, len))\n \t\t\t\tdie(\"error: unsupported object format '%s'\", hash);\n+\t\t\tclient_sid = parse_feature_value(feature_list, \"session-id\", &len, NULL);\n+\t\t\tif (client_sid) {\n+\t\t\t\tchar *sid = xstrndup(client_sid, len);\n+\t\t\t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n+\t\t\t\tfree(sid);\n+\t\t\t}\n \t\t}\n \n \t\tif (!strcmp(reader->line, \"push-cert\")) {\ndiff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh\nindex afa2159657..f1d189d5bc 100755\n--- a/t/t5705-session-id-in-capabilities.sh\n+++ b/t/t5705-session-id-in-capabilities.sh\n@@ -28,13 +28,17 @@ do\n \t'\n \n \ttest_expect_success \"session IDs not advertised by default (push v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n+\t\ttest_when_finished \"git -C local push --delete origin new-branch\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tgit -C local pull --no-rebase origin &&\n \t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local push origin &&\n-\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n+\t\tgit -c protocol.version=$PROTO -C local push \\\n+\t\t\t--receive-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-receive-pack\" \\\n+\t\t\torigin HEAD:new-branch &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\" &&\n+\t\ttest -z \"$(grep \\\"key\\\":\\\"client-sid\\\" tr2-server-events)\"\n \t'\n done\n \n@@ -57,13 +61,17 @@ do\n \t'\n \n \ttest_expect_success \"session IDs advertised (push v${PROTO})\" '\n-\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n+\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n+\t\ttest_when_finished \"git -C local push --delete origin new-branch\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tgit -C local pull --no-rebase origin &&\n \t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n-\t\tgit -c protocol.version=$PROTO -C local push origin &&\n-\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n+\t\tgit -c protocol.version=$PROTO -C local push \\\n+\t\t\t--receive-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-receive-pack\" \\\n+\t\t\torigin HEAD:new-branch &&\n+\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events &&\n+\t\tgrep \\\"key\\\":\\\"client-sid\\\" tr2-server-events\n \t'\n done\n \n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409740","messageId":"32fe78f3e95334197e73230169a9b0a292ec47bf.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 05/11] receive-pack: advertise session ID in v0 capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:28Z","receivedAt":"2020-11-12T01:50:01Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When transfer.advertiseSID is true, advertise receive-pack's session ID\nvia the new session-id capability.\n\nSigned-off-by: Josh Steadmon \n---\n builtin/receive-pack.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bb9909c52e..6ed498b6c7 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -54,6 +54,7 @@ static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int advertise_atomic_push = 1;\n static int advertise_push_options;\n+static int advertise_sid;\n static int unpack_limit = 100;\n static off_t max_input_size;\n static int report_status;\n@@ -248,6 +249,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"transfer.advertisesid\") == 0) {\n+\t\tadvertise_sid = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -268,6 +274,8 @@ static void show_ref(const char *path, const struct object_id *oid)\n \t\t\tstrbuf_addf(&cap, \" push-cert=%s\", push_cert_nonce);\n \t\tif (advertise_push_options)\n \t\t\tstrbuf_addstr(&cap, \" push-options\");\n+\t\tif (advertise_sid)\n+\t\t\tstrbuf_addf(&cap, \" session-id=%s\", trace2_session_id());\n \t\tstrbuf_addf(&cap, \" object-format=%s\", the_hash_algo->name);\n \t\tstrbuf_addf(&cap, \" agent=%s\", git_user_agent_sanitized());\n \t\tpacket_write_fmt(1, \"%s %s%c%s\\n\",\n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409741","messageId":"612957b9d56a30a22b4b05c46d9050e3fd2979d8.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 04/11] upload-pack: advertise session ID in v0 capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:27Z","receivedAt":"2020-11-12T01:50:06Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When transfer.advertiseSID is true, advertise upload-pack's session ID\nvia the new session-id capability.\n\nSigned-off-by: Josh Steadmon \n---\n upload-pack.c | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b858eb457..ebb4099268 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -110,6 +110,7 @@ struct upload_pack_data {\n \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n+\tunsigned advertise_sid : 1;\n };\n \n static void upload_pack_data_init(struct upload_pack_data *data)\n@@ -141,6 +142,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tpacket_writer_init(&data->writer, 1);\n \n \tdata->keepalive = 5;\n+\tdata->advertise_sid = 0;\n }\n \n static void upload_pack_data_clear(struct upload_pack_data *data)\n@@ -1178,6 +1180,11 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)\n \t\tstrbuf_addf(buf, \" symref=%s:%s\", item->string, (char *)item->util);\n }\n \n+static void format_session_id(struct strbuf *buf, struct upload_pack_data *d) {\n+\tif (d->advertise_sid)\n+\t\tstrbuf_addf(buf, \" session-id=%s\", trace2_session_id());\n+}\n+\n static int send_ref(const char *refname, const struct object_id *oid,\n \t\t int flag, void *cb_data)\n {\n@@ -1193,9 +1200,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \n \tif (capabilities) {\n \t\tstruct strbuf symref_info = STRBUF_INIT;\n+\t\tstruct strbuf session_id = STRBUF_INIT;\n \n \t\tformat_symref_info(&symref_info, &data->symref);\n-\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n+\t\tformat_session_id(&session_id, data);\n+\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n \t\t\t (data->allow_uor & ALLOW_TIP_SHA1) ?\n@@ -1205,9 +1214,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\t\t data->stateless_rpc ? \" no-done\" : \"\",\n \t\t\t symref_info.buf,\n \t\t\t data->allow_filter ? \" filter\" : \"\",\n+\t\t\t session_id.buf,\n \t\t\t the_hash_algo->name,\n \t\t\t git_user_agent_sanitized());\n \t\tstrbuf_release(&symref_info);\n+\t\tstrbuf_release(&session_id);\n \t} else {\n \t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n@@ -1299,6 +1310,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)\n \t\tdata->allow_sideband_all = git_config_bool(var, value);\n \t} else if (!strcmp(\"core.precomposeunicode\", var)) {\n \t\tprecomposed_unicode = git_config_bool(var, value);\n+\t} else if (!strcmp(\"transfer.advertisesid\", var)) {\n+\t\tdata->advertise_sid = git_config_bool(var, value);\n \t}\n \n \tif (current_config_scope() != CONFIG_SCOPE_LOCAL &&\n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409742","messageId":"227c709ba54ee457f05704e52310b39edeaae1ca.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 03/11] trace2: add a public function for getting the SID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:26Z","receivedAt":"2020-11-12T01:50:09Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Add a public wrapper, trace2_session_id(), around tr2_sid_get(), which\nis intended to be private trace2 implementation.\n\nSigned-off-by: Josh Steadmon \n---\n trace2.c | 5 +++++\n trace2.h | 2 ++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/trace2.c b/trace2.c\nindex 2c6b570077..256120c7fd 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -792,3 +792,8 @@ void trace2_printf(const char *fmt, ...)\n \tva_end(ap);\n }\n #endif\n+\n+const char *trace2_session_id(void)\n+{\n+\treturn tr2_sid_get();\n+}\ndiff --git a/trace2.h b/trace2.h\nindex b18bc5529c..ede18c2e06 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -500,4 +500,6 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason);\n \t} while (0)\n #endif\n \n+const char *trace2_session_id(void);\n+\n #endif /* TRACE2_H */\n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409743","messageId":"937534065a9d5609f61fdcf7a8309418a07cb755.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 02/11] docs: new transfer.advertiseSID option","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:25Z","receivedAt":"2020-11-12T01:50:16Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Document a new config option that allows users to determine whether or\nnot to advertise their session IDs to remote Git clients and servers.\n\nSigned-off-by: Josh Steadmon \n---\n Documentation/config/transfer.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt\nindex f5b6245270..505126a780 100644\n--- a/Documentation/config/transfer.txt\n+++ b/Documentation/config/transfer.txt\n@@ -69,3 +69,7 @@ transfer.unpackLimit::\n \tWhen `fetch.unpackLimit` or `receive.unpackLimit` are\n \tnot set, the value of this variable is used instead.\n \tThe default value is 100.\n+\n+transfer.advertiseSID::\n+\tBoolean. When true, client and server processes will advertise their\n+\tunique session IDs to their remote counterpart. Defaults to false.\n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409744","messageId":"184cabb6f5d56f4c7cc744fc311428f1ab13d93b.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"[PATCH v3 01/11] docs: new capability to advertise session IDs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:24Z","receivedAt":"2020-11-12T01:50:20Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"In future patches, we will add the ability for Git servers and clients\nto advertise unique session IDs via protocol capabilities. This\nallows for easier debugging when both client and server logs are\navailable.\n\nSigned-off-by: Josh Steadmon \n---\n .../technical/protocol-capabilities.txt | 17 +++++++++++++++--\n Documentation/technical/protocol-v2.txt | 13 +++++++++++++\n 2 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex ba869a7d36..9dfade930d 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -27,8 +27,8 @@ and 'push-cert' capabilities are sent and recognized by the receive-pack\n (push to server) process.\n \n The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized\n-by both upload-pack and receive-pack protocols. The 'agent' capability\n-may optionally be sent in both protocols.\n+by both upload-pack and receive-pack protocols. The 'agent' and 'session-id'\n+capabilities may optionally be sent in both protocols.\n \n All other capabilities are only recognized by the upload-pack (fetch\n from server) process.\n@@ -365,3 +365,16 @@ If the upload-pack server advertises the 'filter' capability,\n fetch-pack may send \"filter\" commands to request a partial clone\n or partial fetch and request that the server omit various objects\n from the packfile.\n+\n+session-id=\n+-----------------------\n+\n+The server may advertise a session ID that can be used to identify this process\n+across multiple requests. The client may advertise its own session ID back to\n+the server as well.\n+\n+Session IDs should be unique to a given process. They must fit within a\n+packet-line, and must not contain non-printable or whitespace characters. The\n+current implementation uses trace2 session IDs (see\n+link:api-trace2.html[api-trace2] for details), but this may change and users of\n+the session ID should not rely on this fact.\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex e597b74da3..85daeb5d9e 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -492,3 +492,16 @@ form `object-format=X`) to notify the client that the server is able to deal\n with objects using hash algorithm X. If not specified, the server is assumed to\n only handle SHA-1. If the client would like to use a hash algorithm other than\n SHA-1, it should specify its object-format string.\n+\n+session-id=\n+~~~~~~~~~~~~~~~~~~~~~~~\n+\n+The server may advertise a session ID that can be used to identify this process\n+across multiple requests. The client may advertise its own session ID back to\n+the server as well.\n+\n+Session IDs should be unique to a given process. They must fit within a\n+packet-line, and must not contain non-printable or whitespace characters. The\n+current implementation uses trace2 session IDs (see\n+link:api-trace2.html[api-trace2] for details), but this may change and users of\n+the session ID should not rely on this fact.\n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409745","messageId":"cover.1605136908.git.steadmon@google.com","threadId":"54527","inReplyTo":"cover.1604006121.git.steadmon@google.com","subject":"[PATCH v3 00/11] Advertise session ID in protocol capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T23:29:23Z","receivedAt":"2020-11-12T01:50:24Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"In order to more easily debug remote operations, it is useful to be able\nto identify sessions on either side of the connection. This series\nallows clients and servers to provide a unique session ID via a new\n\"session-id\" protocol capability. This session ID can be logged on each\nside of the connection, allowing logs to be joined during debugging\nsessions.\n\nChanges from V2:\n* De-emphasize connection to trace2\n* Renamed capability from \"trace2-sid\" to \"session-id\"\n* Noted (lack of) session ID structure in capability docs\n* Advertise SID regardless of whether trace2 is enabled\n* Simplify conditionals\n* Style cleanups\n\nChanges since V1:\n* Added a public trace2_session_id() function to trace2.{h,c}. Used this\n in place of tr2_sid_get().\n* Fixed a style issue regarding using NULL rather than 0.\n\nJosh Steadmon (11):\n docs: new capability to advertise session IDs\n docs: new transfer.advertiseSID option\n trace2: add a public function for getting the SID\n upload-pack: advertise session ID in v0 capabilities\n receive-pack: advertise session ID in v0 capabilities\n serve: advertise session ID in v2 capabilities\n transport: log received server session ID\n fetch-pack: advertise session ID in capabilities\n upload-pack, serve: log received client session ID\n send-pack: advertise session ID in capabilities\n receive-pack: log received client session ID\n\n Documentation/config/transfer.txt | 4 +\n .../technical/protocol-capabilities.txt | 17 +++-\n Documentation/technical/protocol-v2.txt | 13 ++++\n builtin/receive-pack.c | 15 ++++\n fetch-pack.c | 9 +++\n send-pack.c | 7 ++\n serve.c | 18 +++++\n t/t5705-session-id-in-capabilities.sh | 78 +++++++++++++++++++\n trace2.c | 5 ++\n trace2.h | 2 +\n transport.c | 10 +++\n upload-pack.c | 23 +++++-\n 12 files changed, 198 insertions(+), 3 deletions(-)\n create mode 100755 t/t5705-session-id-in-capabilities.sh\n\nRange-diff against v2:\n 1: d04028c3c7 < -: ---------- docs: new capability to advertise trace2 SIDs\n 2: 5d5097b671 < -: ---------- docs: new trace2.advertiseSID option\n -: ---------- > 1: 184cabb6f5 docs: new capability to advertise session IDs\n -: ---------- > 2: 937534065a docs: new transfer.advertiseSID option\n 3: 7f42aacd2b = 3: 227c709ba5 trace2: add a public function for getting the SID\n 4: 4912af5f2b ! 4: 612957b9d5 upload-pack: advertise trace2 SID in v0 capabilities\n @@ Metadata\n Author: Josh Steadmon \n \n ## Commit message ##\n - upload-pack: advertise trace2 SID in v0 capabilities\n + upload-pack: advertise session ID in v0 capabilities\n \n - When trace2 is enabled and trace2.advertiseSID is true, advertise\n - upload-pack's trace2 session ID via the new trace2-sid capability.\n + When transfer.advertiseSID is true, advertise upload-pack's session ID\n + via the new session-id capability.\n \n \n @@ upload-pack.c: struct upload_pack_data {\n \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n -+\tunsigned advertise_trace2_sid : 1;\n ++\tunsigned advertise_sid : 1;\n };\n \n static void upload_pack_data_init(struct upload_pack_data *data)\n @@ upload-pack.c: static void upload_pack_data_init(struct upload_pack_data *data)\n \tpacket_writer_init(&data->writer, 1);\n \n \tdata->keepalive = 5;\n -+\tdata->advertise_trace2_sid = 0;\n ++\tdata->advertise_sid = 0;\n }\n \n static void upload_pack_data_clear(struct upload_pack_data *data)\n @@ upload-pack.c: static void format_symref_info(struct strbuf *buf, struct string_\n \t\tstrbuf_addf(buf, \" symref=%s:%s\", item->string, (char *)item->util);\n }\n \n -+static void format_trace2_info(struct strbuf *buf, struct upload_pack_data *d) {\n -+\tif (d->advertise_trace2_sid && trace2_is_enabled())\n -+\t\tstrbuf_addf(buf, \" trace2-sid=%s\", trace2_session_id());\n ++static void format_session_id(struct strbuf *buf, struct upload_pack_data *d) {\n ++\tif (d->advertise_sid)\n ++\t\tstrbuf_addf(buf, \" session-id=%s\", trace2_session_id());\n +}\n +\n static int send_ref(const char *refname, const struct object_id *oid,\n @@ upload-pack.c: static int send_ref(const char *refname, const struct object_id *\n \n \tif (capabilities) {\n \t\tstruct strbuf symref_info = STRBUF_INIT;\n -+\t\tstruct strbuf trace2_info = STRBUF_INIT;\n ++\t\tstruct strbuf session_id = STRBUF_INIT;\n \n \t\tformat_symref_info(&symref_info, &data->symref);\n -\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n -+\t\tformat_trace2_info(&trace2_info, data);\n ++\t\tformat_session_id(&session_id, data);\n +\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n @@ upload-pack.c: static int send_ref(const char *refname, const struct object_id *\n \t\t\t data->stateless_rpc ? \" no-done\" : \"\",\n \t\t\t symref_info.buf,\n \t\t\t data->allow_filter ? \" filter\" : \"\",\n -+\t\t\t trace2_info.buf,\n ++\t\t\t session_id.buf,\n \t\t\t the_hash_algo->name,\n \t\t\t git_user_agent_sanitized());\n \t\tstrbuf_release(&symref_info);\n -+\t\tstrbuf_release(&trace2_info);\n ++\t\tstrbuf_release(&session_id);\n \t} else {\n \t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n @@ upload-pack.c: static int upload_pack_config(const char *var, const char *value,\n \t\tdata->allow_sideband_all = git_config_bool(var, value);\n \t} else if (!strcmp(\"core.precomposeunicode\", var)) {\n \t\tprecomposed_unicode = git_config_bool(var, value);\n -+\t} else if (!strcmp(\"trace2.advertisesid\", var)) {\n -+\t\tdata->advertise_trace2_sid = git_config_bool(var, value);\n ++\t} else if (!strcmp(\"transfer.advertisesid\", var)) {\n ++\t\tdata->advertise_sid = git_config_bool(var, value);\n \t}\n \n \tif (current_config_scope() != CONFIG_SCOPE_LOCAL &&\n 5: 7003189c81 ! 5: 32fe78f3e9 receive-pack: advertise trace2 SID in v0 capabilities\n @@ Metadata\n Author: Josh Steadmon \n \n ## Commit message ##\n - receive-pack: advertise trace2 SID in v0 capabilities\n + receive-pack: advertise session ID in v0 capabilities\n \n - When trace2 is enabled and trace2.advertiseSID is true, advertise\n - receive-pack's trace2 session ID via the new trace2-sid capability.\n + When transfer.advertiseSID is true, advertise receive-pack's session ID\n + via the new session-id capability.\n \n \n @@ builtin/receive-pack.c: static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int advertise_atomic_push = 1;\n static int advertise_push_options;\n -+static int advertise_trace2_sid;\n ++static int advertise_sid;\n static int unpack_limit = 100;\n static off_t max_input_size;\n static int report_status;\n @@ builtin/receive-pack.c: static int receive_pack_config(const char *var, const ch\n \t\treturn 0;\n \t}\n \n -+\tif (strcmp(var, \"trace2.advertisesid\") == 0) {\n -+\t\tadvertise_trace2_sid = git_config_bool(var, value);\n ++\tif (strcmp(var, \"transfer.advertisesid\") == 0) {\n ++\t\tadvertise_sid = git_config_bool(var, value);\n +\t\treturn 0;\n +\t}\n +\n @@ builtin/receive-pack.c: static void show_ref(const char *path, const struct obje\n \t\t\tstrbuf_addf(&cap, \" push-cert=%s\", push_cert_nonce);\n \t\tif (advertise_push_options)\n \t\t\tstrbuf_addstr(&cap, \" push-options\");\n -+\t\tif (advertise_trace2_sid && trace2_is_enabled())\n -+\t\t\tstrbuf_addf(&cap, \" trace2-sid=%s\", trace2_session_id());\n ++\t\tif (advertise_sid)\n ++\t\t\tstrbuf_addf(&cap, \" session-id=%s\", trace2_session_id());\n \t\tstrbuf_addf(&cap, \" object-format=%s\", the_hash_algo->name);\n \t\tstrbuf_addf(&cap, \" agent=%s\", git_user_agent_sanitized());\n \t\tpacket_write_fmt(1, \"%s %s%c%s\\n\",\n 6: 9573428cc0 ! 6: fe6731cc09 serve: advertise trace2 SID in v2 capabilities\n @@ Metadata\n Author: Josh Steadmon \n \n ## Commit message ##\n - serve: advertise trace2 SID in v2 capabilities\n + serve: advertise session ID in v2 capabilities\n \n - When trace2 is enabled and trace2.advertiseSID is true, advertise the\n - server's trace2 session ID for all protocol v2 connections via the new\n - trace2-sid capability.\n + When transfer.advertiseSID is true, advertise the server's session ID\n + for all protocol v2 connections via the new session-id capability.\n \n \n @@ serve.c\n #include \"serve.h\"\n #include \"upload-pack.h\"\n \n -+static int advertise_trace2_sid;\n ++static int advertise_sid;\n +\n static int always_advertise(struct repository *r,\n \t\t\t struct strbuf *value)\n @@ serve.c: static int object_format_advertise(struct repository *r,\n \treturn 1;\n }\n \n -+static int trace2_advertise(struct repository *r, struct strbuf *value)\n ++static int session_id_advertise(struct repository *r, struct strbuf *value)\n +{\n -+\tif (!advertise_trace2_sid || !trace2_is_enabled())\n ++\tif (!advertise_sid)\n +\t\treturn 0;\n +\tif (value)\n +\t\tstrbuf_addstr(value, trace2_session_id());\n @@ serve.c: static struct protocol_capability capabilities[] = {\n \t{ \"fetch\", upload_pack_advertise, upload_pack_v2 },\n \t{ \"server-option\", always_advertise, NULL },\n \t{ \"object-format\", object_format_advertise, NULL },\n -+\t{ \"trace2-sid\", trace2_advertise, NULL },\n ++\t{ \"session-id\", session_id_advertise, NULL },\n };\n \n static void advertise_capabilities(void)\n @@ serve.c: static int process_request(void)\n /* Main serve loop for protocol version 2 */\n void serve(struct serve_options *options)\n {\n -+\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n ++\tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n +\n \tif (options->advertise_capabilities || !options->stateless_rpc) {\n \t\t/* serve by default supports v2 */\n 7: 21bdbf23f3 ! 7: 014cae8dc1 transport: log received server trace2 SID\n @@ Metadata\n Author: Josh Steadmon \n \n ## Commit message ##\n - transport: log received server trace2 SID\n + transport: log received server session ID\n \n - When a client receives a trace2-sid capability from a protocol v0, v1,\n + When a client receives a session-id capability from a protocol v0, v1,\n or v2 server, log the received session ID via a trace2 data event.\n \n \n - ## t/t5705-trace2-sid-in-capabilities.sh (new) ##\n + ## t/t5705-session-id-in-capabilities.sh (new) ##\n @@\n +#!/bin/sh\n +\n -+test_description='trace2 SID in capabilities'\n ++test_description='session ID in capabilities'\n +\n +. ./test-lib.sh\n +\n +REPO=\"$(pwd)/repo\"\n +LOCAL_PRISTINE=\"$(pwd)/local_pristine\"\n +\n -+test_expect_success 'setup repos for trace2 SID capability tests' '\n ++test_expect_success 'setup repos for session ID capability tests' '\n +\tgit init \"$REPO\" &&\n +\ttest_commit -C \"$REPO\" a &&\n +\tgit clone \"file://$REPO\" \"$LOCAL_PRISTINE\" &&\n @@ t/t5705-trace2-sid-in-capabilities.sh (new)\n +\n +for PROTO in 0 1 2\n +do\n -+\ttest_expect_success \"trace2 session IDs not advertised by default (fetch v${PROTO})\" '\n ++\ttest_expect_success \"session IDs not advertised by default (fetch v${PROTO})\" '\n +\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n +\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n +\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n @@ t/t5705-trace2-sid-in-capabilities.sh (new)\n +\t\ttest -z \"$(grep \\\"key\\\":\\\"server-sid\\\" tr2-client-events)\"\n +\t'\n +\n -+\ttest_expect_success \"trace2 session IDs not advertised by default (push v${PROTO})\" '\n ++\ttest_expect_success \"session IDs not advertised by default (push v${PROTO})\" '\n +\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n +\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n +\t\tgit -C local pull --no-rebase origin &&\n @@ t/t5705-trace2-sid-in-capabilities.sh (new)\n +done\n +\n +test_expect_success 'enable SID advertisement' '\n -+\tgit -C \"$REPO\" config trace2.advertiseSID true &&\n -+\tgit -C \"$LOCAL_PRISTINE\" config trace2.advertiseSID true\n ++\tgit -C \"$REPO\" config transfer.advertiseSID true &&\n ++\tgit -C \"$LOCAL_PRISTINE\" config transfer.advertiseSID true\n +'\n +\n +for PROTO in 0 1 2\n +do\n -+\ttest_expect_success \"trace2 session IDs advertised (fetch v${PROTO})\" '\n ++\ttest_expect_success \"session IDs advertised (fetch v${PROTO})\" '\n +\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n +\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n +\t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n @@ t/t5705-trace2-sid-in-capabilities.sh (new)\n +\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n +\t'\n +\n -+\ttest_expect_success \"trace2 session IDs advertised (push v${PROTO})\" '\n ++\ttest_expect_success \"session IDs advertised (push v${PROTO})\" '\n +\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n +\t\tcp -r \"$LOCAL_PRISTINE\" local &&\n +\t\tgit -C local pull --no-rebase origin &&\n @@ transport.c: static struct ref *handshake(struct transport *transport, int for_p\n \tstruct ref *refs = NULL;\n \tstruct packet_reader reader;\n +\tint sid_len;\n -+\tconst char *server_trace2_sid;\n ++\tconst char *server_sid;\n \n \tconnect_setup(transport, for_push);\n \n @@ transport.c: static struct ref *handshake(struct transport *transport, int for_p\n \tdata->version = discover_version(&reader);\n \tswitch (data->version) {\n \tcase protocol_v2:\n -+\t\tif (server_feature_v2(\"trace2-sid\", &server_trace2_sid))\n -+\t\t\ttrace2_data_string(\"trace2\", NULL, \"server-sid\", server_trace2_sid);\n ++\t\tif (server_feature_v2(\"session-id\", &server_sid))\n ++\t\t\ttrace2_data_string(\"transfer\", NULL, \"server-sid\", server_sid);\n \t\tif (must_list_refs)\n \t\t\tget_remote_refs(data->fd[1], &reader, &refs, for_push,\n \t\t\t\t\tref_prefixes,\n @@ transport.c: static struct ref *handshake(struct transport *transport, int for_p\n \t\t\t\t for_push ? REF_NORMAL : 0,\n \t\t\t\t &data->extra_have,\n \t\t\t\t &data->shallow);\n -+\t\tserver_trace2_sid = server_feature_value(\"trace2-sid\", &sid_len);\n -+\t\tif (server_trace2_sid) {\n -+\t\t\tchar *server_sid = xstrndup(server_trace2_sid, sid_len);\n -+\t\t\ttrace2_data_string(\"trace2\", NULL, \"server-sid\", server_sid);\n -+\t\t\tfree(server_sid);\n ++\t\tserver_sid = server_feature_value(\"session-id\", &sid_len);\n ++\t\tif (server_sid) {\n ++\t\t\tchar *sid = xstrndup(server_sid, sid_len);\n ++\t\t\ttrace2_data_string(\"transfer\", NULL, \"server-sid\", sid);\n ++\t\t\tfree(sid);\n +\t\t}\n \t\tbreak;\n \tcase protocol_unknown_version:\n 8: 11b5b1b54f ! 8: fc9f6c9286 fetch-pack: advertise trace2 SID in capabilities\n @@ Metadata\n Author: Josh Steadmon \n \n ## Commit message ##\n - fetch-pack: advertise trace2 SID in capabilities\n + fetch-pack: advertise session ID in capabilities\n \n - When trace2 is enabled, the server sent a trace2-sid capability, and\n - trace2.advertiseSID is true, advertise fetch-pack's own session ID back\n - to the server.\n + When the server sent a session-id capability and transfer.advertiseSID\n + is true, advertise fetch-pack's own session ID back to the server.\n \n \n @@ fetch-pack.c: static int fetch_fsck_objects = -1;\n static int transfer_fsck_objects = -1;\n static int agent_supported;\n static int server_supports_filtering;\n -+static int server_sent_trace2_sid;\n -+static int advertise_trace2_sid;\n ++static int advertise_sid;\n static struct shallow_lock shallow_lock;\n static const char *alternate_shallow_file;\n static struct strbuf fsck_msg_types = STRBUF_INIT;\n @@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,\n \t\t\tif (deepen_not_ok) strbuf_addstr(&c, \" deepen-not\");\n \t\t\tif (agent_supported) strbuf_addf(&c, \" agent=%s\",\n \t\t\t\t\t\t\t git_user_agent_sanitized());\n -+\t\t\tif (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())\n -+\t\t\t\tstrbuf_addf(&c, \" trace2-sid=%s\", trace2_session_id());\n ++\t\t\tif (advertise_sid)\n ++\t\t\t\tstrbuf_addf(&c, \" session-id=%s\", trace2_session_id());\n \t\t\tif (args->filter_options.choice)\n \t\t\t\tstrbuf_addstr(&c, \" filter\");\n \t\t\tpacket_buf_write(&req_buf, \"want %s%s\\n\", remote_hex, c.buf);\n @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t agent_len, agent_feature);\n \t}\n \n -+\tif (server_supports(\"trace2-sid\"))\n -+\t\tserver_sent_trace2_sid = 1;\n ++\tif (!server_supports(\"session-id\"))\n ++\t\tadvertise_sid = 0;\n +\n \tif (server_supports(\"shallow\"))\n \t\tprint_verbose(args, _(\"Server supports %s\"), \"shallow\");\n @@ fetch-pack.c: static int send_fetch_request(struct fetch_negotiator *negotiator,\n \t\tpacket_buf_write(&req_buf, \"command=fetch\");\n \tif (server_supports_v2(\"agent\", 0))\n \t\tpacket_buf_write(&req_buf, \"agent=%s\", git_user_agent_sanitized());\n -+\tif (advertise_trace2_sid && server_supports_v2(\"trace2-sid\", 0) && trace2_is_enabled())\n -+\t\tpacket_buf_write(&req_buf, \"trace2-sid=%s\", trace2_session_id());\n ++\tif (advertise_sid && server_supports_v2(\"session-id\", 0))\n ++\t\tpacket_buf_write(&req_buf, \"session-id=%s\", trace2_session_id());\n \tif (args->server_options && args->server_options->nr &&\n \t server_supports_v2(\"server-option\", 1)) {\n \t\tint i;\n @@ fetch-pack.c: static void fetch_pack_config(void)\n \tgit_config_get_bool(\"repack.usedeltabaseoffset\", &prefer_ofs_delta);\n \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n -+\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n ++\tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n \tif (!uri_protocols.nr) {\n \t\tchar *str;\n \n 9: 23f44bc904 ! 9: bde9c1d97a upload-pack, serve: log received client trace2 SID\n @@ Metadata\n Author: Josh Steadmon \n \n ## Commit message ##\n - upload-pack, serve: log received client trace2 SID\n + upload-pack, serve: log received client session ID\n \n When upload-pack (protocol v0/v1) or a protocol v2 server receives a\n - trace2-sid capability from a client, log the received session ID via a\n + session-id capability from a client, log the received session ID via a\n trace2 data event.\n \n @@ serve.c: static int process_request(void)\n \n \tcheck_algorithm(the_repository, &keys);\n \n -+\tif (has_capability(&keys, \"trace2-sid\", &client_sid))\n -+\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n ++\tif (has_capability(&keys, \"session-id\", &client_sid))\n ++\t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n +\n \tcommand->command(the_repository, &keys, &reader);\n \n \tstrvec_clear(&keys);\n \n - ## t/t5705-trace2-sid-in-capabilities.sh ##\n -@@ t/t5705-trace2-sid-in-capabilities.sh: test_expect_success 'setup repos for trace2 SID capability tests' '\n + ## t/t5705-session-id-in-capabilities.sh ##\n +@@ t/t5705-session-id-in-capabilities.sh: test_expect_success 'setup repos for session ID capability tests' '\n for PROTO in 0 1 2\n do\n - \ttest_expect_success \"trace2 session IDs not advertised by default (fetch v${PROTO})\" '\n + \ttest_expect_success \"session IDs not advertised by default (fetch v${PROTO})\" '\n -\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n +\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n @@ t/t5705-trace2-sid-in-capabilities.sh: test_expect_success 'setup repos for trac\n +\t\ttest -z \"$(grep \\\"key\\\":\\\"client-sid\\\" tr2-server-events)\"\n \t'\n \n - \ttest_expect_success \"trace2 session IDs not advertised by default (push v${PROTO})\" '\n -@@ t/t5705-trace2-sid-in-capabilities.sh: test_expect_success 'enable SID advertisement' '\n + \ttest_expect_success \"session IDs not advertised by default (push v${PROTO})\" '\n +@@ t/t5705-session-id-in-capabilities.sh: test_expect_success 'enable SID advertisement' '\n for PROTO in 0 1 2\n do\n - \ttest_expect_success \"trace2 session IDs advertised (fetch v${PROTO})\" '\n + \ttest_expect_success \"session IDs advertised (fetch v${PROTO})\" '\n -\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n +\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n -+\t\tgit -C local config trace2.advertiseSID true &&\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n -\t\tgit -c protocol.version=$PROTO -C local fetch origin &&\n -\t\tgrep \\\"key\\\":\\\"server-sid\\\" tr2-client-events\n @@ t/t5705-trace2-sid-in-capabilities.sh: test_expect_success 'enable SID advertise\n +\t\tgrep \\\"key\\\":\\\"client-sid\\\" tr2-server-events\n \t'\n \n - \ttest_expect_success \"trace2 session IDs advertised (push v${PROTO})\" '\n + \ttest_expect_success \"session IDs advertised (push v${PROTO})\" '\n \n ## upload-pack.c ##\n @@ upload-pack.c: static void receive_needs(struct upload_pack_data *data,\n @@ upload-pack.c: static void receive_needs(struct upload_pack_data *data,\n \t\treset_timeout(data->timeout);\n \t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n @@ upload-pack.c: static void receive_needs(struct upload_pack_data *data,\n - \t\tif (data->allow_filter &&\n \t\t parse_feature_request(features, \"filter\"))\n \t\t\tdata->filter_capability_requested = 1;\n -+\t\tif ((arg = parse_feature_value(features, \"trace2-sid\", &feature_len, NULL)))\n -+\t\t{\n + \n ++\t\targ = parse_feature_value(features, \"session-id\", &feature_len, NULL);\n ++\t\tif (arg) {\n +\t\t\tchar *client_sid = xstrndup(arg, feature_len);\n -+\t\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n ++\t\t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n +\t\t\tfree(client_sid);\n +\t\t}\n - \n ++\n \t\to = parse_object(the_repository, &oid_buf);\n \t\tif (!o) {\n + \t\t\tpacket_writer_error(&data->writer,\n10: c0b1ffc6d8 ! 10: 012949e7da send-pack: advertise trace2 SID in capabilities\n @@ Metadata\n Author: Josh Steadmon \n \n ## Commit message ##\n - send-pack: advertise trace2 SID in capabilities\n + send-pack: advertise session ID in capabilities\n \n - When trace2 is enabled, the server sent a trace2-sid capability, and\n - trace2.advertiseSID is true, advertise send-pack's own session ID back\n - to the server.\n + When the server sent a session-id capability and transfer.advertiseSID\n + is true, advertise send-pack's own session ID back to the server.\n \n \n @@ send-pack.c: int send_pack(struct send_pack_args *args,\n \tint use_sideband = 0;\n \tint quiet_supported = 0;\n \tint agent_supported = 0;\n -+\tint server_sent_trace2_sid = 0;\n -+\tint advertise_trace2_sid = 0;\n ++\tint advertise_sid = 0;\n \tint use_atomic = 0;\n \tint atomic_supported = 0;\n \tint use_push_options = 0;\n @@ send-pack.c: int send_pack(struct send_pack_args *args,\n \tconst char *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n \n -+\tgit_config_get_bool(\"trace2.advertisesid\", &advertise_trace2_sid);\n ++\tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n +\n \t/* Does the other end support the reporting? */\n \tif (server_supports(\"report-status-v2\"))\n @@ send-pack.c: int send_pack(struct send_pack_args *args,\n \t\tquiet_supported = 1;\n \tif (server_supports(\"agent\"))\n \t\tagent_supported = 1;\n -+\tif (server_supports(\"trace2-sid\"))\n -+\t\tserver_sent_trace2_sid = 1;\n ++\tif (!server_supports(\"session-id\"))\n ++\t\tadvertise_sid = 0;\n \tif (server_supports(\"no-thin\"))\n \t\targs->use_thin_pack = 0;\n \tif (server_supports(\"atomic\"))\n @@ send-pack.c: int send_pack(struct send_pack_args *args,\n \t\tstrbuf_addf(&cap_buf, \" object-format=%s\", the_hash_algo->name);\n \tif (agent_supported)\n \t\tstrbuf_addf(&cap_buf, \" agent=%s\", git_user_agent_sanitized());\n -+\tif (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled())\n -+\t\tstrbuf_addf(&cap_buf, \" trace2-sid=%s\", trace2_session_id());\n ++\tif (advertise_sid)\n ++\t\tstrbuf_addf(&cap_buf, \" session-id=%s\", trace2_session_id());\n \n \t/*\n \t * NEEDSWORK: why does delete-refs have to be so specific to\n11: c47eddd9df ! 11: ea2a318f2b receive-pack: log received client trace2 SID\n @@ Metadata\n Author: Josh Steadmon \n \n ## Commit message ##\n - receive-pack: log received client trace2 SID\n + receive-pack: log received client session ID\n \n - When receive-pack receives a trace2-sid capability from the client, log\n + When receive-pack receives a session-id capability from the client, log\n the received session ID via a trace2 data event.\n \n @@ builtin/receive-pack.c: static struct command *read_head_info(struct packet_read\n \t\tif (linelen < reader->pktlen) {\n \t\t\tconst char *feature_list = reader->line + linelen + 1;\n \t\t\tconst char *hash = NULL;\n -+\t\t\tconst char *client_trace2_sid;\n ++\t\t\tconst char *client_sid;\n \t\t\tint len = 0;\n \t\t\tif (parse_feature_request(feature_list, \"report-status\"))\n \t\t\t\treport_status = 1;\n @@ builtin/receive-pack.c: static struct command *read_head_info(struct packet_read\n \t\t\t}\n \t\t\tif (xstrncmpz(the_hash_algo->name, hash, len))\n \t\t\t\tdie(\"error: unsupported object format '%s'\", hash);\n -+\t\t\tclient_trace2_sid = parse_feature_value(feature_list, \"trace2-sid\", &len, NULL);\n -+\t\t\tif (client_trace2_sid) {\n -+\t\t\t\tchar *client_sid = xstrndup(client_trace2_sid, len);\n -+\t\t\t\ttrace2_data_string(\"trace2\", NULL, \"client-sid\", client_sid);\n -+\t\t\t\tfree(client_sid);\n ++\t\t\tclient_sid = parse_feature_value(feature_list, \"session-id\", &len, NULL);\n ++\t\t\tif (client_sid) {\n ++\t\t\t\tchar *sid = xstrndup(client_sid, len);\n ++\t\t\t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n ++\t\t\t\tfree(sid);\n +\t\t\t}\n \t\t}\n \n \t\tif (!strcmp(reader->line, \"push-cert\")) {\n \n - ## t/t5705-trace2-sid-in-capabilities.sh ##\n -@@ t/t5705-trace2-sid-in-capabilities.sh: do\n + ## t/t5705-session-id-in-capabilities.sh ##\n +@@ t/t5705-session-id-in-capabilities.sh: do\n \t'\n \n - \ttest_expect_success \"trace2 session IDs not advertised by default (push v${PROTO})\" '\n + \ttest_expect_success \"session IDs not advertised by default (push v${PROTO})\" '\n -\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n +\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n +\t\ttest_when_finished \"git -C local push --delete origin new-branch\" &&\n @@ t/t5705-trace2-sid-in-capabilities.sh: do\n \t'\n done\n \n -@@ t/t5705-trace2-sid-in-capabilities.sh: do\n +@@ t/t5705-session-id-in-capabilities.sh: do\n \t'\n \n - \ttest_expect_success \"trace2 session IDs advertised (push v${PROTO})\" '\n + \ttest_expect_success \"session IDs advertised (push v${PROTO})\" '\n -\t\ttest_when_finished \"rm -rf local tr2-client-events\" &&\n +\t\ttest_when_finished \"rm -rf local tr2-client-events tr2-server-events\" &&\n +\t\ttest_when_finished \"git -C local push --delete origin new-branch\" &&\n-- \n2.29.2.222.g5d2a92d10f8-goog\n\n"},{"id":"409746","messageId":"20201111225318.GG36751@google.com","threadId":"54527","inReplyTo":"xmqqpn4slt8o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 07/11] transport: log received server trace2 SID","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-11T22:53:18Z","receivedAt":"2020-11-12T01:50:33Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.11.04 13:14, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > When a client receives a trace2-sid capability from a protocol v0, v1,\n> > or v2 server, log the received session ID via a trace2 data event.\n> \n> Would this pose a new security threat surface? Just wondering if we\n> want to ignore the capability if it is not enabled on our end with\n> the configuration.\n> \n> Thanks.\n\nAs Jeff pointed out, the json-writer handles string escapes, so I don't\nthink we could cause any trouble with the trace2 \"event\" target. The\n\"normal\" target ignores data events, so this would not show up in a\nnormal trace2 log. The \"perf\" target doesn't seem to do any escaping,\nbut it's intended to be human readable rather than parseable, so I'm not\nsure if we need to worry there. Jeff, any thoughts?\n"},{"id":"409760","messageId":"87d00ihdqf.fsf@evledraar.gmail.com","threadId":"54527","inReplyTo":"634cf106-7bc2-e8fa-5745-5e2d26b50e14@jeffhostetler.com","subject":"Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-11-12T14:05:44Z","receivedAt":"2020-11-12T14:05:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 05 2020, Jeff Hostetler wrote:\n\n> However, the opportunity to introduce a prefix as I suggested earlier\n> does offer the opportunity to introduce funky chars that would not\n> have any protection, so you may want to c-quote the string when\n> inserting it into the wire protocol.\n>\n> $ GIT_TRACE2_EVENT=1 GIT_TRACE2_PARENT_SID=hello git version\n> {\"event\":\"version\",\"sid\":\"hello/20201030T154143.660608Z-H86606a97- \n> P00001d30\",...}\n> ...\n>\n> (Allowing the user to insert a prefix like that has been useful for\n> tracking control/experiment testing, so I wouldn't want to disable\n> it.)\n\nAFAICT the way it's documented now is the \"is the session-id[...]\"\nparagraph in api-trace2.txt.\n\nI'd like to see us document the implementation a bit better and\nexplicitly support the \"hack\" of setting GIT_TRACE2_PARENT_SID=hello.\n\nI.e. maybe I've missed something but we just say \"session-id is\nprepended with the session-id of the parent\" but don't mention that we\nseparate them with slashes, so you can split on that to get the depth &\nindividual ID's.\n\nMy reading of the updated doc patch in v3 is that not allowing\n\"non-printable or whitespace\" allows you to e.g. have slashes in those\ncustom session IDs, which would be quite inconvenient since it would\nbreak that property.\n\nAnd we should explicitly support the GIT_TRACE2_PARENT_SID=* setting\nfrom an external process, and make the SID definition loose enough to\nallow for SIDs that don't look like Git's in that chain. I.e. a useful\nproperty (and one I've seen in the wild) is to have some external\nprogramt that already has SIDs/UUID run IDs spawn git, setting\nGIT_TRACE2_PARENT_SID= makes things convenient for\nthe purposes of logging.n\n"},{"id":"409774","messageId":"xmqqo8k2jxa2.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"87d00ihdqf.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-12T17:32:53Z","receivedAt":"2020-11-12T17:33:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> AFAICT the way it's documented now is the \"is the session-id[...]\"\n> paragraph in api-trace2.txt.\n>\n> I'd like to see us document the implementation a bit better and\n> explicitly support the \"hack\" of setting GIT_TRACE2_PARENT_SID=hello.\n>\n> I.e. maybe I've missed something but we just say \"session-id is\n> prepended with the session-id of the parent\" but don't mention that we\n> separate them with slashes, so you can split on that to get the depth &\n> individual ID's.\n>\n> My reading of the updated doc patch in v3 is that not allowing\n> \"non-printable or whitespace\" allows you to e.g. have slashes in those\n> custom session IDs, which would be quite inconvenient since it would\n> break that property.\n\nA few things I want to see stakeholders agree on:\n\n - In \"a/b/c\", what's the \"session ID\" of the leaf child process?\n \"a/b/c\"? or \"c\"? If the former (which is what I am guessing),\n do we have name to refer to \"b\" or \"c\" alone (if not, we should\n have one)?\n\n - Do we encourage/force other implementations of Git protocol to\n adopt a similar \"slash-separated non-whitespace ASCII printable\"\n structure? I do not think such a structure is too limiting but\n others may feel differently. Or is a \"session ID\" supposed to be\n an opaque token without any structure, and upon seeing \"a/b/c\"\n the recipient should not read anything into its slash, or any\n relation with another session whose ID is \"a/b/d\"?\n\n> And we should explicitly support the GIT_TRACE2_PARENT_SID=* setting\n> from an external process, and make the SID definition loose enough to\n> allow for SIDs that don't look like Git's in that chain. I.e. a useful\n> property (and one I've seen in the wild) is to have some external\n> programt that already has SIDs/UUID run IDs spawn git, setting\n> GIT_TRACE2_PARENT_SID= makes things convenient for\n> the purposes of logging.n\n"},{"id":"409801","messageId":"1a60fdab-a203-56d0-8dcd-43f88c29d3ff@jeffhostetler.com","threadId":"54527","inReplyTo":"20201111225318.GG36751@google.com","subject":"Re: [PATCH v2 07/11] transport: log received server trace2 SID","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2020-11-12T21:30:44Z","receivedAt":"2020-11-12T21:30:50Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 11/11/20 5:53 PM, Josh Steadmon wrote:\n> On 2020.11.04 13:14, Junio C Hamano wrote:\n>> Josh Steadmon writes:\n>>\n>>> When a client receives a trace2-sid capability from a protocol v0, v1,\n>>> or v2 server, log the received session ID via a trace2 data event.\n>>\n>> Would this pose a new security threat surface? Just wondering if we\n>> want to ignore the capability if it is not enabled on our end with\n>> the configuration.\n>>\n>> Thanks.\n> \n> As Jeff pointed out, the json-writer handles string escapes, so I don't\n> think we could cause any trouble with the trace2 \"event\" target. The\n> \"normal\" target ignores data events, so this would not show up in a\n> normal trace2 log. The \"perf\" target doesn't seem to do any escaping,\n> but it's intended to be human readable rather than parseable, so I'm not\n> sure if we need to worry there. Jeff, any thoughts?\n> \n\nOnly the \"event\" target prints the SID and it is JSON quoted there.\n\nNeither the \"perf\" nor \"normal\" target print them. The \"perf\" target\ndoes print the SID \"depth\" parameter (which is the number of slashes\nin the complete SID).\n\nMy earlier concerns were about whitespace, CL/LF and other\nnon-printables in the wire protocol and etc.\n\nJeff\n\n"},{"id":"409803","messageId":"1b044820-2616-36f4-9d0b-368a74b9d125@jeffhostetler.com","threadId":"54527","inReplyTo":"xmqqo8k2jxa2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2020-11-12T22:10:36Z","receivedAt":"2020-11-12T22:10:40Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 11/12/20 12:32 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n> \n>> AFAICT the way it's documented now is the \"is the session-id[...]\"\n>> paragraph in api-trace2.txt.\n>>\n>> I'd like to see us document the implementation a bit better and\n>> explicitly support the \"hack\" of setting GIT_TRACE2_PARENT_SID=hello.\n\nI've occasionally used that hack for control/experiment-type\ntesting, but not that often.\n\nI was more pointing out that I had to use that environment\ninheritance mechanism so that child processes can be associated\nwith their Git process ancestry. And so it is possible for someone\nto abuse that mechanism for other purposes (and introduce injections\ninto what Josh is proposing).\n\n>>\n>> I.e. maybe I've missed something but we just say \"session-id is\n>> prepended with the session-id of the parent\" but don't mention that we\n>> separate them with slashes, so you can split on that to get the depth &\n>> individual ID's.\n>>\n>> My reading of the updated doc patch in v3 is that not allowing\n>> \"non-printable or whitespace\" allows you to e.g. have slashes in those\n>> custom session IDs, which would be quite inconvenient since it would\n>> break that property.\n> \n> A few things I want to see stakeholders agree on:\n> \n> - In \"a/b/c\", what's the \"session ID\" of the leaf child process?\n> \"a/b/c\"? or \"c\"? If the former (which is what I am guessing),\n> do we have name to refer to \"b\" or \"c\" alone (if not, we should\n> have one)?\n\nI consider a process' SID to be the complete \"a/b/c\" string.\nBut I do know that when I look at my logging data, that I will\nalso find data for a process with SID of \"a\" and data for another\nprocess with SID \"a/b\".\n\nSo perhaps we should have names for:\n\n [1] \"a/b/c\" -- my process' complete SID name\n [2] \"c\" -- my process' SID component name\n [3] \"a/b\" -- my parent's complete SID name\n\n> \n> - Do we encourage/force other implementations of Git protocol to\n> adopt a similar \"slash-separated non-whitespace ASCII printable\"\n> structure? I do not think such a structure is too limiting but\n> others may feel differently. Or is a \"session ID\" supposed to be\n> an opaque token without any structure, and upon seeing \"a/b/c\"\n> the recipient should not read anything into its slash, or any\n> relation with another session whose ID is \"a/b/d\"?\n\nWhen analyzing Git perf data, there are times when you basically want\nto \"SELECT * where SID startswith 'a/b/' ...\" and summarize over the\nchild processes of \"a/b\". So data from \"a/b/c\" and \"a/b/d\" would be\naggregated. (I do have some of that data in the \"child_exit\" events\nreported for the \"a/b\" process, but sometimes you need to actually\nsee the records for the child processes.)\n\nSo I guess I'm saying that the hierarchy has been useful and we should\ntry to keep it as is.\n\n> \n>> And we should explicitly support the GIT_TRACE2_PARENT_SID=* setting\n>> from an external process, and make the SID definition loose enough to\n>> allow for SIDs that don't look like Git's in that chain. I.e. a useful\n>> property (and one I've seen in the wild) is to have some external\n>> programt that already has SIDs/UUID run IDs spawn git, setting\n>> GIT_TRACE2_PARENT_SID= makes things convenient for\n>> the purposes of logging.n\n\nYes, it can be useful for external tools that drive Git to be able to\nset a SID prefix to help track that into Git process.\n\nLikewise, it would be nice to add code to some of the Git shell script\ncommands (such as git-mergetool and git-prompt) to augment the SID\nbeing passed to child Git commands to help track why they are being\ninvoked.\n\nJeff\n\n\n\n"},{"id":"410809","messageId":"xmqqpn41ccor.fsf@gitster.c.googlers.com","threadId":"54527","inReplyTo":"cover.1605136908.git.steadmon@google.com","subject":"Re: [PATCH v3 00/11] Advertise session ID in protocol capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T22:08:04Z","receivedAt":"2020-11-25T22:08:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> In order to more easily debug remote operations, it is useful to be able\n> to identify sessions on either side of the connection. This series\n> allows clients and servers to provide a unique session ID via a new\n> \"session-id\" protocol capability. This session ID can be logged on each\n> side of the connection, allowing logs to be joined during debugging\n> sessions.\n>\n> Changes from V2:\n> * De-emphasize connection to trace2\n> * Renamed capability from \"trace2-sid\" to \"session-id\"\n> * Noted (lack of) session ID structure in capability docs\n> * Advertise SID regardless of whether trace2 is enabled\n> * Simplify conditionals\n> * Style cleanups\n>\n> Changes since V1:\n> * Added a public trace2_session_id() function to trace2.{h,c}. Used this\n> in place of tr2_sid_get().\n> * Fixed a style issue regarding using NULL rather than 0.\n\nThis round didn't see any responses, but it has already been\nextensively reviewed in the previous two rounds and looking good.\n\nWill mark to be merged to 'next', but please holler to stop me if\nneeded (those in v2 review CC'ed).\n\nThanks.\n\n"},{"id":"410824","messageId":"20201125225659.GJ36751@google.com","threadId":"54527","inReplyTo":"xmqqpn41ccor.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 00/11] Advertise session ID in protocol capabilities","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-25T22:56:59Z","receivedAt":"2020-11-25T22:57:28Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.11.25 14:08, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > In order to more easily debug remote operations, it is useful to be able\n> > to identify sessions on either side of the connection. This series\n> > allows clients and servers to provide a unique session ID via a new\n> > \"session-id\" protocol capability. This session ID can be logged on each\n> > side of the connection, allowing logs to be joined during debugging\n> > sessions.\n> >\n> > Changes from V2:\n> > * De-emphasize connection to trace2\n> > * Renamed capability from \"trace2-sid\" to \"session-id\"\n> > * Noted (lack of) session ID structure in capability docs\n> > * Advertise SID regardless of whether trace2 is enabled\n> > * Simplify conditionals\n> > * Style cleanups\n> >\n> > Changes since V1:\n> > * Added a public trace2_session_id() function to trace2.{h,c}. Used this\n> > in place of tr2_sid_get().\n> > * Fixed a style issue regarding using NULL rather than 0.\n> \n> This round didn't see any responses, but it has already been\n> extensively reviewed in the previous two rounds and looking good.\n> \n> Will mark to be merged to 'next', but please holler to stop me if\n> needed (those in v2 review CC'ed).\n> \n> Thanks.\n\nI still intend to write some documentation to clarify the trace2 SID\nstructure, but I think that can be done as a separate series. Thanks!\n"}]}