{"thread":{"id":"52993","subject":"[PATCH] clone: use --quiet when stderr is not a terminal","startedAt":"2020-03-13T21:09:09Z","lastAt":"2020-03-16T00:27:05Z","messageCount":8,"participants":["Derrick Stolee via GitGitGadget","Elijah Newren","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"393235","messageId":"pull.581.git.1584133742475.gitgitgadget@gmail.com","threadId":"52993","inReplyTo":null,"subject":"[PATCH] clone: use --quiet when stderr is not a terminal","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-13T21:09:02Z","receivedAt":"2020-03-13T21:09:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\n\"git clone\" is used by many build systems to download Git code before\nrunning a build. The output of these systems is usually color-coded to\nseparate stdout and stderr output, which highlights anything over stderr\nas an error or warning. Most build systems use \"--quiet\" when cloning to\navoid adding progress noise to these outputs, but occasionally users\ncreate their own scripts that call \"git clone\" and forget the --quiet\noption.\n\nJust such a user voiced a complaint that \"git clone\" was showing \"error\nmessages\" in bright red. The messages were progress indicators for\n\"Updating files\".\n\nTo save users from this confusion, let's default to --quiet when stderr\nis not a terminal window.\n\nTo test that this works, use the GIT_PROGRESS_DELAY environment variable\nto enforce that all progress indicators appear immediately, and check\nthat a redirected stderr has no output. We also need to update some\ntests that inspect stderr after a \"git clone\" or \"git submodule update\"\ncommand. It is easy to update the clone tests with the --verbose option,\nwhile we can remove the clone output from the expected output of the\nsubmodule test.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    clone: use --quiet when stderr is not a terminal\n    \n    I think this is generally how we are intending Git builtins to work.\n    There was a complaint recently about my proposed addition of progress to\n    'git read-tree', but that was because scripts would suddenly get noisy\n    if they were not expecting it. This is the opposite: we will make 'git\n    clone' quieter.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-581%2Fderrickstolee%2Fclone-quiet-default-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-581/derrickstolee/clone-quiet-default-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/581\n\n builtin/clone.c             | 3 +++\n t/t5550-http-fetch-dumb.sh  | 2 +-\n t/t5601-clone.sh            | 7 ++++++-\n t/t7406-submodule-update.sh | 8 --------\n 4 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 1ad26f4d8c8..a2e6905f0ef 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -957,6 +957,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tstruct argv_array ref_prefixes = ARGV_ARRAY_INIT;\n \n+\tif (!isatty(2))\n+\t\toption_verbosity = -1;\n+\n \tpacket_trace_identity(\"clone\");\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n \t\t\t     builtin_clone_usage, 0);\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex b811d89cfd6..c0bdcafa304 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -332,7 +332,7 @@ test_expect_success 'redirects can be forbidden/allowed' '\n \ttest_must_fail git -c http.followRedirects=false \\\n \t\tclone $HTTPD_URL/dumb-redir/repo.git dumb-redir &&\n \tgit -c http.followRedirects=true \\\n-\t\tclone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr\n+\t\tclone --verbose $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr\n '\n \n test_expect_success 'redirects are reported to stderr' '\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 84ea2a3eb70..2902a201977 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -39,7 +39,7 @@ test_expect_success 'clone with excess parameters (2)' '\n \n test_expect_success C_LOCALE_OUTPUT 'output from clone' '\n \trm -fr dst &&\n-\tgit clone -n \"file://$(pwd)/src\" dst >output 2>&1 &&\n+\tgit clone --verbose -n \"file://$(pwd)/src\" dst >output 2>&1 &&\n \ttest $(grep Clon output | wc -l) = 1\n '\n \n@@ -297,6 +297,11 @@ test_expect_success 'clone from original with relative alternate' '\n \tgrep /src/\\\\.git/objects target-10/objects/info/alternates\n '\n \n+test_expect_success 'clone quietly without terminal' '\n+\tGIT_PROGRESS_DELAY=0 git clone src progress 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'clone checking out a tag' '\n \tgit clone --branch=some-tag src dst.tag &&\n \tGIT_DIR=src/.git git rev-parse some-tag >expected &&\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 4fb447a143e..ebf08e3a77a 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -115,18 +115,10 @@ Submodule path '../super/submodule': checked out '$submodulesha1'\n EOF\n \n cat <<EOF >expect2\n-Cloning into '$pwd/recursivesuper/super/merging'...\n-Cloning into '$pwd/recursivesuper/super/none'...\n-Cloning into '$pwd/recursivesuper/super/rebasing'...\n-Cloning into '$pwd/recursivesuper/super/submodule'...\n Submodule 'merging' ($pwd/merging) registered for path '../super/merging'\n Submodule 'none' ($pwd/none) registered for path '../super/none'\n Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'\n Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'\n-done.\n-done.\n-done.\n-done.\n EOF\n \n test_expect_success 'submodule update --init --recursive from subdirectory' '\n\nbase-commit: b4374e96c84ed9394fed363973eb540da308ed4f\n-- \ngitgitgadget\n"},{"id":"393241","messageId":"CABPp-BHKEMp-nA0edS1bgDpTgsh1xuXGgq4pL+9K2ft=He6wQg@mail.gmail.com","threadId":"52993","inReplyTo":"pull.581.git.1584133742475.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: use --quiet when stderr is not a terminal","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-03-14T19:16:59Z","receivedAt":"2020-03-15T01:46:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Mar 13, 2020 at 2:11 PM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> \"git clone\" is used by many build systems to download Git code before\n> running a build. The output of these systems is usually color-coded to\n> separate stdout and stderr output, which highlights anything over stderr\n> as an error or warning. Most build systems use \"--quiet\" when cloning to\n> avoid adding progress noise to these outputs, but occasionally users\n> create their own scripts that call \"git clone\" and forget the --quiet\n> option.\n>\n> Just such a user voiced a complaint that \"git clone\" was showing \"error\n> messages\" in bright red. The messages were progress indicators for\n> \"Updating files\".\n>\n> To save users from this confusion, let's default to --quiet when stderr\n> is not a terminal window.\n>\n> To test that this works, use the GIT_PROGRESS_DELAY environment variable\n> to enforce that all progress indicators appear immediately, and check\n> that a redirected stderr has no output. We also need to update some\n> tests that inspect stderr after a \"git clone\" or \"git submodule update\"\n> command. It is easy to update the clone tests with the --verbose option,\n> while we can remove the clone output from the expected output of the\n> submodule test.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>     clone: use --quiet when stderr is not a terminal\n>\n>     I think this is generally how we are intending Git builtins to work.\n>     There was a complaint recently about my proposed addition of progress to\n>     'git read-tree', but that was because scripts would suddenly get noisy\n>     if they were not expecting it. This is the opposite: we will make 'git\n>     clone' quieter.\n>\n>     Thanks, -Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-581%2Fderrickstolee%2Fclone-quiet-default-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-581/derrickstolee/clone-quiet-default-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/581\n>\n>  builtin/clone.c             | 3 +++\n>  t/t5550-http-fetch-dumb.sh  | 2 +-\n>  t/t5601-clone.sh            | 7 ++++++-\n>  t/t7406-submodule-update.sh | 8 --------\n>  4 files changed, 10 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 1ad26f4d8c8..a2e6905f0ef 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -957,6 +957,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>\n>         struct argv_array ref_prefixes = ARGV_ARRAY_INIT;\n>\n> +       if (!isatty(2))\n> +               option_verbosity = -1;\n> +\n>         packet_trace_identity(\"clone\");\n>         argc = parse_options(argc, argv, prefix, builtin_clone_options,\n>                              builtin_clone_usage, 0);\n> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\n> index b811d89cfd6..c0bdcafa304 100755\n> --- a/t/t5550-http-fetch-dumb.sh\n> +++ b/t/t5550-http-fetch-dumb.sh\n> @@ -332,7 +332,7 @@ test_expect_success 'redirects can be forbidden/allowed' '\n>         test_must_fail git -c http.followRedirects=false \\\n>                 clone $HTTPD_URL/dumb-redir/repo.git dumb-redir &&\n>         git -c http.followRedirects=true \\\n> -               clone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr\n> +               clone --verbose $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr\n>  '\n>\n>  test_expect_success 'redirects are reported to stderr' '\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 84ea2a3eb70..2902a201977 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -39,7 +39,7 @@ test_expect_success 'clone with excess parameters (2)' '\n>\n>  test_expect_success C_LOCALE_OUTPUT 'output from clone' '\n>         rm -fr dst &&\n> -       git clone -n \"file://$(pwd)/src\" dst >output 2>&1 &&\n> +       git clone --verbose -n \"file://$(pwd)/src\" dst >output 2>&1 &&\n>         test $(grep Clon output | wc -l) = 1\n>  '\n>\n> @@ -297,6 +297,11 @@ test_expect_success 'clone from original with relative alternate' '\n>         grep /src/\\\\.git/objects target-10/objects/info/alternates\n>  '\n>\n> +test_expect_success 'clone quietly without terminal' '\n> +       GIT_PROGRESS_DELAY=0 git clone src progress 2>err &&\n> +       test_must_be_empty err\n> +'\n> +\n>  test_expect_success 'clone checking out a tag' '\n>         git clone --branch=some-tag src dst.tag &&\n>         GIT_DIR=src/.git git rev-parse some-tag >expected &&\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index 4fb447a143e..ebf08e3a77a 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -115,18 +115,10 @@ Submodule path '../super/submodule': checked out '$submodulesha1'\n>  EOF\n>\n>  cat <<EOF >expect2\n> -Cloning into '$pwd/recursivesuper/super/merging'...\n> -Cloning into '$pwd/recursivesuper/super/none'...\n> -Cloning into '$pwd/recursivesuper/super/rebasing'...\n> -Cloning into '$pwd/recursivesuper/super/submodule'...\n>  Submodule 'merging' ($pwd/merging) registered for path '../super/merging'\n>  Submodule 'none' ($pwd/none) registered for path '../super/none'\n>  Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'\n>  Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'\n> -done.\n> -done.\n> -done.\n> -done.\n>  EOF\n>\n>  test_expect_success 'submodule update --init --recursive from subdirectory' '\n>\n> base-commit: b4374e96c84ed9394fed363973eb540da308ed4f\n> --\n> gitgitgadget\n\nSeems reasonable to me.\n"},{"id":"393243","messageId":"xmqqh7yqc16w.fsf@gitster.c.googlers.com","threadId":"52993","inReplyTo":"pull.581.git.1584133742475.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: use --quiet when stderr is not a terminal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-14T17:10:15Z","receivedAt":"2020-03-15T01:47:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> \"git clone\" is used by many build systems to download Git code before\n> running a build. The output of these systems is usually color-coded to\n> separate stdout and stderr output, which highlights anything over stderr\n> as an error or warning. Most build systems use \"--quiet\" when cloning to\n> avoid adding progress noise to these outputs, but occasionally users\n> create their own scripts that call \"git clone\" and forget the --quiet\n> option.\n>\n> Just such a user voiced a complaint that \"git clone\" was showing \"error\n> messages\" in bright red. The messages were progress indicators for\n> \"Updating files\".\n>\n> To save users from this confusion, let's default to --quiet when stderr\n> is not a terminal window.\n\nThis is the kind of behaviour change that makes me (and probably\nothers who have been with the project long enough) to say \"it is\ncertain that some other users and tools are relying on the current\nbehaviour and their expectation, when explained, would look just as\nsensible, if not more, than 'any output to the standard error stream\nis an error', which is the justfication given for this change.\"\n\nI would not be surprised if a GUI program is counting the bytes\ncoming to the progress output to show the equivalent with bits on\nthe screen, for example.  They would say \"Git has always given\nprogress output to the standard error stream.  We, as any other\nsensible folks, know that they are not errors and won't give a\nmisleading and alarming messages in red.  We could change our\nprogram to pass --progress but why should we be the one who are\nforced to do such a change\", and I do not have *any* excuse I find\nsensible enough to give them.\n\nI do not mind queuing this (or any similar backward compatibility\nbreaking changes) and merging it down to 'next', but if we were plan\nto have it in a tagged release, I'd prefer to keep it in 'next' for\nat least a few releases before doing so, and under three conditions:\nmajor organizations and those who build tools around Git promise me\nthat they adopt 'next' for their developers and users early, and\nthat they actively measure and report potential damages before it is\nadvanced to 'master', and that they won't let their users complain\nafter it hits a tagged release.\n\nIf the world and userbase were like today back when \"clone\" learned\nthe --quiet option and showing the progress meter 15 years ago, I\nsuspect that we may have chosen this way from the beginning, though.\n"},{"id":"393265","messageId":"a044478f-ca04-3475-90de-0332e80f552d@gmail.com","threadId":"52993","inReplyTo":"xmqqh7yqc16w.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] clone: use --quiet when stderr is not a terminal","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-15T12:20:22Z","receivedAt":"2020-03-15T12:20:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\n\nOn 3/14/2020 1:10 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> \"git clone\" is used by many build systems to download Git code before\n>> running a build. The output of these systems is usually color-coded to\n>> separate stdout and stderr output, which highlights anything over stderr\n>> as an error or warning. Most build systems use \"--quiet\" when cloning to\n>> avoid adding progress noise to these outputs, but occasionally users\n>> create their own scripts that call \"git clone\" and forget the --quiet\n>> option.\n>>\n>> Just such a user voiced a complaint that \"git clone\" was showing \"error\n>> messages\" in bright red. The messages were progress indicators for\n>> \"Updating files\".\n>>\n>> To save users from this confusion, let's default to --quiet when stderr\n>> is not a terminal window.\n> \n> This is the kind of behaviour change that makes me (and probably\n> others who have been with the project long enough) to say \"it is\n> certain that some other users and tools are relying on the current\n> behaviour and their expectation, when explained, would look just as\n> sensible, if not more, than 'any output to the standard error stream\n> is an error', which is the justfication given for this change.\"\n>\n> I would not be surprised if a GUI program is counting the bytes\n> coming to the progress output to show the equivalent with bits on\n> the screen, for example.  They would say \"Git has always given\n> progress output to the standard error stream.  We, as any other\n> sensible folks, know that they are not errors and won't give a\n> misleading and alarming messages in red.  We could change our\n> program to pass --progress but why should we be the one who are\n> forced to do such a change\", and I do not have *any* excuse I find\n> sensible enough to give them.\n\nYou are absolutely right. Tools should do the right thing, and users\ncreating their own scripts should understand the output of the\ncommands they are calling.\n\nAnd I was coming back to this thread shortly after waking up since\nI realized why the test fallout was bigger than I anticipated: this\nchange shouldn't enable \"--quiet\" but instead \"--no-progress\". The\nloss of messages like \"Cloning from ...\" is actually a problematic\nbehavior change.\n\nI'll send a v2 using \"--no-progress\" instead.\n\n> I do not mind queuing this (or any similar backward compatibility\n> breaking changes) and merging it down to 'next', but if we were plan\n> to have it in a tagged release, I'd prefer to keep it in 'next' for\n> at least a few releases before doing so, and under three conditions:\n> major organizations and those who build tools around Git promise me\n> that they adopt 'next' for their developers and users early, and\n> that they actively measure and report potential damages before it is\n> advanced to 'master', and that they won't let their users complain\n> after it hits a tagged release.\n\nI understand that this is low-priority. This idea of adapting \"next\"\nearly is interesting to me. It is likely difficult to deploy that\nbranch to CI/CD systems, but I have been thinking of ways to\nautomate our integration process in microsoft/git (and Scalar and\nVFS for Git) so we find the behavior changes and pain points a lot\nsooner. I'll see what I can do on this front in the coming weeks.\n\n> If the world and userbase were like today back when \"clone\" learned\n> the --quiet option and showing the progress meter 15 years ago, I\n> suspect that we may have chosen this way from the beginning, though.\n\nI appreciate your perspective here. I will not be upset if this\nchange is rejected due to backward compatibility concerns. It's\ntricky to update the user experience around these well-established\nfeatures like \"clone\".\n\nThanks,\n-Stolee\n"},{"id":"393267","messageId":"310a176a-1b30-a479-638e-33a51fb2c896@gmail.com","threadId":"52993","inReplyTo":"a044478f-ca04-3475-90de-0332e80f552d@gmail.com","subject":"[RFC] Universal progress option (was Re: [PATCH] clone: use --quiet when stderr is not a terminal)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-15T13:41:39Z","receivedAt":"2020-03-15T13:41:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/15/2020 8:20 AM, Derrick Stolee wrote:\n> And I was coming back to this thread shortly after waking up since\n> I realized why the test fallout was bigger than I anticipated: this\n> change shouldn't enable \"--quiet\" but instead \"--no-progress\". The\n> loss of messages like \"Cloning from ...\" is actually a problematic\n> behavior change.\n> \n> I'll send a v2 using \"--no-progress\" instead.\n\n...and then I find out that \"git clone --no-progress\" does not stop\nthe \"Updating files\" message because it does not pass the progress\noption to that subsystem.\n\nPlease eject this patch for now, since \"--quiet\" is too aggressive\nand \"--no-progress\" doesn't work.\n\n...pause for change of topic...\n\nAfter seeing too many of these \"let's plumb the user-facing progress\noption from the builtin into the underlying subsystem\" patches, I have\nhalf a mind to completely rework how we handle \"--[no-]progress\".\n\nHere is a proposal for making the progress API easier to use, and\nhopefully clean up our code around it:\n\n1. Add a GIT_PROGRESS environment variable that is a boolean for\n   showing progress or not.\n\n2. Update Git's option-parsing to check for --[no-]progress in\n   every builtin (before the builtins do their own parsing). If\n   present, this overrides GIT_PROGRESS. Otherwise, if GIT_PROGRESS\n   is unset, initialize GIT_PROGRESS to the opposite of isatty(2).\n\n3. Update start_progress_delay() and start_progress() to return\n   NULL if GIT_PROGRESS=0.\n\n4. Refactor the callers of start_progress[_delay]() to call it\n   unconditionally and remove the variables used in the old\n   conditions.\n\nWhile typically adding global state is undesirable, the current\nmechanism of passing progress flags from builtins down to lower\nlayers (and adding --[no-]progress to subcommands) has shown to\nbe difficult to keep consistent and makes the rest of the code\nmessier.\n\nI'm interested in pursuing this refactor, but only if the\ncommunity thinks it is a good idea. There are probably better\nalternatives, too. Please let me know.\n\nThanks,\n-Stolee\n\n   \n"},{"id":"393284","messageId":"xmqqk13l9zmf.fsf@gitster.c.googlers.com","threadId":"52993","inReplyTo":"310a176a-1b30-a479-638e-33a51fb2c896@gmail.com","subject":"Re: [RFC] Universal progress option (was Re: [PATCH] clone: use --quiet when stderr is not a terminal)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-15T19:39:20Z","receivedAt":"2020-03-15T19:39:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> 2. Update Git's option-parsing to check for --[no-]progress in\n>    every builtin (before the builtins do their own parsing).\n\n    git $frotz -e --no-progress other options\n\nNow, without knowing what exactly $frotz is and how it handles its\ncommand line options, you cannot tell if you should unset the global\n\"progress\" variable.  It could be that \"-e\" is an option that takes\none argument (e.g. \"git grep -e\") in which case, you should not\ntouch the global, or that \"-e\" is an explicit request to invoke an\neditor by countermanding anything in the environment or config\n(e.g. \"git merge -e\"), in which case you found \"--no-progress\" that\naffects the global.\n\nIf the parser that kicks in before the commands do their own parsing\nneeds to know that much to correctly understand \"--progress\" anyway,\nwouldn't the same amount of effort would allow us to teach these\nindividual commands to understand \"--progress\" and pass it correctly\ndown to the underlying helpers?\n\nSo, \"git clone --no-progress\" that lets checkout progress may be a\nbug worth fixing, but I do not think a global switch is a good way\nforward.\n\nThanks.\n"},{"id":"393290","messageId":"xmqq5zf59nkb.fsf@gitster.c.googlers.com","threadId":"52993","inReplyTo":"xmqqk13l9zmf.fsf@gitster.c.googlers.com","subject":"Re: [RFC] Universal progress option (was Re: [PATCH] clone: use --quiet when stderr is not a terminal)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-15T23:59:48Z","receivedAt":"2020-03-15T23:59:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> If the parser that kicks in before the commands do their own parsing\n> needs to know that much to correctly understand \"--progress\" anyway,\n> wouldn't the same amount of effort would allow us to teach these\n> individual commands to understand \"--progress\" and pass it correctly\n> down to the underlying helpers?\n>\n> So, \"git clone --no-progress\" that lets checkout progress may be a\n> bug worth fixing, but I do not think a global switch is a good way\n> forward.\n\nYou can sort-of work it around by introducing \"--[no-]progress\" that\nis taken as an option to the \"git\" command, just like \"--[no-]pager\"\nis, to work around the issue above.  But I have a feeling that you\ndid not like the resulting UI, which is totally backward incompatible\nand break users' existing scripts and habits.\n\nThe resulting UI built around \"git --[no-]progress subcmd\" may feel\nmuch nicer, and I suspect that it would be something we would have\npicked, if we had today's experience back when we started adding\nprogress display to individual subcommands.\n\nAs long as a clear transition path can be drawn, I do not\nnecessarily object to such a direction that (1) introduces the\nglobal level \"git --[no-]progress $subcmd\" option, and (2)\ndeprecates and eventually removes the \"--progress\" option at the\nsubcommand level.\n"},{"id":"393292","messageId":"db38c8ef-426b-ecbd-b8b4-de6faf8d3ff1@gmail.com","threadId":"52993","inReplyTo":"xmqq5zf59nkb.fsf@gitster.c.googlers.com","subject":"Re: [RFC] Universal progress option (was Re: [PATCH] clone: use --quiet when stderr is not a terminal)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-16T00:27:00Z","receivedAt":"2020-03-16T00:27:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/15/2020 7:59 PM, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> If the parser that kicks in before the commands do their own parsing\n>> needs to know that much to correctly understand \"--progress\" anyway,\n>> wouldn't the same amount of effort would allow us to teach these\n>> individual commands to understand \"--progress\" and pass it correctly\n>> down to the underlying helpers?\n>>\n>> So, \"git clone --no-progress\" that lets checkout progress may be a\n>> bug worth fixing, but I do not think a global switch is a good way\n>> forward.\n> \n> You can sort-of work it around by introducing \"--[no-]progress\" that\n> is taken as an option to the \"git\" command, just like \"--[no-]pager\"\n> is, to work around the issue above.  But I have a feeling that you\n> did not like the resulting UI, which is totally backward incompatible\n> and break users' existing scripts and habits.\n> \n> The resulting UI built around \"git --[no-]progress subcmd\" may feel\n> much nicer, and I suspect that it would be something we would have\n> picked, if we had today's experience back when we started adding\n> progress display to individual subcommands.\n> \n> As long as a clear transition path can be drawn, I do not\n> necessarily object to such a direction that (1) introduces the\n> global level \"git --[no-]progress $subcmd\" option, and (2)\n> deprecates and eventually removes the \"--progress\" option at the\n> subcommand level.\n\nI think a way to compromise is to create an OPT_PROGRESS() macro\nthat starts as a weak wrapper around the current OPT_BOOL() that\nis used with most builtins manually adding --[no-]progress. Then,\nit can be updated to do a more advanced action upon seeing --progress\nor --no-progress to set GIT_PROGRESS as described.\n\nThis would allow builtins to easily integrate --[no-]progress into\ntheir command-line parsing without issues as you described.\n\nThanks,\n-Stolee\n\n"}]}