{"thread":{"id":"60344","subject":"[PATCH] upload-pack: add tracing for fetches","startedAt":"2023-10-11T16:04:10Z","lastAt":"2023-10-30T20:25:39Z","messageCount":10,"participants":["Robert Coup via GitGitGadget","Taylor Blau","Jeff King","Robert Coup"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"483055","messageId":"pull.1598.git.1697040242703.gitgitgadget@gmail.com","threadId":"60344","inReplyTo":null,"subject":"[PATCH] upload-pack: add tracing for fetches","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-11T16:04:02Z","receivedAt":"2023-10-11T16:04:10Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup <robert@coup.net.nz>\n\nInformation on how users are accessing hosted repositories can be\nhelpful to server operators. For example, being able to broadly\ndifferentiate between fetches and initial clones; the use of shallow\nrepository features; or partial clone filters.\n\na29263c (fetch-pack: add tracing for negotiation rounds, 2022-08-02)\nadded some information on have counts to fetch-pack itself to help\ndiagnose negotiation; but from a git-upload-pack (server) perspective,\nthere's no means of accessing such information without using\nGIT_TRACE_PACKET to examine the protocol packets.\n\nImprove this by emitting a Trace2 JSON event from upload-pack with\nsummary information on the contents of a fetch request.\n\n* haves, wants, and want-ref counts can help determine (broadly) between\n  fetches and clones, and the use of single-branch, etc.\n* shallow clone depth, tip counts, and deepening options.\n* any partial clone filter type.\n\nSigned-off-by: Robert Coup <robert@coup.net.nz>\n---\n    upload-pack: add tracing for fetches\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1598%2Frcoup%2Frc-upload-pack-trace-info-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1598/rcoup/rc-upload-pack-trace-info-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1598\n\n t/t5500-fetch-pack.sh | 38 +++++++++++++++++++++++++++-----------\n upload-pack.c         | 29 +++++++++++++++++++++++++++++\n 2 files changed, 56 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex d18f2823d86..bb15ac34f77 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -132,13 +132,18 @@ test_expect_success 'single branch object count' '\n '\n \n test_expect_success 'single given branch clone' '\n-\tgit clone --single-branch --branch A \"file://$(pwd)/.\" branch-a &&\n-\ttest_must_fail git --git-dir=branch-a/.git rev-parse origin/B\n+\tGIT_TRACE2_EVENT=\"$(pwd)/branch-a/trace2_event\" \\\n+\t\tgit clone --single-branch --branch A \"file://$(pwd)/.\" branch-a &&\n+\ttest_must_fail git --git-dir=branch-a/.git rev-parse origin/B &&\n+\tgrep \\\"fetch-info\\\".*\\\"haves\\\":0 branch-a/trace2_event &&\n+\tgrep \\\"fetch-info\\\".*\\\"wants\\\":1 branch-a/trace2_event\n '\n \n test_expect_success 'clone shallow depth 1' '\n-\tgit clone --no-single-branch --depth 1 \"file://$(pwd)/.\" shallow0 &&\n-\ttest \"$(git --git-dir=shallow0/.git rev-list --count HEAD)\" = 1\n+\tGIT_TRACE2_EVENT=\"$(pwd)/shallow0/trace2_event\" \\\n+\t\tgit clone --no-single-branch --depth 1 \"file://$(pwd)/.\" shallow0 &&\n+\ttest \"$(git --git-dir=shallow0/.git rev-list --count HEAD)\" = 1 &&\n+\tgrep \\\"fetch-info\\\".*\\\"depth\\\":1 shallow0/trace2_event\n '\n \n test_expect_success 'clone shallow depth 1 with fsck' '\n@@ -235,7 +240,10 @@ test_expect_success 'add two more (part 2)' '\n test_expect_success 'deepening pull in shallow repo' '\n \t(\n \t\tcd shallow &&\n-\t\tgit pull --depth 4 .. B\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2_event\" \\\n+\t\t\tgit pull --depth 4 .. B &&\n+\t\tgrep \\\"fetch-info\\\".*\\\"depth\\\":4 trace2_event &&\n+\t\tgrep \\\"fetch-info\\\".*\\\"shallows\\\":2 trace2_event\n \t)\n '\n \n@@ -306,9 +314,12 @@ test_expect_success 'fetch --depth --no-shallow' '\n test_expect_success 'turn shallow to complete repository' '\n \t(\n \t\tcd shallow &&\n-\t\tgit fetch --unshallow &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2_event\" \\\n+\t\t\tgit fetch --unshallow &&\n \t\t! test -f .git/shallow &&\n-\t\tgit fsck --full\n+\t\tgit fsck --full &&\n+\t\tgrep \\\"fetch-info\\\".*\\\"shallows\\\":2 trace2_event &&\n+\t\tgrep \\\"fetch-info\\\".*\\\"depth\\\":2147483647 trace2_event\n \t)\n '\n \n@@ -826,13 +837,15 @@ test_expect_success 'clone shallow since ...' '\n '\n \n test_expect_success 'fetch shallow since ...' '\n-\tgit -C shallow11 fetch --shallow-since \"200000000 +0700\" origin &&\n+\tGIT_TRACE2_EVENT=$(pwd)/shallow11/trace2_event \\\n+\t\tgit -C shallow11 fetch --shallow-since \"200000000 +0700\" origin &&\n \tgit -C shallow11 log --pretty=tformat:%s origin/main >actual &&\n \tcat >expected <<-\\EOF &&\n \tthree\n \ttwo\n \tEOF\n-\ttest_cmp expected actual\n+\ttest_cmp expected actual &&\n+\tgrep \\\"fetch-info\\\".*\\\"deepen-since\\\":true shallow11/trace2_event\n '\n \n test_expect_success 'clone shallow since selects no commits' '\n@@ -987,13 +1000,16 @@ test_expect_success 'filtering by size' '\n \ttest_config -C server uploadpack.allowfilter 1 &&\n \n \ttest_create_repo client &&\n-\tgit -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&\n+\tGIT_TRACE2_EVENT=$(pwd)/client/trace2_event \\\n+\t\tgit -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&\n \n \t# Ensure that object is not inadvertently fetched\n \tcommit=$(git -C server rev-parse HEAD) &&\n \tblob=$(git hash-object server/one.t) &&\n \tgit -C client rev-list --objects --missing=allow-any \"$commit\" >oids &&\n-\t! grep \"$blob\" oids\n+\t! grep \"$blob\" oids &&\n+\n+\tgrep \\\"fetch-info\\\".*\\\"filter\\\":\\\"blob:limit\\\" client/trace2_event\n '\n \n test_expect_success 'filtering by size has no effect if support for it is not advertised' '\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 83f3d2651ab..165c1c6350a 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -33,6 +33,7 @@\n #include \"commit-reach.h\"\n #include \"shallow.h\"\n #include \"write-or-die.h\"\n+#include \"json-writer.h\"\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -1552,6 +1553,32 @@ static int parse_have(const char *line, struct oid_array *haves)\n \treturn 0;\n }\n \n+static void trace2_fetch_info(struct upload_pack_data *data)\n+{\n+\tstruct json_writer jw = JSON_WRITER_INIT;\n+\n+\tjw_object_begin(&jw, 0);\n+\t{\n+\t\tjw_object_intmax(&jw, \"haves\", data->haves.nr);\n+\t\tjw_object_intmax(&jw, \"wants\", data->want_obj.nr);\n+\t\tjw_object_intmax(&jw, \"want-refs\", data->wanted_refs.nr);\n+\t\tjw_object_intmax(&jw, \"depth\", data->depth);\n+\t\tjw_object_intmax(&jw, \"shallows\", data->shallows.nr);\n+\t\tjw_object_bool(&jw, \"deepen-since\", data->deepen_since);\n+\t\tjw_object_intmax(&jw, \"deepen-not\", data->deepen_not.nr);\n+\t\tjw_object_bool(&jw, \"deepen-relative\", data->deepen_relative);\n+\t\tif (data->filter_options.choice)\n+\t\t\tjw_object_string(&jw, \"filter\", list_object_filter_config_name(data->filter_options.choice));\n+\t\telse\n+\t\t\tjw_object_null(&jw, \"filter\");\n+\t}\n+\tjw_end(&jw);\n+\n+\ttrace2_data_json(\"upload-pack\", the_repository, \"fetch-info\", &jw);\n+\n+\tjw_release(&jw);\n+}\n+\n static void process_args(struct packet_reader *request,\n \t\t\t struct upload_pack_data *data)\n {\n@@ -1640,6 +1667,8 @@ static void process_args(struct packet_reader *request,\n \n \tif (request->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected flush after fetch arguments\"));\n+\n+\ttrace2_fetch_info(data);\n }\n \n static int process_haves(struct upload_pack_data *data, struct oid_array *common)\n\nbase-commit: aab89be2eb6ca51eefeb8c8066f673f447058856\n-- \ngitgitgadget\n"},{"id":"483089","messageId":"ZScTGyI/XVveu4M7@nand.local","threadId":"60344","inReplyTo":"pull.1598.git.1697040242703.gitgitgadget@gmail.com","subject":"Re: [PATCH] upload-pack: add tracing for fetches","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T21:26:51Z","receivedAt":"2023-10-11T21:27:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 11, 2023 at 04:04:02PM +0000, Robert Coup via GitGitGadget wrote:\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index d18f2823d86..bb15ac34f77 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -132,13 +132,18 @@ test_expect_success 'single branch object count' '\n>  '\n>\n>  test_expect_success 'single given branch clone' '\n> -\tgit clone --single-branch --branch A \"file://$(pwd)/.\" branch-a &&\n> -\ttest_must_fail git --git-dir=branch-a/.git rev-parse origin/B\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/branch-a/trace2_event\" \\\n> +\t\tgit clone --single-branch --branch A \"file://$(pwd)/.\" branch-a &&\n> +\ttest_must_fail git --git-dir=branch-a/.git rev-parse origin/B &&\n> +\tgrep \\\"fetch-info\\\".*\\\"haves\\\":0 branch-a/trace2_event &&\n> +\tgrep \\\"fetch-info\\\".*\\\"wants\\\":1 branch-a/trace2_event\n\nNot at all related to your patch here, but I feel like we have a bunch\nof these greps sprinkled throughout the test suite which serve to\ninspect some key(s) of a JSON object printed to the trace2 event stream.\n\nIt might be nice to have a more robust test-tool that could do this\nheavy lifting for us instead of having to write these grep expressions\nourselves.\n\nBut definitely outside the scope of this patch ;-).\n\nThanks,\nTaylor\n"},{"id":"483091","messageId":"ZScUWrj6CQTg05HL@nand.local","threadId":"60344","inReplyTo":"pull.1598.git.1697040242703.gitgitgadget@gmail.com","subject":"Re: [PATCH] upload-pack: add tracing for fetches","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T21:32:10Z","receivedAt":"2023-10-11T21:32:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 11, 2023 at 04:04:02PM +0000, Robert Coup via GitGitGadget wrote:\n> From: Robert Coup <robert@coup.net.nz>\n>\n> Information on how users are accessing hosted repositories can be\n> helpful to server operators. For example, being able to broadly\n> differentiate between fetches and initial clones; the use of shallow\n> repository features; or partial clone filters.\n\nIndeed. One of the custom patches that GitHub is carrying in its private\nfork is something similar to this that dumps information from\nupload-pack into a custom logging system specific to GitHub (called\n\"sockstat\", in case anybody was curious).\n\nI suspect that we would still live with that patch because we depend on\nsome of the custom logging infrastructure provided by sockstat, but this\nis definitely a good direction to be pursuing for git.git nonetheless.\n\n> @@ -1552,6 +1553,32 @@ static int parse_have(const char *line, struct oid_array *haves)\n>  \treturn 0;\n>  }\n>\n> +static void trace2_fetch_info(struct upload_pack_data *data)\n> +{\n> +\tstruct json_writer jw = JSON_WRITER_INIT;\n> +\n> +\tjw_object_begin(&jw, 0);\n> +\t{\n\nIs there a reason that we have a separate scope here? I think we may\nwant to drop this as unnecessary, but it's entirely possible that I'm\nmissing something here...\n\n> +\t\tjw_object_intmax(&jw, \"haves\", data->haves.nr);\n> +\t\tjw_object_intmax(&jw, \"wants\", data->want_obj.nr);\n> +\t\tjw_object_intmax(&jw, \"want-refs\", data->wanted_refs.nr);\n> +\t\tjw_object_intmax(&jw, \"depth\", data->depth);\n> +\t\tjw_object_intmax(&jw, \"shallows\", data->shallows.nr);\n> +\t\tjw_object_bool(&jw, \"deepen-since\", data->deepen_since);\n> +\t\tjw_object_intmax(&jw, \"deepen-not\", data->deepen_not.nr);\n> +\t\tjw_object_bool(&jw, \"deepen-relative\", data->deepen_relative);\n> +\t\tif (data->filter_options.choice)\n> +\t\t\tjw_object_string(&jw, \"filter\", list_object_filter_config_name(data->filter_options.choice));\n\nI'm pretty sure that list_object_filter_config_name() returns characters\nthat are safe for JSON-encoding, and/or that jw_object_string() does any\nquoting beforehand, but worth checking nonetheless.\n\n> +\t\telse\n> +\t\t\tjw_object_null(&jw, \"filter\");\n\nThese all seem like useful things to have.\n\nThanks,\nTaylor\n"},{"id":"483101","messageId":"20231011222728.GC518221@coredump.intra.peff.net","threadId":"60344","inReplyTo":"pull.1598.git.1697040242703.gitgitgadget@gmail.com","subject":"Re: [PATCH] upload-pack: add tracing for fetches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T22:27:28Z","receivedAt":"2023-10-11T22:27:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2023 at 04:04:02PM +0000, Robert Coup via GitGitGadget wrote:\n\n> Improve this by emitting a Trace2 JSON event from upload-pack with\n> summary information on the contents of a fetch request.\n> \n> * haves, wants, and want-ref counts can help determine (broadly) between\n>   fetches and clones, and the use of single-branch, etc.\n> * shallow clone depth, tip counts, and deepening options.\n> * any partial clone filter type.\n\nI think this is a reasonable thing to have. I see Taylor left a more\ndetailed review, but I did notice one thing...\n\n> +static void trace2_fetch_info(struct upload_pack_data *data)\n> +{\n> +\tstruct json_writer jw = JSON_WRITER_INIT;\n> +\n> +\tjw_object_begin(&jw, 0);\n> +\t{\n> +\t\tjw_object_intmax(&jw, \"haves\", data->haves.nr);\n> +\t\tjw_object_intmax(&jw, \"wants\", data->want_obj.nr);\n> +\t\tjw_object_intmax(&jw, \"want-refs\", data->wanted_refs.nr);\n> +\t\tjw_object_intmax(&jw, \"depth\", data->depth);\n> +\t\tjw_object_intmax(&jw, \"shallows\", data->shallows.nr);\n> +\t\tjw_object_bool(&jw, \"deepen-since\", data->deepen_since);\n> +\t\tjw_object_intmax(&jw, \"deepen-not\", data->deepen_not.nr);\n> +\t\tjw_object_bool(&jw, \"deepen-relative\", data->deepen_relative);\n> +\t\tif (data->filter_options.choice)\n> +\t\t\tjw_object_string(&jw, \"filter\", list_object_filter_config_name(data->filter_options.choice));\n> +\t\telse\n> +\t\t\tjw_object_null(&jw, \"filter\");\n> +\t}\n> +\tjw_end(&jw);\n> +\n> +\ttrace2_data_json(\"upload-pack\", the_repository, \"fetch-info\", &jw);\n> +\n> +\tjw_release(&jw);\n> +}\n\nGenerating the json output isn't entirely trivial (and certainly\ninvolves allocations), but we throw it away unused if tracing isn't\nenabled. Maybe we'd want something like:\n\n  if (!trace2_is_enabled())\n          return;\n\nat the top of the function? It looks like other callers of\njw_object_begin() have a similar issue, and this is probably premature\noptimization to some degree. It just feels like it should be easy for\ntracing to be zero-cost (beyond a single conditional) when it's\ndisabled.\n\n-Peff\n"},{"id":"483102","messageId":"CACf-nVfMLRy6DGbX+G+eJ-LFNEywTmJkpdem8no=TTziLOV+Jw@mail.gmail.com","threadId":"60344","inReplyTo":"ZScUWrj6CQTg05HL@nand.local","subject":"Re: [PATCH] upload-pack: add tracing for fetches","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2023-10-11T22:33:26Z","receivedAt":"2023-10-11T22:33:43Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Taylor,\n\nThanks for taking a look.\n\nOn Wed, 11 Oct 2023 at 22:32, Taylor Blau <me@ttaylorr.com> wrote:\n>\n> One of the custom patches that GitHub is carrying in its private\n> fork is something similar to this that dumps information from\n> upload-pack into a custom logging system specific to GitHub\n\nI suspected as much. My initial goal is clones vs fetches, filter use, and\nshallow use... since I'm changing code is there anything else that you think is\nworthwhile instrumenting? I thought about casting the ints to booleans\n(ie: >0 haves; >0 wants; etc) but I figured the actual numbers might come in\nhandy at some point, and changing them back in future would be incompatible. I\ndeliberately didn't add thin/ofs/etc flags: most of that stuff is on by default\nnow so it felt like noise.\n\n> > + jw_object_begin(&jw, 0);\n> > + {\n>\n> Is there a reason that we have a separate scope here? I think we may\n> want to drop this as unnecessary, but it's entirely possible that I'm\n> missing something here...\n\nI just followed the patterns in test-json-writer.c - personally I think since\nJSON objects/arrays have starts & ends then it's easy to read. But one person's\nreadable is another's coding style violation ;-)\n\n> > + jw_object_string(&jw, \"filter\", list_object_filter_config_name(data->filter_options.choice));\n>\n> I'm pretty sure that list_object_filter_config_name() returns characters\n> that are safe for JSON-encoding, and/or that jw_object_string() does any\n> quoting beforehand, but worth checking nonetheless.\n\nYes, jw_object_string() calls append_quoted_string() which deals with escaping\nand quoting.\n\nThanks,\n\nRob :)\n"},{"id":"483103","messageId":"CACf-nVeNV7NHaFKp976=Ln29CBbCFHcZ3kdQ4AorEcMMZDL4pQ@mail.gmail.com","threadId":"60344","inReplyTo":"20231011222728.GC518221@coredump.intra.peff.net","subject":"Re: [PATCH] upload-pack: add tracing for fetches","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2023-10-11T22:36:07Z","receivedAt":"2023-10-11T22:36:24Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Jeff,\n\nOn Wed, 11 Oct 2023 at 23:27, Jeff King <peff@peff.net> wrote:\n> I think this is a reasonable thing to have.\n\nThanks for taking a look\n\n> Generating the json output isn't entirely trivial (and certainly\n> involves allocations), but we throw it away unused if tracing isn't\n> enabled. Maybe we'd want something like:\n>\n>   if (!trace2_is_enabled())\n>           return;\n>\n> at the top of the function? It looks like other callers of\n> jw_object_begin() have a similar issue, and this is probably premature\n> optimization to some degree. It just feels like it should be easy for\n> tracing to be zero-cost (beyond a single conditional) when it's\n> disabled.\n\nThis definitely makes sense to me. Will add to a v2.\n\nThanks, Rob :)\n"},{"id":"483369","messageId":"pull.1598.v2.git.1697577168128.gitgitgadget@gmail.com","threadId":"60344","inReplyTo":"pull.1598.git.1697040242703.gitgitgadget@gmail.com","subject":"[PATCH v2] upload-pack: add tracing for fetches","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-17T21:12:47Z","receivedAt":"2023-10-17T21:12:52Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup <robert@coup.net.nz>\n\nInformation on how users are accessing hosted repositories can be\nhelpful to server operators. For example, being able to broadly\ndifferentiate between fetches and initial clones; the use of shallow\nrepository features; or partial clone filters.\n\na29263c (fetch-pack: add tracing for negotiation rounds, 2022-08-02)\nadded some information on have counts to fetch-pack itself to help\ndiagnose negotiation; but from a git-upload-pack (server) perspective,\nthere's no means of accessing such information without using\nGIT_TRACE_PACKET to examine the protocol packets.\n\nImprove this by emitting a Trace2 JSON event from upload-pack with\nsummary information on the contents of a fetch request.\n\n* haves, wants, and want-ref counts can help determine (broadly) between\n  fetches and clones, and the use of single-branch, etc.\n* shallow clone depth, tip counts, and deepening options.\n* any partial clone filter type.\n\nSigned-off-by: Robert Coup <robert@coup.net.nz>\n---\n    upload-pack: add tracing for fetches\n    \n    \n    Changes since V1\n    ================\n    \n     * Don't generate the JSON event unless Trace2 is active.\n     * Code style fix.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1598%2Frcoup%2Frc-upload-pack-trace-info-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1598/rcoup/rc-upload-pack-trace-info-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1598\n\nRange-diff vs v1:\n\n 1:  f9157979848 ! 1:  9290d586b08 upload-pack: add tracing for fetches\n     @@ upload-pack.c: static int parse_have(const char *line, struct oid_array *haves)\n      +\tstruct json_writer jw = JSON_WRITER_INIT;\n      +\n      +\tjw_object_begin(&jw, 0);\n     -+\t{\n     -+\t\tjw_object_intmax(&jw, \"haves\", data->haves.nr);\n     -+\t\tjw_object_intmax(&jw, \"wants\", data->want_obj.nr);\n     -+\t\tjw_object_intmax(&jw, \"want-refs\", data->wanted_refs.nr);\n     -+\t\tjw_object_intmax(&jw, \"depth\", data->depth);\n     -+\t\tjw_object_intmax(&jw, \"shallows\", data->shallows.nr);\n     -+\t\tjw_object_bool(&jw, \"deepen-since\", data->deepen_since);\n     -+\t\tjw_object_intmax(&jw, \"deepen-not\", data->deepen_not.nr);\n     -+\t\tjw_object_bool(&jw, \"deepen-relative\", data->deepen_relative);\n     -+\t\tif (data->filter_options.choice)\n     -+\t\t\tjw_object_string(&jw, \"filter\", list_object_filter_config_name(data->filter_options.choice));\n     -+\t\telse\n     -+\t\t\tjw_object_null(&jw, \"filter\");\n     -+\t}\n     ++\tjw_object_intmax(&jw, \"haves\", data->haves.nr);\n     ++\tjw_object_intmax(&jw, \"wants\", data->want_obj.nr);\n     ++\tjw_object_intmax(&jw, \"want-refs\", data->wanted_refs.nr);\n     ++\tjw_object_intmax(&jw, \"depth\", data->depth);\n     ++\tjw_object_intmax(&jw, \"shallows\", data->shallows.nr);\n     ++\tjw_object_bool(&jw, \"deepen-since\", data->deepen_since);\n     ++\tjw_object_intmax(&jw, \"deepen-not\", data->deepen_not.nr);\n     ++\tjw_object_bool(&jw, \"deepen-relative\", data->deepen_relative);\n     ++\tif (data->filter_options.choice)\n     ++\t\tjw_object_string(&jw, \"filter\", list_object_filter_config_name(data->filter_options.choice));\n     ++\telse\n     ++\t\tjw_object_null(&jw, \"filter\");\n      +\tjw_end(&jw);\n      +\n      +\ttrace2_data_json(\"upload-pack\", the_repository, \"fetch-info\", &jw);\n     @@ upload-pack.c: static void process_args(struct packet_reader *request,\n       \tif (request->status != PACKET_READ_FLUSH)\n       \t\tdie(_(\"expected flush after fetch arguments\"));\n      +\n     -+\ttrace2_fetch_info(data);\n     ++\tif (trace2_is_enabled())\n     ++\t\ttrace2_fetch_info(data);\n       }\n       \n       static int process_haves(struct upload_pack_data *data, struct oid_array *common)\n\n\n t/t5500-fetch-pack.sh | 38 +++++++++++++++++++++++++++-----------\n upload-pack.c         | 28 ++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex d18f2823d86..bb15ac34f77 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -132,13 +132,18 @@ test_expect_success 'single branch object count' '\n '\n \n test_expect_success 'single given branch clone' '\n-\tgit clone --single-branch --branch A \"file://$(pwd)/.\" branch-a &&\n-\ttest_must_fail git --git-dir=branch-a/.git rev-parse origin/B\n+\tGIT_TRACE2_EVENT=\"$(pwd)/branch-a/trace2_event\" \\\n+\t\tgit clone --single-branch --branch A \"file://$(pwd)/.\" branch-a &&\n+\ttest_must_fail git --git-dir=branch-a/.git rev-parse origin/B &&\n+\tgrep \\\"fetch-info\\\".*\\\"haves\\\":0 branch-a/trace2_event &&\n+\tgrep \\\"fetch-info\\\".*\\\"wants\\\":1 branch-a/trace2_event\n '\n \n test_expect_success 'clone shallow depth 1' '\n-\tgit clone --no-single-branch --depth 1 \"file://$(pwd)/.\" shallow0 &&\n-\ttest \"$(git --git-dir=shallow0/.git rev-list --count HEAD)\" = 1\n+\tGIT_TRACE2_EVENT=\"$(pwd)/shallow0/trace2_event\" \\\n+\t\tgit clone --no-single-branch --depth 1 \"file://$(pwd)/.\" shallow0 &&\n+\ttest \"$(git --git-dir=shallow0/.git rev-list --count HEAD)\" = 1 &&\n+\tgrep \\\"fetch-info\\\".*\\\"depth\\\":1 shallow0/trace2_event\n '\n \n test_expect_success 'clone shallow depth 1 with fsck' '\n@@ -235,7 +240,10 @@ test_expect_success 'add two more (part 2)' '\n test_expect_success 'deepening pull in shallow repo' '\n \t(\n \t\tcd shallow &&\n-\t\tgit pull --depth 4 .. B\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2_event\" \\\n+\t\t\tgit pull --depth 4 .. B &&\n+\t\tgrep \\\"fetch-info\\\".*\\\"depth\\\":4 trace2_event &&\n+\t\tgrep \\\"fetch-info\\\".*\\\"shallows\\\":2 trace2_event\n \t)\n '\n \n@@ -306,9 +314,12 @@ test_expect_success 'fetch --depth --no-shallow' '\n test_expect_success 'turn shallow to complete repository' '\n \t(\n \t\tcd shallow &&\n-\t\tgit fetch --unshallow &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2_event\" \\\n+\t\t\tgit fetch --unshallow &&\n \t\t! test -f .git/shallow &&\n-\t\tgit fsck --full\n+\t\tgit fsck --full &&\n+\t\tgrep \\\"fetch-info\\\".*\\\"shallows\\\":2 trace2_event &&\n+\t\tgrep \\\"fetch-info\\\".*\\\"depth\\\":2147483647 trace2_event\n \t)\n '\n \n@@ -826,13 +837,15 @@ test_expect_success 'clone shallow since ...' '\n '\n \n test_expect_success 'fetch shallow since ...' '\n-\tgit -C shallow11 fetch --shallow-since \"200000000 +0700\" origin &&\n+\tGIT_TRACE2_EVENT=$(pwd)/shallow11/trace2_event \\\n+\t\tgit -C shallow11 fetch --shallow-since \"200000000 +0700\" origin &&\n \tgit -C shallow11 log --pretty=tformat:%s origin/main >actual &&\n \tcat >expected <<-\\EOF &&\n \tthree\n \ttwo\n \tEOF\n-\ttest_cmp expected actual\n+\ttest_cmp expected actual &&\n+\tgrep \\\"fetch-info\\\".*\\\"deepen-since\\\":true shallow11/trace2_event\n '\n \n test_expect_success 'clone shallow since selects no commits' '\n@@ -987,13 +1000,16 @@ test_expect_success 'filtering by size' '\n \ttest_config -C server uploadpack.allowfilter 1 &&\n \n \ttest_create_repo client &&\n-\tgit -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&\n+\tGIT_TRACE2_EVENT=$(pwd)/client/trace2_event \\\n+\t\tgit -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&\n \n \t# Ensure that object is not inadvertently fetched\n \tcommit=$(git -C server rev-parse HEAD) &&\n \tblob=$(git hash-object server/one.t) &&\n \tgit -C client rev-list --objects --missing=allow-any \"$commit\" >oids &&\n-\t! grep \"$blob\" oids\n+\t! grep \"$blob\" oids &&\n+\n+\tgrep \\\"fetch-info\\\".*\\\"filter\\\":\\\"blob:limit\\\" client/trace2_event\n '\n \n test_expect_success 'filtering by size has no effect if support for it is not advertised' '\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 83f3d2651ab..ea234ab6a45 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -33,6 +33,7 @@\n #include \"commit-reach.h\"\n #include \"shallow.h\"\n #include \"write-or-die.h\"\n+#include \"json-writer.h\"\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -1552,6 +1553,30 @@ static int parse_have(const char *line, struct oid_array *haves)\n \treturn 0;\n }\n \n+static void trace2_fetch_info(struct upload_pack_data *data)\n+{\n+\tstruct json_writer jw = JSON_WRITER_INIT;\n+\n+\tjw_object_begin(&jw, 0);\n+\tjw_object_intmax(&jw, \"haves\", data->haves.nr);\n+\tjw_object_intmax(&jw, \"wants\", data->want_obj.nr);\n+\tjw_object_intmax(&jw, \"want-refs\", data->wanted_refs.nr);\n+\tjw_object_intmax(&jw, \"depth\", data->depth);\n+\tjw_object_intmax(&jw, \"shallows\", data->shallows.nr);\n+\tjw_object_bool(&jw, \"deepen-since\", data->deepen_since);\n+\tjw_object_intmax(&jw, \"deepen-not\", data->deepen_not.nr);\n+\tjw_object_bool(&jw, \"deepen-relative\", data->deepen_relative);\n+\tif (data->filter_options.choice)\n+\t\tjw_object_string(&jw, \"filter\", list_object_filter_config_name(data->filter_options.choice));\n+\telse\n+\t\tjw_object_null(&jw, \"filter\");\n+\tjw_end(&jw);\n+\n+\ttrace2_data_json(\"upload-pack\", the_repository, \"fetch-info\", &jw);\n+\n+\tjw_release(&jw);\n+}\n+\n static void process_args(struct packet_reader *request,\n \t\t\t struct upload_pack_data *data)\n {\n@@ -1640,6 +1665,9 @@ static void process_args(struct packet_reader *request,\n \n \tif (request->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected flush after fetch arguments\"));\n+\n+\tif (trace2_is_enabled())\n+\t\ttrace2_fetch_info(data);\n }\n \n static int process_haves(struct upload_pack_data *data, struct oid_array *common)\n\nbase-commit: aab89be2eb6ca51eefeb8c8066f673f447058856\n-- \ngitgitgadget\n"},{"id":"483696","messageId":"CACf-nVevRU+yxfv44UcV9VcfAwuWLa+4w_OAMAVD5Z5nshqq7g@mail.gmail.com","threadId":"60344","inReplyTo":"pull.1598.v2.git.1697577168128.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] upload-pack: add tracing for fetches","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2023-10-23T18:28:55Z","receivedAt":"2023-10-23T18:29:11Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Jeff & Taylor,\n\nSorry to nag, but would it be possible to give this v2 patch a\nlook-over? Would be good to get it progressed.\n\nThanks,\n\nRob :)\n\n\nOn Tue, 17 Oct 2023 at 22:12, Robert Coup via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>     Changes since V1\n>     ================\n>\n>      * Don't generate the JSON event unless Trace2 is active.\n>      * Code style fix.\n"},{"id":"483704","messageId":"20231023185555.GD1537181@coredump.intra.peff.net","threadId":"60344","inReplyTo":"pull.1598.v2.git.1697577168128.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] upload-pack: add tracing for fetches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-23T18:55:55Z","receivedAt":"2023-10-23T18:55:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 17, 2023 at 09:12:47PM +0000, Robert Coup via GitGitGadget wrote:\n\n> Information on how users are accessing hosted repositories can be\n> helpful to server operators. For example, being able to broadly\n> differentiate between fetches and initial clones; the use of shallow\n> repository features; or partial clone filters.\n> \n> a29263c (fetch-pack: add tracing for negotiation rounds, 2022-08-02)\n> added some information on have counts to fetch-pack itself to help\n> diagnose negotiation; but from a git-upload-pack (server) perspective,\n> there's no means of accessing such information without using\n> GIT_TRACE_PACKET to examine the protocol packets.\n> \n> Improve this by emitting a Trace2 JSON event from upload-pack with\n> summary information on the contents of a fetch request.\n> \n> * haves, wants, and want-ref counts can help determine (broadly) between\n>   fetches and clones, and the use of single-branch, etc.\n> * shallow clone depth, tip counts, and deepening options.\n> * any partial clone filter type.\n> \n> Signed-off-by: Robert Coup <robert@coup.net.nz>\n> ---\n>     upload-pack: add tracing for fetches\n>     \n>     \n>     Changes since V1\n>     ================\n>     \n>      * Don't generate the JSON event unless Trace2 is active.\n>      * Code style fix.\n\nThanks, the first bullet point there addressed my only concern.\n\nThe rest looks good to me overall. I think it is a useful feature to\nhave (as Taylor mentioned, GitHub has something similar via custom\ncode), but it has been long enough since I have operated a server that I\ndon't have opinions on what specific items should or should not be\nincluded. :)\n\n-Peff\n"},{"id":"484150","messageId":"ZUARP/OhnENlIw4C@nand.local","threadId":"60344","inReplyTo":"20231023185555.GD1537181@coredump.intra.peff.net","subject":"Re: [PATCH v2] upload-pack: add tracing for fetches","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-30T20:25:35Z","receivedAt":"2023-10-30T20:25:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 23, 2023 at 02:55:55PM -0400, Jeff King wrote:\n> On Tue, Oct 17, 2023 at 09:12:47PM +0000, Robert Coup via GitGitGadget wrote:\n>\n> > Information on how users are accessing hosted repositories can be\n> > helpful to server operators. For example, being able to broadly\n> > differentiate between fetches and initial clones; the use of shallow\n> > repository features; or partial clone filters.\n> >\n> > a29263c (fetch-pack: add tracing for negotiation rounds, 2022-08-02)\n> > added some information on have counts to fetch-pack itself to help\n> > diagnose negotiation; but from a git-upload-pack (server) perspective,\n> > there's no means of accessing such information without using\n> > GIT_TRACE_PACKET to examine the protocol packets.\n> >\n> > Improve this by emitting a Trace2 JSON event from upload-pack with\n> > summary information on the contents of a fetch request.\n> >\n> > * haves, wants, and want-ref counts can help determine (broadly) between\n> >   fetches and clones, and the use of single-branch, etc.\n> > * shallow clone depth, tip counts, and deepening options.\n> > * any partial clone filter type.\n> >\n> > Signed-off-by: Robert Coup <robert@coup.net.nz>\n> > ---\n> >     upload-pack: add tracing for fetches\n> >\n> >\n> >     Changes since V1\n> >     ================\n> >\n> >      * Don't generate the JSON event unless Trace2 is active.\n> >      * Code style fix.\n>\n> Thanks, the first bullet point there addressed my only concern.\n\nAgreed, this looks great.\n\nThanks,\nTaylor\n"}]}