[PATCH] clone: use --quiet when stderr is not a terminal

[PATCH] clone: use --quiet when stderr is not a terminal

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

"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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
    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(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 1ad26f4d8c8..a2e6905f0ef 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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);
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index b811d89cfd6..c0bdcafa304 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -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' '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 84ea2a3eb70..2902a201977 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -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 &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 4fb447a143e..ebf08e3a77a 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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
-- 
gitgitgadget

Re: [PATCH] clone: use --quiet when stderr is not a terminal

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> "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.

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.

Re: [PATCH] clone: use --quiet when stderr is not a terminal

[Elijah Newren]

On Fri, Mar 13, 2020 at 2:11 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> "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.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>     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(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 1ad26f4d8c8..a2e6905f0ef 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -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);
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index b811d89cfd6..c0bdcafa304 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -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' '
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 84ea2a3eb70..2902a201977 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -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 &&
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 4fb447a143e..ebf08e3a77a 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -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
> --
> gitgitgadget

Seems reasonable to me.

Re: [PATCH] clone: use --quiet when stderr is not a terminal

[Derrick Stolee]

On 3/14/2020 1:10 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> "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.
> 
> 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.

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.

> 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.

> 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)

[Derrick Stolee]

On 3/15/2020 8:20 AM, Derrick Stolee wrote:
> 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)

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> 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)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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)

[Derrick Stolee]

On 3/15/2020 7:59 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> 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.

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