patchclone: use --quiet when stderr is not a terminal

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

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

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

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

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

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