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

[Elijah Newren <newren@gmail.com>]

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.