{"thread":{"id":"61536","subject":"[PATCH] promisor-remote: add promisor.quiet configuration option","startedAt":"2024-05-23T13:20:00Z","lastAt":"2024-05-29T09:36:44Z","messageCount":10,"participants":["Tom Hughes","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"495404","messageId":"20240523131926.1959245-1-tom@compton.nu","threadId":"61536","inReplyTo":null,"subject":"[PATCH] promisor-remote: add promisor.quiet configuration option","fromName":"Tom Hughes","fromEmail":"tom@compton.nu","sentAt":"2024-05-23T13:19:26Z","receivedAt":"2024-05-23T13:20:00Z","isPatch":true,"sender":{"key":"tom@compton.nu","avatar":"https://avatars.githubusercontent.com/u/147741?v=4"},"body":"Add a configuration optione to allow output from the promisor\nfetching objects to be suppressed/\n\nThis allows us to stop commands like git blame being swamped\nwith progress messages and gc notifications from the promisor\nwhen used in a partial clone.\n\nSigned-off-by: Tom Hughes <tom@compton.nu>\n---\n Documentation/config.txt          |  2 ++\n Documentation/config/promisor.txt |  3 ++\n promisor-remote.c                 |  3 ++\n t/t0410-partial-clone.sh          | 47 +++++++++++++++++++++++++++++++\n 4 files changed, 55 insertions(+)\n create mode 100644 Documentation/config/promisor.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 70b448b132..6cae835db9 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -487,6 +487,8 @@ include::config/pager.txt[]\n \n include::config/pretty.txt[]\n \n+include::config/promisor.txt[]\n+\n include::config/protocol.txt[]\n \n include::config/pull.txt[]\ndiff --git a/Documentation/config/promisor.txt b/Documentation/config/promisor.txt\nnew file mode 100644\nindex 0000000000..98c5cb2ec2\n--- /dev/null\n+++ b/Documentation/config/promisor.txt\n@@ -0,0 +1,3 @@\n+promisor.quiet::\n+\tIf set to \"true\" assume `--quiet` when fetching additional\n+\tobjects for a partial clone.\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex b414922c44..2ca7c2ae48 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -23,6 +23,7 @@ static int fetch_objects(struct repository *repo,\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tint i;\n \tFILE *child_in;\n+\tint quiet;\n \n \tif (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {\n \t\tstatic int warning_shown;\n@@ -41,6 +42,8 @@ static int fetch_objects(struct repository *repo,\n \t\t     \"fetch\", remote_name, \"--no-tags\",\n \t\t     \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n \t\t     \"--filter=blob:none\", \"--stdin\", NULL);\n+\tif (!git_config_get_bool(\"promisor.quiet\", &quiet) && quiet)\n+\t\tstrvec_push(&child.args, \"--quiet\");\n \tif (start_command(&child))\n \t\tdie(_(\"promisor-remote: unable to fork off fetch subprocess\"));\n \tchild_in = xfdopen(child.in, \"w\");\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 88a66f0904..99257c3792 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -3,6 +3,7 @@\n test_description='partial clone'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n # missing promisor objects cause repacks which write bitmaps to fail\n GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0\n@@ -689,6 +690,52 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '\n \t! grep \"[?]$FILE_HASH\" out\n '\n \n+test_expect_success TTY 'promisor.quiet=false works' '\n+\trm -rf server server2 repo &&\n+\trm -rf server server3 repo &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server foo &&\n+\tgit -C server repack -a -d --write-bitmap-index &&\n+\n+\tgit clone \"file://$(pwd)/server\" repo &&\n+\tgit hash-object repo/foo.t >blobhash &&\n+\trm -rf repo/.git/objects/* &&\n+\n+\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C server config uploadpack.allowfilter 1 &&\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"origin\" &&\n+\tgit -C repo config promisor.quiet \"false\" &&\n+\n+\ttest_terminal git -C repo cat-file -p $(cat blobhash) 2>err &&\n+\n+\t# Ensure that progress messages are written\n+\tgrep \"Receiving objects\" err\n+'\n+\n+test_expect_success TTY 'promisor.quiet=true works' '\n+\trm -rf server server2 repo &&\n+\trm -rf server server3 repo &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server foo &&\n+\tgit -C server repack -a -d --write-bitmap-index &&\n+\n+\tgit clone \"file://$(pwd)/server\" repo &&\n+\tgit hash-object repo/foo.t >blobhash &&\n+\trm -rf repo/.git/objects/* &&\n+\n+\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C server config uploadpack.allowfilter 1 &&\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"origin\" &&\n+\tgit -C repo config promisor.quiet \"true\" &&\n+\n+\ttest_terminal git -C repo cat-file -p $(cat blobhash) 2>err &&\n+\n+\t# Ensure that no progress messages are written\n+\t! grep \"Receiving objects\" err\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.45.1\n\n"},{"id":"495483","messageId":"xmqqsey8qia7.fsf@gitster.g","threadId":"61536","inReplyTo":"20240523131926.1959245-1-tom@compton.nu","subject":"Re: [PATCH] promisor-remote: add promisor.quiet configuration option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-23T22:23:44Z","receivedAt":"2024-05-23T22:23:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Hughes <tom@compton.nu> writes:\n\n> Add a configuration optione to allow output from the promisor\n> fetching objects to be suppressed/\n\n\"optione\" -> \"option\", \"suppressed/\" -> \"suppressed.\".\n\n> This allows us to stop commands like git blame being swamped\n> with progress messages and gc notifications from the promisor\n> when used in a partial clone.\n\n\"git blame\" -> \"'git blame'\", perhaps.\n\nIt is an interesting observation.  I thought \"git blame\" was quite\nbad at streaming (i.e., until it learned the origin of each and\nevery line, it never produced any output the user asked for), which\nactually would make it a non issue that the output the user wanted\ngets mixed with the progress messages and other garbage.  Unless the\nuser understands that \"git blame\" is not spending time itself, but\nis waiting for necessary blobs to be fetched from the promisor, and\nis expected to wait unusally longer than the fully local case,\nhaving to stare at a blank/unchanging screen would make it uneasy\nfor the end-user and that is why we have progress eye-candy.\n\nI am OK for promisor.quiet being optional, but I am torn when I\nimagine what comes next.  On one hand, I myself probably would find\nit neat to make these lazy fetches happen completely silently as if\nnothing strange is happening from the point of view of end-users\n(except for some operations may be unusually slow compared to fully\nlocal repository).  On the other hand, I suspect people will be\ntempted to push it to be on by default at which time it may hurt\nunsuspecting (new) users who may have been helped by progress bars.\n\n> diff --git a/Documentation/config/promisor.txt b/Documentation/config/promisor.txt\n> new file mode 100644\n> index 0000000000..98c5cb2ec2\n> --- /dev/null\n> +++ b/Documentation/config/promisor.txt\n> @@ -0,0 +1,3 @@\n> +promisor.quiet::\n> +\tIf set to \"true\" assume `--quiet` when fetching additional\n> +\tobjects for a partial clone.\n\nOK.\n\n> diff --git a/promisor-remote.c b/promisor-remote.c\n\nThe implementation is absolutely trivial and straight-forward.\n\n> +test_expect_success TTY 'promisor.quiet=false works' '\n\nDo not say \"works\"---recall the best practice of writing a good bug\nreports.  Stating your expectation more explicitly, e.g. \"shows\nprogress messages\" or somesuch.\n\n> +\trm -rf server server2 repo &&\n> +\trm -rf server server3 repo &&\n\nWhy remove the same thing twice?\n\n> +\ttest_create_repo server &&\n> +\ttest_commit -C server foo &&\n> +\tgit -C server repack -a -d --write-bitmap-index &&\n> +\n> +\tgit clone \"file://$(pwd)/server\" repo &&\n> +\tgit hash-object repo/foo.t >blobhash &&\n\nDo you need a temporary file, or would\n\n\tblobhash=$(git hash-object repo/foo.t) &&\n\nwork just fine?  Of course you'd later have to say\n\n\t... git -C repo cat-file -p $blobhash\n\ninstead of \"$(cat blobhash)\".  Even simpler, I wonder if you can\nremove this hash-object invocation, and then do\n\n\t... git -C repo cat-file -p :foo.t\n\n\n> +\trm -rf repo/.git/objects/* &&\n\nThis! IS! BAD! for the reason stated later ...\n\n\n> +\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n> +\tgit -C server config uploadpack.allowfilter 1 &&\n\n... but these are OK and expected, ...\n\n> +\tgit -C repo config core.repositoryformatversion 1 &&\n> +\tgit -C repo config extensions.partialclone \"origin\" &&\n\n... and this is way too different from what would happen in the real\nlife.\n\nI'd prefer not to see manual destruction of $GIT_DIR/objects/* or\nmanual futzing of repository format version and extensions.  These\nconfiguration variables are *NOT* for end-users to futz with, and\nthe tests should not be doing so either.\n\nCan't we prepare the \"repo\" only by creating a partial clone in an\nusual way?\n\n> +\tgit -C repo config promisor.quiet \"false\" &&\n\nThis of course is good, as this is what the test wants to check.\n\n> +\ttest_terminal git -C repo cat-file -p $(cat blobhash) 2>err &&\n\nIt seems that exactly the same set of comments apply to the next\none, so I'll refrain from repeating myself.\n\nThanks.\n\n> +test_expect_success TTY 'promisor.quiet=true works' '\n> +\trm -rf server server2 repo &&\n> +\trm -rf server server3 repo &&\n> +\ttest_create_repo server &&\n> +\ttest_commit -C server foo &&\n> +\tgit -C server repack -a -d --write-bitmap-index &&\n> +\n> +\tgit clone \"file://$(pwd)/server\" repo &&\n> +\tgit hash-object repo/foo.t >blobhash &&\n> +\trm -rf repo/.git/objects/* &&\n> +\n> +\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n> +\tgit -C server config uploadpack.allowfilter 1 &&\n> +\tgit -C repo config core.repositoryformatversion 1 &&\n> +\tgit -C repo config extensions.partialclone \"origin\" &&\n> +\tgit -C repo config promisor.quiet \"true\" &&\n> +\n> +\ttest_terminal git -C repo cat-file -p $(cat blobhash) 2>err &&\n> +\n> +\t# Ensure that no progress messages are written\n> +\t! grep \"Receiving objects\" err\n> +'\n> +\n>  . \"$TEST_DIRECTORY\"/lib-httpd.sh\n>  start_httpd\n"},{"id":"495523","messageId":"1d39d59e-0d8e-40a0-83d1-6ead6c428bec@compton.nu","threadId":"61536","inReplyTo":"xmqqsey8qia7.fsf@gitster.g","subject":"Re: [PATCH] promisor-remote: add promisor.quiet configuration option","fromName":"Tom Hughes","fromEmail":"tom@compton.nu","sentAt":"2024-05-24T08:31:30Z","receivedAt":"2024-05-24T08:31:45Z","isPatch":true,"sender":{"key":"tom@compton.nu","avatar":"https://avatars.githubusercontent.com/u/147741?v=4"},"body":"On 23/05/2024 23:23, Junio C Hamano wrote:\n\n> It is an interesting observation.  I thought \"git blame\" was quite\n> bad at streaming (i.e., until it learned the origin of each and\n> every line, it never produced any output the user asked for), which\n> actually would make it a non issue that the output the user wanted\n> gets mixed with the progress messages and other garbage.  Unless the\n> user understands that \"git blame\" is not spending time itself, but\n> is waiting for necessary blobs to be fetched from the promisor, and\n> is expected to wait unusally longer than the fully local case,\n> having to stare at a blank/unchanging screen would make it uneasy\n> for the end-user and that is why we have progress eye-candy.\n\nBlame actually has it's own progress message that counts the\nnumber of lines analysed which gets interrupted by the progress\nmessages from the promisor.\n\nSomething like \"git log -S\" behaves a bit differently - it doesn't\nhave progress and because it's using a pager by default that causes\nthe promisor progress to be suppressed because stderr is no longer\na terminal but you do still get lots of background gc notifications.\n\n> I am OK for promisor.quiet being optional, but I am torn when I\n> imagine what comes next.  On one hand, I myself probably would find\n> it neat to make these lazy fetches happen completely silently as if\n> nothing strange is happening from the point of view of end-users\n> (except for some operations may be unusually slow compared to fully\n> local repository).  On the other hand, I suspect people will be\n> tempted to push it to be on by default at which time it may hurt\n> unsuspecting (new) users who may have been helped by progress bars.\n\nI do agree that it's hard to know what the right thing to do is here\nor even to know the full scope of the effect.\n\nI'll update the patch to address the specific review comments.\n\nTom\n\n-- \nTom Hughes (tom@compton.nu)\nhttp://compton.nu/\n\n"},{"id":"495526","messageId":"20240524090937.2448229-1-tom@compton.nu","threadId":"61536","inReplyTo":"20240523131926.1959245-1-tom@compton.nu","subject":"[PATCH v2] promisor-remote: add promisor.quiet configuration option","fromName":"Tom Hughes","fromEmail":"tom@compton.nu","sentAt":"2024-05-24T09:09:37Z","receivedAt":"2024-05-24T09:10:04Z","isPatch":true,"sender":{"key":"tom@compton.nu","avatar":"https://avatars.githubusercontent.com/u/147741?v=4"},"body":"Add a configuration option to allow output from the promisor\nfetching objects to be suppressed.\n\nThis allows us to stop commands like 'git blame' being swamped\nwith progress messages and gc notifications from the promisor\nwhen used in a partial clone.\n\nSigned-off-by: Tom Hughes <tom@compton.nu>\n---\n Documentation/config.txt          |  2 ++\n Documentation/config/promisor.txt |  3 +++\n promisor-remote.c                 |  3 +++\n t/t0410-partial-clone.sh          | 37 +++++++++++++++++++++++++++++++\n 4 files changed, 45 insertions(+)\n create mode 100644 Documentation/config/promisor.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 70b448b132..6cae835db9 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -487,6 +487,8 @@ include::config/pager.txt[]\n \n include::config/pretty.txt[]\n \n+include::config/promisor.txt[]\n+\n include::config/protocol.txt[]\n \n include::config/pull.txt[]\ndiff --git a/Documentation/config/promisor.txt b/Documentation/config/promisor.txt\nnew file mode 100644\nindex 0000000000..98c5cb2ec2\n--- /dev/null\n+++ b/Documentation/config/promisor.txt\n@@ -0,0 +1,3 @@\n+promisor.quiet::\n+\tIf set to \"true\" assume `--quiet` when fetching additional\n+\tobjects for a partial clone.\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex b414922c44..2ca7c2ae48 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -23,6 +23,7 @@ static int fetch_objects(struct repository *repo,\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tint i;\n \tFILE *child_in;\n+\tint quiet;\n \n \tif (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {\n \t\tstatic int warning_shown;\n@@ -41,6 +42,8 @@ static int fetch_objects(struct repository *repo,\n \t\t     \"fetch\", remote_name, \"--no-tags\",\n \t\t     \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n \t\t     \"--filter=blob:none\", \"--stdin\", NULL);\n+\tif (!git_config_get_bool(\"promisor.quiet\", &quiet) && quiet)\n+\t\tstrvec_push(&child.args, \"--quiet\");\n \tif (start_command(&child))\n \t\tdie(_(\"promisor-remote: unable to fork off fetch subprocess\"));\n \tchild_in = xfdopen(child.in, \"w\");\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 88a66f0904..2957160efa 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -3,6 +3,7 @@\n test_description='partial clone'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n # missing promisor objects cause repacks which write bitmaps to fail\n GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0\n@@ -689,6 +690,42 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '\n \t! grep \"[?]$FILE_HASH\" out\n '\n \n+test_expect_success TTY 'promisor.quiet=false shows progress messages' '\n+\trm -rf server repo &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server foo &&\n+\tgit -C server rm foo.t &&\n+\tgit -C server commit -m remove &&\n+\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C server config uploadpack.allowfilter 1 &&\n+\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" repo &&\n+\tgit -C repo config promisor.quiet \"false\" &&\n+\n+\ttest_terminal git -C repo cat-file -p foo:foo.t 2>err &&\n+\n+\t# Ensure that progress messages are written\n+\tgrep \"Receiving objects\" err\n+'\n+\n+test_expect_success TTY 'promisor.quiet=true does not show progress messages' '\n+\trm -rf server repo &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server foo &&\n+\tgit -C server rm foo.t &&\n+\tgit -C server commit -m remove &&\n+\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C server config uploadpack.allowfilter 1 &&\n+\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" repo &&\n+\tgit -C repo config promisor.quiet \"true\" &&\n+\n+\ttest_terminal git -C repo cat-file -p foo:foo.t 2>err &&\n+\n+\t# Ensure that no progress messages are written\n+\t! grep \"Receiving objects\" err\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.45.1\n\n"},{"id":"495583","messageId":"xmqqa5kfkrsu.fsf@gitster.g","threadId":"61536","inReplyTo":"20240524090937.2448229-1-tom@compton.nu","subject":"Re: [PATCH v2] promisor-remote: add promisor.quiet configuration option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T18:06:57Z","receivedAt":"2024-05-24T18:07:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Hughes <tom@compton.nu> writes:\n\n> +test_expect_success TTY 'promisor.quiet=false shows progress messages' '\n> +\trm -rf server repo &&\n> +\ttest_create_repo server &&\n> +\ttest_commit -C server foo &&\n> +\tgit -C server rm foo.t &&\n> +\tgit -C server commit -m remove &&\n> +\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n> +\tgit -C server config uploadpack.allowfilter 1 &&\n> +\n> +\tgit clone --filter=blob:none \"file://$(pwd)/server\" repo &&\n> +\tgit -C repo config promisor.quiet \"false\" &&\n> +\n> +\ttest_terminal git -C repo cat-file -p foo:foo.t 2>err &&\n> +\n> +\t# Ensure that progress messages are written\n> +\tgrep \"Receiving objects\" err\n> +'\n> +\n> +test_expect_success TTY 'promisor.quiet=true does not show progress messages' '\n> +\trm -rf server repo &&\n> +\ttest_create_repo server &&\n> +\ttest_commit -C server foo &&\n> +\tgit -C server rm foo.t &&\n> +\tgit -C server commit -m remove &&\n> +\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n> +\tgit -C server config uploadpack.allowfilter 1 &&\n> +\n> +\tgit clone --filter=blob:none \"file://$(pwd)/server\" repo &&\n> +\tgit -C repo config promisor.quiet \"true\" &&\n> +\n> +\ttest_terminal git -C repo cat-file -p foo:foo.t 2>err &&\n> +\n> +\t# Ensure that no progress messages are written\n> +\t! grep \"Receiving objects\" err\n> +'\n\nWouldn't we want to see this as three (or four) tests,\n\n (1) \"setup\" that prepares the server end\n\n (2) \"quiet=false\" test that\n     - makes a partial clone, \n     - sets .quiet to false, \n     - runs cat-file -p,\n     - makes sure that the lazy fetching is chatty.\n\n (3) \"quiet=true\" test, which is the same as (2) except that it sets\n     .quiet to true and expects the lazy fetching to be silent.\n\n (4) \"quiet=unconfigured\" test, which is the same as (2) except that\n     it leaves .quiet unconfigured.\n\nOther than that, looking very much nicer than the previous round\nthat manually mucked with internal implementation on the client end.\n\nThanks.\n\n"},{"id":"495627","messageId":"20240525052946.GD1895047@coredump.intra.peff.net","threadId":"61536","inReplyTo":"20240523131926.1959245-1-tom@compton.nu","subject":"Re: [PATCH] promisor-remote: add promisor.quiet configuration option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-25T05:29:46Z","receivedAt":"2024-05-25T05:29:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 23, 2024 at 02:19:26PM +0100, Tom Hughes wrote:\n\n> Add a configuration optione to allow output from the promisor\n> fetching objects to be suppressed/\n> \n> This allows us to stop commands like git blame being swamped\n> with progress messages and gc notifications from the promisor\n> when used in a partial clone.\n\nI'm not at all opposed to providing a way to suppress this, but I feel\nlike in the long run, the more fundamental issue is that git-blame kicks\noff a zillion fetches as it traverses. That's not only ugly but it's\nalso horribly inefficient.\n\nIn an ideal world we'd queue all of the blobs we need, do a single\nfetch, and then compute the blame on the result. That's probably easier\nsaid than done, though we have done it in other spots (e.g., for\ncheckout).\n\nIn terms of user experience, you can simulate it with something like:\n\n  # fault in all of the necessary blobs in one batch\n  git rev-list HEAD -- $file |\n  git diff-tree --stdin --format= -r --diff-filter=d -m --raw -- $file |\n  awk '{print $4}' |\n  git -c fetch.negotiationAlgorithm=noop \\\n      fetch --no-tags --no-write-fetch-head --recurse-submodules=no \\\n      --filter=blob:none --stdin\n\n  git blame $file\n\nObviously that command is horrid and not something users should have to\ncare about. But if we had some way for blame to say \"hey, I am\ntraversing from X..Y, looking at these pathspecs\", then our first\nlazy-fetch could try to grab all of them. And I think the same would be\nthe case for \"git log -p\", and so on.\n\nDoing a separate traversal isn't maximally efficient, but it might not\nbe too bad in practice (and we could even do partial traversals to\nbalance chunking versus responsiveness, though in the case of\nnon-incremental blame we need everything before we generate an\nanswer anyway).\n\nBut anyway, I bring it up here because I think once we reach that end\nstate, it won't be as interesting to turn off the fetch progress.\n\n-Peff\n"},{"id":"495631","messageId":"20240525100927.2949808-1-tom@compton.nu","threadId":"61536","inReplyTo":"20240524090937.2448229-1-tom@compton.nu","subject":"[PATCH v3] promisor-remote: add promisor.quiet configuration option","fromName":"Tom Hughes","fromEmail":"tom@compton.nu","sentAt":"2024-05-25T10:09:27Z","receivedAt":"2024-05-25T10:09:44Z","isPatch":true,"sender":{"key":"tom@compton.nu","avatar":"https://avatars.githubusercontent.com/u/147741?v=4"},"body":"Add a configuration option to allow output from the promisor\nfetching objects to be suppressed.\n\nThis allows us to stop commands like 'git blame' being swamped\nwith progress messages and gc notifications from the promisor\nwhen used in a partial clone.\n\nSigned-off-by: Tom Hughes <tom@compton.nu>\n---\n Documentation/config.txt          |  2 ++\n Documentation/config/promisor.txt |  3 +++\n promisor-remote.c                 |  3 +++\n t/t0410-partial-clone.sh          | 43 +++++++++++++++++++++++++++++++\n 4 files changed, 51 insertions(+)\n create mode 100644 Documentation/config/promisor.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 70b448b132..6cae835db9 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -487,6 +487,8 @@ include::config/pager.txt[]\n \n include::config/pretty.txt[]\n \n+include::config/promisor.txt[]\n+\n include::config/protocol.txt[]\n \n include::config/pull.txt[]\ndiff --git a/Documentation/config/promisor.txt b/Documentation/config/promisor.txt\nnew file mode 100644\nindex 0000000000..98c5cb2ec2\n--- /dev/null\n+++ b/Documentation/config/promisor.txt\n@@ -0,0 +1,3 @@\n+promisor.quiet::\n+\tIf set to \"true\" assume `--quiet` when fetching additional\n+\tobjects for a partial clone.\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex b414922c44..2ca7c2ae48 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -23,6 +23,7 @@ static int fetch_objects(struct repository *repo,\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tint i;\n \tFILE *child_in;\n+\tint quiet;\n \n \tif (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {\n \t\tstatic int warning_shown;\n@@ -41,6 +42,8 @@ static int fetch_objects(struct repository *repo,\n \t\t     \"fetch\", remote_name, \"--no-tags\",\n \t\t     \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n \t\t     \"--filter=blob:none\", \"--stdin\", NULL);\n+\tif (!git_config_get_bool(\"promisor.quiet\", &quiet) && quiet)\n+\t\tstrvec_push(&child.args, \"--quiet\");\n \tif (start_command(&child))\n \t\tdie(_(\"promisor-remote: unable to fork off fetch subprocess\"));\n \tchild_in = xfdopen(child.in, \"w\");\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 88a66f0904..8d468eb170 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -3,6 +3,7 @@\n test_description='partial clone'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n # missing promisor objects cause repacks which write bitmaps to fail\n GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0\n@@ -689,6 +690,48 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '\n \t! grep \"[?]$FILE_HASH\" out\n '\n \n+test_expect_success 'setup for promisor.quiet tests' '\n+\trm -rf server &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server foo &&\n+\tgit -C server rm foo.t &&\n+\tgit -C server commit -m remove &&\n+\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C server config uploadpack.allowfilter 1\n+'\n+\n+test_expect_success TTY 'promisor.quiet=false shows progress messages' '\n+\trm -rf repo &&\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" repo &&\n+\tgit -C repo config promisor.quiet \"false\" &&\n+\n+\ttest_terminal git -C repo cat-file -p foo:foo.t 2>err &&\n+\n+\t# Ensure that progress messages are written\n+\tgrep \"Receiving objects\" err\n+'\n+\n+test_expect_success TTY 'promisor.quiet=true does not show progress messages' '\n+\trm -rf repo &&\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" repo &&\n+\tgit -C repo config promisor.quiet \"true\" &&\n+\n+\ttest_terminal git -C repo cat-file -p foo:foo.t 2>err &&\n+\n+\t# Ensure that no progress messages are written\n+\t! grep \"Receiving objects\" err\n+'\n+\n+test_expect_success TTY 'promisor.quiet=unconfigured shows progress messages' '\n+\trm -rf repo &&\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" repo &&\n+\n+\ttest_terminal git -C repo cat-file -p foo:foo.t 2>err &&\n+\n+\t# Ensure that progress messages are written\n+\tgrep \"Receiving objects\" err\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.45.1\n\n"},{"id":"495632","messageId":"ab69eeda-65d2-4e38-9abc-fb1230fe8ab6@compton.nu","threadId":"61536","inReplyTo":"xmqqa5kfkrsu.fsf@gitster.g","subject":"Re: [PATCH v2] promisor-remote: add promisor.quiet configuration option","fromName":"Tom Hughes","fromEmail":"tom@compton.nu","sentAt":"2024-05-25T10:10:24Z","receivedAt":"2024-05-25T10:10:30Z","isPatch":true,"sender":{"key":"tom@compton.nu","avatar":"https://avatars.githubusercontent.com/u/147741?v=4"},"body":"On 24/05/2024 19:06, Junio C Hamano wrote:\n\n> Wouldn't we want to see this as three (or four) tests,\n> \n>   (1) \"setup\" that prepares the server end\n> \n>   (2) \"quiet=false\" test that\n>       - makes a partial clone,\n>       - sets .quiet to false,\n>       - runs cat-file -p,\n>       - makes sure that the lazy fetching is chatty.\n> \n>   (3) \"quiet=true\" test, which is the same as (2) except that it sets\n>       .quiet to true and expects the lazy fetching to be silent.\n> \n>   (4) \"quiet=unconfigured\" test, which is the same as (2) except that\n>       it leaves .quiet unconfigured.\n\nSounds good to me. I've sent a v3 with those changes.\n\nTom\n\n-- \nTom Hughes (tom@compton.nu)\nhttp://compton.nu/\n\n"},{"id":"495633","messageId":"1b10454e-c5ad-4ce3-a724-27306ee8824c@compton.nu","threadId":"61536","inReplyTo":"20240525052946.GD1895047@coredump.intra.peff.net","subject":"Re: [PATCH] promisor-remote: add promisor.quiet configuration option","fromName":"Tom Hughes","fromEmail":"tom@compton.nu","sentAt":"2024-05-25T10:29:13Z","receivedAt":"2024-05-25T10:29:19Z","isPatch":true,"sender":{"key":"tom@compton.nu","avatar":"https://avatars.githubusercontent.com/u/147741?v=4"},"body":"On 25/05/2024 06:29, Jeff King wrote:\n> On Thu, May 23, 2024 at 02:19:26PM +0100, Tom Hughes wrote:\n> \n>> Add a configuration optione to allow output from the promisor\n>> fetching objects to be suppressed/\n>>\n>> This allows us to stop commands like git blame being swamped\n>> with progress messages and gc notifications from the promisor\n>> when used in a partial clone.\n> \n> I'm not at all opposed to providing a way to suppress this, but I feel\n> like in the long run, the more fundamental issue is that git-blame kicks\n> off a zillion fetches as it traverses. That's not only ugly but it's\n> also horribly inefficient.\n\nThis is true. One thing I found that makes things a lot more\nefficient if you're using ssh as the transport is to enable\npersistent multiplexing in .ssh/config with something like:\n\nHost git.example.com\n   ControlMaster auto\n   ControlPath /run/user/%i/ssh/control.%C\n   ControlPersist 1m\n   SendEnv GIT_PROTOCOL\n\nwhich avoids each fetch having to setup and authenticate a\nnew ssh session.\n\n> In an ideal world we'd queue all of the blobs we need, do a single\n> fetch, and then compute the blame on the result. That's probably easier\n> said than done, though we have done it in other spots (e.g., for\n> checkout).\n\nThat would certainly be an excellent improvement, yes.\n\nTom\n\n-- \nTom Hughes (tom@compton.nu)\nhttp://compton.nu/\n\n"},{"id":"495808","messageId":"20240529093643.GE1098944@coredump.intra.peff.net","threadId":"61536","inReplyTo":"1b10454e-c5ad-4ce3-a724-27306ee8824c@compton.nu","subject":"Re: [PATCH] promisor-remote: add promisor.quiet configuration option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-29T09:36:43Z","receivedAt":"2024-05-29T09:36:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 25, 2024 at 11:29:13AM +0100, Tom Hughes wrote:\n\n> > I'm not at all opposed to providing a way to suppress this, but I feel\n> > like in the long run, the more fundamental issue is that git-blame kicks\n> > off a zillion fetches as it traverses. That's not only ugly but it's\n> > also horribly inefficient.\n> \n> This is true. One thing I found that makes things a lot more\n> efficient if you're using ssh as the transport is to enable\n> persistent multiplexing in .ssh/config with something like:\n> \n> Host git.example.com\n>   ControlMaster auto\n>   ControlPath /run/user/%i/ssh/control.%C\n>   ControlPersist 1m\n>   SendEnv GIT_PROTOCOL\n> \n> which avoids each fetch having to setup and authenticate a\n> new ssh session.\n\nGood point. That is sort of the opposite approach of my suggestion. That\nis, I was suggesting that git-blame batch everything to make a single\nefficient request. But if we could reduce the cost of making individual\nrequests, then we wouldn't need to batch (which is quite a lot simpler).\n\nThe ssh session is going to be one source of latency and overhead. But\njust spawning the fetch and remote upload-pack are another (especially\nif you have to authenticate, and especially with the v2 protocol, which\nhas an extra round-trip for capabilities upgrade).\n\nIf there was a long-running mode to git-fetch where it kept open a v2\nsession to the server and just said \"hey, send me object X\" and then\n\"OK, now send me object Y\" that would eliminate all of that overhead\n(and even for http, under the hood curl is good at keeping the session\nopen between requests).\n\nYou'd still have some extra latency (while you're talking to the server,\nthe local blame process is paused), but I suspect it would be a lot more\ntolerable.\n\nAnd now your progress question is re-opened again. You might want a\nmore succinct progress for something like blame that still does all of\nits fetching before generating output. E.g., you might want a single\nprogress line with the current state (fetching or not), the count of\nfetched objects, the speed, and so on. And for something like \"git log\n-p\", where the progress would be interspersed with actual output, you\nmight want to suppress it entirely.\n\nSo yeah, I have no real objection to what your patch is doing. Depending\non how future work unfolds it might be more or less useful than it is\nnow, but even in the worst case it probably won't be a bad thing to have\nin our toolbox.\n\n-Peff\n"}]}