clone: use --quiet when stderr is not a terminal
PATCH 1/1 clone: use --quiet when stderr is not a terminal
From: Derrick Stolee <[email protected]>
"git clone" is used by many build systems to download Git code before running a build. The output of these systems is usually color-coded to separate stdout and stderr output, which highlights anything over stderr as an error or warning. Most build systems use "--quiet" when cloning to avoid adding progress noise to these outputs, but occasionally users create their own scripts that call "git clone" and forget the --quiet option.
Just such a user voiced a complaint that "git clone" was showing "error messages" in bright red. The messages were progress indicators for "Updating files".
To save users from this confusion, let's default to --quiet when stderr is not a terminal window.
To test that this works, use the GIT_PROGRESS_DELAY environment variable to enforce that all progress indicators appear immediately, and check that a redirected stderr has no output. We also need to update some tests that inspect stderr after a "git clone" or "git submodule update" command. It is easy to update the clone tests with the --verbose option, while we can remove the clone output from the expected output of the submodule test.
---
clone: use --quiet when stderr is not a terminal
I think this is generally how we are intending Git builtins to work. There was a complaint recently about my proposed addition of progress to 'git read-tree', but that was because scripts would suddenly get noisy if they were not expecting it. This is the opposite: we will make 'git clone' quieter.
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-581%2Fderrickstolee%2Fclone-quiet-default-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-581/derrickstolee/clone-quiet-default-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/581
builtin/clone.c | 3 +++ t/t5550-http-fetch-dumb.sh | 2 +- t/t5601-clone.sh | 7 ++++++- t/t7406-submodule-update.sh | 8 -------- 4 files changed, 10 insertions(+), 10 deletions(-)
builtin/clone.c +3 -0
@@ -957,6 +957,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)struct argv_array ref_prefixes =ARGV_ARRAY_INIT ; +if (!isatty (2 )) + option_verbosity =-1 ; +packet_trace_identity ("clone" ); argc =parse_options (argc, argv, prefix, builtin_clone_options, builtin_clone_usage,0 );
t/t5550-http-fetch-dumb.sh +1 -1
@@ -332,7 +332,7 @@ test_expect_success 'redirects can be forbidden/allowed' ' test_must_fail git -c http.followRedirects=false \ clone $HTTPD_URL/dumb-redir/repo.git dumb-redir && git -c http.followRedirects=true \ - clone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr + clone --verbose $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr ' test_expect_success 'redirects are reported to stderr' '
t/t5601-clone.sh +6 -1
@@ -39,7 +39,7 @@ test_expect_success 'clone with excess parameters (2)' ' test_expect_success C_LOCALE_OUTPUT 'output from clone' ' rm -fr dst && - git clone -n "file://$(pwd)/src" dst >output 2>&1 && + git clone --verbose -n "file://$(pwd)/src" dst >output 2>&1 && test $(grep Clon output | wc -l) = 1 '
@@ -297,6 +297,11 @@ test_expect_success 'clone from original with relative alternate' ' grep /src/\\.git/objects target-10/objects/info/alternates ' +test_expect_success 'clone quietly without terminal' ' + GIT_PROGRESS_DELAY=0 git clone src progress 2>err && + test_must_be_empty err +' + test_expect_success 'clone checking out a tag' ' git clone --branch=some-tag src dst.tag && GIT_DIR=src/.git git rev-parse some-tag >expected &&
t/t7406-submodule-update.sh +0 -8
@@ -115,18 +115,10 @@ Submodule path '../super/submodule': checked out '$submodulesha1' EOF cat <<EOF >expect2 -Cloning into '$pwd/recursivesuper/super/merging'... -Cloning into '$pwd/recursivesuper/super/none'... -Cloning into '$pwd/recursivesuper/super/rebasing'... -Cloning into '$pwd/recursivesuper/super/submodule'... Submodule 'merging' ($pwd/merging) registered for path '../super/merging' Submodule 'none' ($pwd/none) registered for path '../super/none' Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing' Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule' -done. -done. -done. -done. EOF test_expect_success 'submodule update --init --recursive from subdirectory' '
base-commit: b4374e96c84ed9394fed363973eb540da308ed4f
Signature · 1 line
gitgitgadget
Quoted 124 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
"git clone" is used by many build systems to download Git code before running a build. The output of these systems is usually color-coded to separate stdout and stderr output, which highlights anything over stderr as an error or warning. Most build systems use "--quiet" when cloning to avoid adding progress noise to these outputs, but occasionally users create their own scripts that call "git clone" and forget the --quiet option.
Just such a user voiced a complaint that "git clone" was showing "error messages" in bright red. The messages were progress indicators for "Updating files".
To save users from this confusion, let's default to --quiet when stderr is not a terminal window.
To test that this works, use the
GIT_PROGRESS_DELAYenvironment variable to enforce that all progress indicators appear immediately, and check that a redirected stderr has no output. We also need to update some tests that inspect stderr after a "git clone" or "git submodule update" command. It is easy to update the clone tests with the --verbose option, while we can remove the clone output from the expected output of the submodule test.Signed-off-by Derrick Stolee <[email protected]>---
clone: use --quiet when stderr is not a terminalI think this is generally how we are intending Git builtins to work. There was a complaint recently about my proposed addition of progress to 'git read-tree', but that was because scripts would suddenly get noisy if they were not expecting it. This is the opposite: we will make 'git clone' quieter.
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-581%2Fderrickstolee%2Fclone-quiet-default-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-581/derrickstolee/clone-quiet-default-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/581builtin/clone.c | 3 +++ t/t5550-http-fetch-dumb.sh | 2 +- t/t5601-clone.sh | 7 ++++++- t/t7406-submodule-update.sh | 8 -------- 4 files changed, 10 insertions(+), 10 deletions(-)builtin/clone.c +3
-0@@ -957,6 +957,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)struct argv_array ref_prefixes =ARGV_ARRAY_INIT ; +if (!isatty (2 )) + option_verbosity =-1 ; +packet_trace_identity ("clone" ); argc =parse_options (argc, argv, prefix, builtin_clone_options, builtin_clone_usage,0 );t/t5550-http-fetch-dumb.sh +1
-1@@ -332,7 +332,7 @@ test_expect_success 'redirects can be forbidden/allowed' ' test_must_fail git -c http.followRedirects=false \ clone $HTTPD_URL/dumb-redir/repo.git dumb-redir && git -c http.followRedirects=true \ - clone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr + clone --verbose $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr ' test_expect_success 'redirects are reported to stderr' 't/t5601-clone.sh +6
-1@@ -39,7 +39,7 @@ test_expect_success 'clone with excess parameters (2)' ' test_expect_success C_LOCALE_OUTPUT 'output from clone' ' rm -fr dst && - git clone -n "file://$(pwd)/src" dst >output 2>&1 && + git clone --verbose -n "file://$(pwd)/src" dst >output 2>&1 && test $(grep Clon output | wc -l) = 1 '@@ -297,6 +297,11 @@ test_expect_success 'clone from original with relative alternate' ' grep /src/\\.git/objects target-10/objects/info/alternates ' +test_expect_success 'clone quietly without terminal' ' + GIT_PROGRESS_DELAY=0 git clone src progress 2>err && + test_must_be_empty err +' + test_expect_success 'clone checking out a tag' ' git clone --branch=some-tag src dst.tag && GIT_DIR=src/.git git rev-parse some-tag >expected &&t/t7406-submodule-update.sh +0
-8@@ -115,18 +115,10 @@ Submodule path '../super/submodule': checked out '$submodulesha1' EOF cat <<EOF >expect2 -Cloning into '$pwd/recursivesuper/super/merging'... -Cloning into '$pwd/recursivesuper/super/none'... -Cloning into '$pwd/recursivesuper/super/rebasing'... -Cloning into '$pwd/recursivesuper/super/submodule'... Submodule 'merging' ($pwd/merging) registered for path '../super/merging' Submodule 'none' ($pwd/none) registered for path '../super/none' Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing' Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule' -done. -done. -done. -done. EOF test_expect_success 'submodule update --init --recursive from subdirectory' '
Quoted 3 lines
base-commit:
b4374e96c84ed9394fed363973eb540da308ed4f--
gitgitgadget
Seems reasonable to me.
Quoted 10 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
"git clone" is used by many build systems to download Git code before running a build. The output of these systems is usually color-coded to separate stdout and stderr output, which highlights anything over stderr as an error or warning. Most build systems use "--quiet" when cloning to avoid adding progress noise to these outputs, but occasionally users create their own scripts that call "git clone" and forget the --quiet option.
Quoted 6 lines
Just such a user voiced a complaint that "git clone" was showing "error messages" in bright red. The messages were progress indicators for "Updating files".
To save users from this confusion, let's default to --quiet when stderr is not a terminal window.
This is the kind of behaviour change that makes me (and probably others who have been with the project long enough) to say "it is certain that some other users and tools are relying on the current behaviour and their expectation, when explained, would look just as sensible, if not more, than 'any output to the standard error stream is an error', which is the justfication given for this change."
I would not be surprised if a GUI program is counting the bytes coming to the progress output to show the equivalent with bits on the screen, for example. They would say "Git has always given progress output to the standard error stream. We, as any other sensible folks, know that they are not errors and won't give a misleading and alarming messages in red. We could change our program to pass --progress but why should we be the one who are forced to do such a change", and I do not have *any* excuse I find sensible enough to give them.
I do not mind queuing this (or any similar backward compatibility breaking changes) and merging it down to 'next', but if we were plan to have it in a tagged release, I'd prefer to keep it in 'next' for at least a few releases before doing so, and under three conditions:
major organizations and those who build tools around Git promise me that they adopt 'next' for their developers and users early, and that they actively measure and report potential damages before it is advanced to 'master', and that they won't let their users complain after it hits a tagged release.
If the world and userbase were like today back when "clone" learned the --quiet option and showing the progress meter 15 years ago, I suspect that we may have chosen this way from the beginning, though.
Quoted 26 lines · Junio C Hamano
Quoted 10 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
"git clone" is used by many build systems to download Git code before running a build. The output of these systems is usually color-coded to separate stdout and stderr output, which highlights anything over stderr as an error or warning. Most build systems use "--quiet" when cloning to avoid adding progress noise to these outputs, but occasionally users create their own scripts that call "git clone" and forget the --quiet option.
Quoted 6 lines
Just such a user voiced a complaint that "git clone" was showing "error messages" in bright red. The messages were progress indicators for "Updating files".
To save users from this confusion, let's default to --quiet when stderr is not a terminal window.
This is the kind of behaviour change that makes me (and probably others who have been with the project long enough) to say "it is certain that some other users and tools are relying on the current behaviour and their expectation, when explained, would look just as sensible, if not more, than 'any output to the standard error stream is an error', which is the justfication given for this change."
Quoted 9 lines
I would not be surprised if a GUI program is counting the bytes coming to the progress output to show the equivalent with bits on the screen, for example. They would say "Git has always given progress output to the standard error stream. We, as any other sensible folks, know that they are not errors and won't give a misleading and alarming messages in red. We could change our program to pass --progress but why should we be the one who are forced to do such a change", and I do not have *any* excuse I find sensible enough to give them.
You are absolutely right. Tools should do the right thing, and users creating their own scripts should understand the output of the commands they are calling.
And I was coming back to this thread shortly after waking up since I realized why the test fallout was bigger than I anticipated: this change shouldn't enable "--quiet" but instead "--no-progress". The loss of messages like "Cloning from ..." is actually a problematic behavior change.
I'll send a v2 using "--no-progress" instead.
Quoted 9 lines
I do not mind queuing this (or any similar backward compatibility breaking changes) and merging it down to 'next', but if we were plan to have it in a tagged release, I'd prefer to keep it in 'next' for at least a few releases before doing so, and under three conditions:
major organizations and those who build tools around Git promise me that they adopt 'next' for their developers and users early, and that they actively measure and report potential damages before it is advanced to 'master', and that they won't let their users complain after it hits a tagged release.
I understand that this is low-priority. This idea of adapting "next" early is interesting to me. It is likely difficult to deploy that branch to CI/CD systems, but I have been thinking of ways to automate our integration process in microsoft/git (and Scalar and VFS for Git) so we find the behavior changes and pain points a lot sooner. I'll see what I can do on this front in the coming weeks.
Quoted 3 lines
If the world and userbase were like today back when "clone" learned the --quiet option and showing the progress meter 15 years ago, I suspect that we may have chosen this way from the beginning, though.
I appreciate your perspective here. I will not be upset if this change is rejected due to backward compatibility concerns. It's tricky to update the user experience around these well-established features like "clone".
Thanks,
-Stolee
[RFC] Universal progress option (was Re: [PATCH] clone: use --quiet when stderr is not a terminal)
Quoted 7 lines · Derrick Stolee
And I was coming back to this thread shortly after waking up since I realized why the test fallout was bigger than I anticipated: this change shouldn't enable "--quiet" but instead "--no-progress". The loss of messages like "Cloning from ..." is actually a problematic behavior change.
I'll send a v2 using "--no-progress" instead.
...and then I find out that "git clone --no-progress" does not stop the "Updating files" message because it does not pass the progress option to that subsystem.
Please eject this patch for now, since "--quiet" is too aggressive and "--no-progress" doesn't work.
...pause for change of topic...
After seeing too many of these "let's plumb the user-facing progress option from the builtin into the underlying subsystem" patches, I have half a mind to completely rework how we handle "--[no-]progress".
Here is a proposal for making the progress API easier to use, and hopefully clean up our code around it:
1. Add a GIT_PROGRESS environment variable that is a boolean for showing progress or not.
2. Update Git's option-parsing to check for --[no-]progress in every builtin (before the builtins do their own parsing). If present, this overrides GIT_PROGRESS. Otherwise, if GIT_PROGRESS is unset, initialize GIT_PROGRESS to the opposite of isatty(2).
3. Update start_progress_delay() and start_progress() to return NULL if GIT_PROGRESS=0.
4. Refactor the callers of start_progress[_delay]() to call it unconditionally and remove the variables used in the old conditions.
While typically adding global state is undesirable, the current mechanism of passing progress flags from builtins down to lower layers (and adding --[no-]progress to subcommands) has shown to be difficult to keep consistent and makes the rest of the code messier.
I'm interested in pursuing this refactor, but only if the community thinks it is a good idea. There are probably better alternatives, too. Please let me know.
Thanks,
-Stolee
Re: [RFC] Universal progress option (was Re: [PATCH] clone: use --quiet when stderr is not a terminal)
Quoted 2 lines · Derrick Stolee
2. Update Git's option-parsing to check for --[no-]progress in every builtin (before the builtins do their own parsing).
git $frotz -e --no-progress other options
Now, without knowing what exactly $frotz is and how it handles its command line options, you cannot tell if you should unset the global "progress" variable. It could be that "-e" is an option that takes one argument (e.g. "git grep -e") in which case, you should not touch the global, or that "-e" is an explicit request to invoke an editor by countermanding anything in the environment or config (e.g. "git merge -e"), in which case you found "--no-progress" that affects the global.
If the parser that kicks in before the commands do their own parsing needs to know that much to correctly understand "--progress" anyway, wouldn't the same amount of effort would allow us to teach these individual commands to understand "--progress" and pass it correctly down to the underlying helpers?
So, "git clone --no-progress" that lets checkout progress may be a bug worth fixing, but I do not think a global switch is a good way forward.
Thanks.
Re: [RFC] Universal progress option (was Re: [PATCH] clone: use --quiet when stderr is not a terminal)
Quoted 9 lines · Junio C Hamano
If the parser that kicks in before the commands do their own parsing needs to know that much to correctly understand "--progress" anyway, wouldn't the same amount of effort would allow us to teach these individual commands to understand "--progress" and pass it correctly down to the underlying helpers?
So, "git clone --no-progress" that lets checkout progress may be a bug worth fixing, but I do not think a global switch is a good way forward.
You can sort-of work it around by introducing "--[no-]progress" that is taken as an option to the "git" command, just like "--[no-]pager" is, to work around the issue above. But I have a feeling that you did not like the resulting UI, which is totally backward incompatible and break users' existing scripts and habits.
The resulting UI built around "git --[no-]progress subcmd" may feel much nicer, and I suspect that it would be something we would have picked, if we had today's experience back when we started adding progress display to individual subcommands.
As long as a clear transition path can be drawn, I do not necessarily object to such a direction that (1) introduces the global level "git --[no-]progress $subcmd" option, and (2) deprecates and eventually removes the "--progress" option at the subcommand level.
Re: [RFC] Universal progress option (was Re: [PATCH] clone: use --quiet when stderr is not a terminal)
Quoted 23 lines · Junio C Hamano
Quoted 9 lines · Junio C Hamano
If the parser that kicks in before the commands do their own parsing needs to know that much to correctly understand "--progress" anyway, wouldn't the same amount of effort would allow us to teach these individual commands to understand "--progress" and pass it correctly down to the underlying helpers?
So, "git clone --no-progress" that lets checkout progress may be a bug worth fixing, but I do not think a global switch is a good way forward.
You can sort-of work it around by introducing "--[no-]progress" that is taken as an option to the "git" command, just like "--[no-]pager" is, to work around the issue above. But I have a feeling that you did not like the resulting UI, which is totally backward incompatible and break users' existing scripts and habits.
The resulting UI built around "git --[no-]progress subcmd" may feel much nicer, and I suspect that it would be something we would have picked, if we had today's experience back when we started adding progress display to individual subcommands.
Quoted 5 lines
As long as a clear transition path can be drawn, I do not necessarily object to such a direction that (1) introduces the global level "git --[no-]progress $subcmd" option, and (2) deprecates and eventually removes the "--progress" option at the subcommand level.
I think a way to compromise is to create an OPT_PROGRESS() macro that starts as a weak wrapper around the current OPT_BOOL() that is used with most builtins manually adding --[no-]progress. Then, it can be updated to do a more advanced action upon seeing --progress or --no-progress to set GIT_PROGRESS as described.
This would allow builtins to easily integrate --[no-]progress into their command-line parsing without issues as you described.
Thanks,
-Stolee