[PATCH] scalar: use verbose mode in clone

[PATCH] scalar: use verbose mode in clone

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

Sometimes when users use scalar to download a monorepo
with a long commit history, they want to check the
progress bar to know how long they still need to wait
during the fetch process, but scalar suppresses this
output by default.

So add `[--verbose| -v]` to scalar clone, to enable
fetch's output.

Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    scalar: use verbose mode in clone
    
    When users use scalar to download a monorepo with a long commit history
    (or the client and server network communication is very poor), we often
    need to spend a long time in the fetch phase of scalar, some users may
    want to check this progress bar To understand the progress of fetch and
    how long they have to wait, so we should enable scalar to display fetch
    progress.
    
    v1. add [--verbose| -v] to scalar clone.
    
    Note: output look like this:
    
    $ scalar clone -v git@github.com:git/git.git
    Initialized empty Git repository in /Users/adl/test/git/src/.git/
    remote: Enumerating objects: 209091, done.
    remote: Counting objects: 100% (991/991), done.
    remote: Compressing objects: 100% (944/944), done.
    Receiving objects: 100% (209085/209085), 81.39 MiB | 126.00 KiB/s, done.
    remote: Total 209085 (delta 54), reused 979 (delta 47), pack-reused 208094
    Resolving deltas: 100% (134000/134000), done.
    From github.com:git/git
     * [new branch]          jch         -> origin/jch
     * [new branch]          main        -> origin/main
     * [new branch]          maint       -> origin/maint
     * [new branch]          master      -> origin/master
     * [new branch]          next        -> origin/next
     * [new branch]          seen        -> origin/seen
     * [new branch]          todo        -> origin/todo
     * [new tag]             v2.39.0-rc2 -> v2.39.0-rc2
     * [new tag]             gitgui-0.10.0    -> gitgui-0.10.0
     * [new tag]             gitgui-0.10.1    -> gitgui-0.10.1
     * [new tag]             gitgui-0.10.2    -> gitgui-0.10.2
     * [new tag]             gitgui-0.11.0    -> gitgui-0.11.0
     ...
    
    
    "new branch", "new tag" output is a bit annoying, it would be better to
    suppress them, but keep the progress.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1441%2Fadlternative%2Fzh%2Fscalar-verbosity-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1441/adlternative/zh/scalar-verbosity-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1441

 Documentation/scalar.txt |  7 ++++++-
 scalar.c                 | 11 ++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/scalar.txt b/Documentation/scalar.txt
index f33436c7f65..7ff37b43945 100644
--- a/Documentation/scalar.txt
+++ b/Documentation/scalar.txt
@@ -8,7 +8,7 @@ scalar - A tool for managing large Git repositories
 SYNOPSIS
 --------
 [verse]
-scalar clone [--single-branch] [--branch <main-branch>] [--full-clone] <url> [<enlistment>]
+scalar clone [--single-branch] [--branch <main-branch>] [--verbose | -v] [--full-clone] <url> [<enlistment>]
 scalar list
 scalar register [<enlistment>]
 scalar unregister [<enlistment>]
@@ -84,6 +84,11 @@ cloning. If the HEAD at the remote did not point at any branch when
 	A sparse-checkout is initialized by default. This behavior can be
 	turned off via `--full-clone`.
 
+-v::
+--verbose::
+	When scalar executes `git fetch`, `--quiet` is used by default to
+	suppress the output of fetch, use verbose mode for cancel this.
+
 List
 ~~~~
 
diff --git a/scalar.c b/scalar.c
index 6c52243cdf1..b1d4504d136 100644
--- a/scalar.c
+++ b/scalar.c
@@ -404,7 +404,7 @@ void load_builtin_commands(const char *prefix, struct cmdnames *cmds)
 static int cmd_clone(int argc, const char **argv)
 {
 	const char *branch = NULL;
-	int full_clone = 0, single_branch = 0;
+	int full_clone = 0, single_branch = 0, verbosity = 0;
 	struct option clone_options[] = {
 		OPT_STRING('b', "branch", &branch, N_("<branch>"),
 			   N_("branch to checkout after clone")),
@@ -413,6 +413,7 @@ static int cmd_clone(int argc, const char **argv)
 		OPT_BOOL(0, "single-branch", &single_branch,
 			 N_("only download metadata for the branch that will "
 			    "be checked out")),
+		OPT__VERBOSITY(&verbosity),
 		OPT_END(),
 	};
 	const char * const clone_usage[] = {
@@ -499,7 +500,9 @@ static int cmd_clone(int argc, const char **argv)
 	if (set_recommended_config(0))
 		return error(_("could not configure '%s'"), dir);
 
-	if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
+	if ((res = run_git("fetch", "origin",
+			   verbosity ? NULL : "--quiet",
+			   NULL))) {
 		warning(_("partial clone failed; attempting full clone"));
 
 		if (set_config("remote.origin.promisor") ||
@@ -508,7 +511,9 @@ static int cmd_clone(int argc, const char **argv)
 			goto cleanup;
 		}
 
-		if ((res = run_git("fetch", "--quiet", "origin", NULL)))
+		if ((res = run_git("fetch", "origin",
+				   verbosity ? NULL : "--quiet",
+				   NULL)))
 			goto cleanup;
 	}
 

base-commit: 2e71cbbddd64695d43383c25c7a054ac4ff86882
-- 
gitgitgadget

Re: [PATCH] scalar: use verbose mode in clone

[Taylor Blau]

On Wed, Dec 07, 2022 at 06:10:56PM +0000, ZheNing Hu via GitGitGadget wrote:
> So add `[--verbose| -v]` to scalar clone, to enable
> fetch's output.

Seems reasonable.

> @@ -84,6 +84,11 @@ cloning. If the HEAD at the remote did not point at any branch when
>  	A sparse-checkout is initialized by default. This behavior can be
>  	turned off via `--full-clone`.
>
> +-v::
> +--verbose::
> +	When scalar executes `git fetch`, `--quiet` is used by default to
> +	suppress the output of fetch, use verbose mode for cancel this.
> +

This description may be exposing a few too many implementation details
for our liking. E.g., scalar happens to use `git fetch`, but it might
not always. That is probably academic, but a more practical reason to do
some hiding here might just be that it's unnecessary detail to expose in
our documentation.

Perhaps something like:

    -v::
    --verbose::
     Enable more verbose output when cloning a repository.

Or something simple like that.

>  List
>  ~~~~
>
> diff --git a/scalar.c b/scalar.c
> index 6c52243cdf1..b1d4504d136 100644
> --- a/scalar.c
> +++ b/scalar.c
> @@ -404,7 +404,7 @@ void load_builtin_commands(const char *prefix, struct cmdnames *cmds)
>  static int cmd_clone(int argc, const char **argv)
>  {
>  	const char *branch = NULL;
> -	int full_clone = 0, single_branch = 0;
> +	int full_clone = 0, single_branch = 0, verbosity = 0;
>  	struct option clone_options[] = {
>  		OPT_STRING('b', "branch", &branch, N_("<branch>"),
>  			   N_("branch to checkout after clone")),
> @@ -413,6 +413,7 @@ static int cmd_clone(int argc, const char **argv)
>  		OPT_BOOL(0, "single-branch", &single_branch,
>  			 N_("only download metadata for the branch that will "
>  			    "be checked out")),
> +		OPT__VERBOSITY(&verbosity),
>  		OPT_END(),
>  	};
>  	const char * const clone_usage[] = {

Looking good.

> @@ -499,7 +500,9 @@ static int cmd_clone(int argc, const char **argv)
>  	if (set_recommended_config(0))
>  		return error(_("could not configure '%s'"), dir);
>
> -	if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
> +	if ((res = run_git("fetch", "origin",
> +			   verbosity ? NULL : "--quiet",
> +			   NULL))) {

Hmmph. This and below are a little strange in that they will end up
calling:

    run_git("fetch", "origin", NULL, NULL);

when running without `--verbose`. `run_git()` will still do the right
thing and stop reading its arguments after the first NULL that it sees.
So I doubt that it's a huge deal in practice, but felt worth calling out
nonetheless.

Is there an opportunity to easily test this new code?

Thanks,
Taylor

Re: [PATCH] scalar: use verbose mode in clone

[ZheNing Hu]

Taylor Blau <me@ttaylorr.com> 于2022年12月8日周四 06:10写道：
>
> On Wed, Dec 07, 2022 at 06:10:56PM +0000, ZheNing Hu via GitGitGadget wrote:
> > So add `[--verbose| -v]` to scalar clone, to enable
> > fetch's output.
>
> Seems reasonable.
>
> > @@ -84,6 +84,11 @@ cloning. If the HEAD at the remote did not point at any branch when
> >       A sparse-checkout is initialized by default. This behavior can be
> >       turned off via `--full-clone`.
> >
> > +-v::
> > +--verbose::
> > +     When scalar executes `git fetch`, `--quiet` is used by default to
> > +     suppress the output of fetch, use verbose mode for cancel this.
> > +
>
> This description may be exposing a few too many implementation details
> for our liking. E.g., scalar happens to use `git fetch`, but it might
> not always. That is probably academic, but a more practical reason to do
> some hiding here might just be that it's unnecessary detail to expose in
> our documentation.
>

Hmmm. There are two steps to downloading data from scalar clone:
the first step is to let "git fetch partial clone"  to  download commits,
trees, tags, and the second step is download the blobs corresponding
to the top-level files of the repository during git checkout. So I'm not sure
if I should mention "fetch" here, since the progress bar for the "checkout"
step is able to be displayed.

> Perhaps something like:
>
>     -v::
>     --verbose::
>      Enable more verbose output when cloning a repository.
>

Just mentioning "clone" is fine... But I'm not sure if users will be
confused, why they will "more verbose" instead of two options
"full verbose" or "not verbose".

> Or something simple like that.
>
> >  List
> >  ~~~~
> >
> > diff --git a/scalar.c b/scalar.c
> > index 6c52243cdf1..b1d4504d136 100644
> > --- a/scalar.c
> > +++ b/scalar.c
> > @@ -404,7 +404,7 @@ void load_builtin_commands(const char *prefix, struct cmdnames *cmds)
> >  static int cmd_clone(int argc, const char **argv)
> >  {
> >       const char *branch = NULL;
> > -     int full_clone = 0, single_branch = 0;
> > +     int full_clone = 0, single_branch = 0, verbosity = 0;
> >       struct option clone_options[] = {
> >               OPT_STRING('b', "branch", &branch, N_("<branch>"),
> >                          N_("branch to checkout after clone")),
> > @@ -413,6 +413,7 @@ static int cmd_clone(int argc, const char **argv)
> >               OPT_BOOL(0, "single-branch", &single_branch,
> >                        N_("only download metadata for the branch that will "
> >                           "be checked out")),
> > +             OPT__VERBOSITY(&verbosity),
> >               OPT_END(),
> >       };
> >       const char * const clone_usage[] = {
>
> Looking good.
>
> > @@ -499,7 +500,9 @@ static int cmd_clone(int argc, const char **argv)
> >       if (set_recommended_config(0))
> >               return error(_("could not configure '%s'"), dir);
> >
> > -     if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
> > +     if ((res = run_git("fetch", "origin",
> > +                        verbosity ? NULL : "--quiet",
> > +                        NULL))) {
>
> Hmmph. This and below are a little strange in that they will end up
> calling:
>
>     run_git("fetch", "origin", NULL, NULL);
>
> when running without `--verbose`. `run_git()` will still do the right
> thing and stop reading its arguments after the first NULL that it sees.
> So I doubt that it's a huge deal in practice, but felt worth calling out
> nonetheless.
>

The reason I'm doing this is seeing that toggle_maintenance() already
does this, and it's not buggy, but it's really inelegant.

My personal understanding is that the original intention of run_git()
is to help developers simply put git parameters into the variable parameters
of the function, and run_git() has no good way to understand null values.
Here we put it in run_git () The last is an act of desperation.

> Is there an opportunity to easily test this new code?
>

It's a bit cumbersome, but I will try.

> Thanks,
> Taylor

Thanks,
ZheNing Hu

Re: [PATCH] scalar: use verbose mode in clone

[Derrick Stolee]

On 12/7/2022 1:10 PM, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>
> 
> Sometimes when users use scalar to download a monorepo
> with a long commit history, they want to check the
> progress bar to know how long they still need to wait
> during the fetch process, but scalar suppresses this
> output by default.

I think this is an accurate description of the status quo.
 
> So add `[--verbose| -v]` to scalar clone, to enable
> fetch's output.

However, this isn't the only thing we could consider doing.

For instance, we typically use isatty(2) to detect if
stderr is a terminal to determine if we should carry
through progress indicators. It seems that maybe run_git()
is not passing through stderr and thus diminishing the
progress indicators to the fetch subprocess. It's worth
looking into to see if there's a different approach that
would get the same goal without needing a new option. It
could also make your proposed '--verbose' to be implied
by isatty(2).

If being verbose becomes the implied default with isatty(2),
then it might be better to add a --quiet option instead, to
opt-out of the progress.

Also, I'm not sure your implementation is doing the right
thing.

> -	if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
> +	if ((res = run_git("fetch", "origin",
> +			   verbosity ? NULL : "--quiet",
> +			   NULL))) {
>  		warning(_("partial clone failed; attempting full clone"));
>  
>  		if (set_config("remote.origin.promisor") ||
> @@ -508,7 +511,9 @@ static int cmd_clone(int argc, const char **argv)
>  			goto cleanup;
>  		}
>  
> -		if ((res = run_git("fetch", "--quiet", "origin", NULL)))
> +		if ((res = run_git("fetch", "origin",
> +				   verbosity ? NULL : "--quiet",
> +				   NULL)))

Specifically, here the "verbosity" being on does not change
the way we are calling 'git fetch', so I do not expect the
behavior to change with this calling pattern.

You might want to add the "--progress" option in the verbose
case.

As Taylor mentioned, a test might be helpful. Here's an
example from t7700-repack.sh that sets up the isatty(2)
configuration correctly, as well as sets the progress
delay to 0 to be sure some progress indicators are written:

test_expect_success TTY '--quiet disables progress' '
	test_terminal env GIT_PROGRESS_DELAY=0 \
		git -C midx repack -ad --quiet --write-midx 2>stderr &&
	test_must_be_empty stderr
'

Thanks,
-Stolee

Re: [PATCH] scalar: use verbose mode in clone

[ZheNing Hu]

Hi,

Derrick Stolee <derrickstolee@github.com> 于2022年12月9日周五 00:30写道：
>
> On 12/7/2022 1:10 PM, ZheNing Hu via GitGitGadget wrote:
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Sometimes when users use scalar to download a monorepo
> > with a long commit history, they want to check the
> > progress bar to know how long they still need to wait
> > during the fetch process, but scalar suppresses this
> > output by default.
>
> I think this is an accurate description of the status quo.
>
> > So add `[--verbose| -v]` to scalar clone, to enable
> > fetch's output.
>
> However, this isn't the only thing we could consider doing.
>
> For instance, we typically use isatty(2) to detect if
> stderr is a terminal to determine if we should carry
> through progress indicators. It seems that maybe run_git()
> is not passing through stderr and thus diminishing the
> progress indicators to the fetch subprocess. It's worth
> looking into to see if there's a different approach that
> would get the same goal without needing a new option. It
> could also make your proposed '--verbose' to be implied
> by isatty(2).
>
> If being verbose becomes the implied default with isatty(2),
> then it might be better to add a --quiet option instead, to
> opt-out of the progress.
>

Good point that we should care about atty.

I guess you mean is to add a parameter to run_git(), which can
control if git commands show stderr/stdout... This solution
may be better. Because git checkout should also have the
same behavior as git fetch: quiet or verbose.

> Also, I'm not sure your implementation is doing the right
> thing.
>
> > -     if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
> > +     if ((res = run_git("fetch", "origin",
> > +                        verbosity ? NULL : "--quiet",
> > +                        NULL))) {
> >               warning(_("partial clone failed; attempting full clone"));
> >
> >               if (set_config("remote.origin.promisor") ||
> > @@ -508,7 +511,9 @@ static int cmd_clone(int argc, const char **argv)
> >                       goto cleanup;
> >               }
> >
> > -             if ((res = run_git("fetch", "--quiet", "origin", NULL)))
> > +             if ((res = run_git("fetch", "origin",
> > +                                verbosity ? NULL : "--quiet",
> > +                                NULL)))
>
> Specifically, here the "verbosity" being on does not change
> the way we are calling 'git fetch', so I do not expect the
> behavior to change with this calling pattern.
>

Sorry, but I don't understand: I deleted "--quiet", and the progress bar can
also be displayed. Why do you say "not change the way we are
calling 'git fetch'"？

> You might want to add the "--progress" option in the verbose
> case.
>

Good advice.

> As Taylor mentioned, a test might be helpful. Here's an
> example from t7700-repack.sh that sets up the isatty(2)
> configuration correctly, as well as sets the progress
> delay to 0 to be sure some progress indicators are written:
>
> test_expect_success TTY '--quiet disables progress' '
>         test_terminal env GIT_PROGRESS_DELAY=0 \
>                 git -C midx repack -ad --quiet --write-midx 2>stderr &&
>         test_must_be_empty stderr
> '
>

Thanks for the reminder, I will pay attention to this "TTY" and
"GIT_PROGRESS_DELAY" when I write tests later.

> Thanks,
> -Stolee

Thanks,
-ZheNing Hu

[PATCH v2] scalar: show progress if stderr refer to a terminal

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

Sometimes when users use scalar to download a monorepo
with a long commit history, they want to check the
progress bar to know how long they still need to wait
during the fetch process, but scalar suppresses this
output by default.

So let's check whether scalar stderr refer to a terminal,
if so, show progress, otherwise disable it.

Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    scalar: show progress if stderr refer to a terminal
    
    When users use scalar to download a monorepo with a long commit history
    (or the client and server network communication is very poor), we often
    need to spend a long time in the fetch phase of scalar, some users may
    want to check this progress bar To understand the progress of fetch and
    how long they have to wait, so we should enable scalar to display fetch
    progress.
    
    v1. add [--verbose| -v] to scalar clone.
    
    v2.
    
     1. remove --verbose option.
     2. check if scalar stderr refer to terminal, if so, show progress.
    
    Note: output look like this:
    
    $ scalar clone git@github.com:git/git.git
    Initialized empty Git repository in /home/adl/test/git/src/.git/
    remote: Enumerating objects: 208997, done.
    remote: Counting objects: 100% (870/870), done.
    remote: Compressing objects: 100% (870/870), done.
    remote: Total 208991 (delta 0), reused 870 (delta 0), pack-reused 208121
    remote: Enumerating objects: 470, done.
    remote: Counting objects: 100% (418/418), done.
    remote: Compressing objects: 100% (418/418), done.
    remote: Total 470 (delta 1), reused 0 (delta 0), pack-reused 52
    Receiving objects: 100% (470/470), 1.96 MiB | 1.64 MiB/s, done.
    Resolving deltas: 100% (1/1), done.
    Updating files: 100% (471/471), done.
    branch 'master' set up to track 'origin/master'.
    Switched to a new branch 'master'
    Your branch is up to date with 'origin/master'.
    
    
    "new branch", "new tag" output is a bit annoying, it would be better to
    suppress them, but keep the progress.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1441%2Fadlternative%2Fzh%2Fscalar-verbosity-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1441/adlternative/zh/scalar-verbosity-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1441

Range-diff vs v1:

 1:  6169841190a ! 1:  2e4c296bd19 scalar: use verbose mode in clone
     @@ Metadata
      Author: ZheNing Hu <adlternative@gmail.com>
      
       ## Commit message ##
     -    scalar: use verbose mode in clone
     +    scalar: show progress if stderr refer to a terminal
      
          Sometimes when users use scalar to download a monorepo
          with a long commit history, they want to check the
     @@ Commit message
          during the fetch process, but scalar suppresses this
          output by default.
      
     -    So add `[--verbose| -v]` to scalar clone, to enable
     -    fetch's output.
     +    So let's check whether scalar stderr refer to a terminal,
     +    if so, show progress, otherwise disable it.
      
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
      
     - ## Documentation/scalar.txt ##
     -@@ Documentation/scalar.txt: scalar - A tool for managing large Git repositories
     - SYNOPSIS
     - --------
     - [verse]
     --scalar clone [--single-branch] [--branch <main-branch>] [--full-clone] <url> [<enlistment>]
     -+scalar clone [--single-branch] [--branch <main-branch>] [--verbose | -v] [--full-clone] <url> [<enlistment>]
     - scalar list
     - scalar register [<enlistment>]
     - scalar unregister [<enlistment>]
     -@@ Documentation/scalar.txt: cloning. If the HEAD at the remote did not point at any branch when
     - 	A sparse-checkout is initialized by default. This behavior can be
     - 	turned off via `--full-clone`.
     - 
     -+-v::
     -+--verbose::
     -+	When scalar executes `git fetch`, `--quiet` is used by default to
     -+	suppress the output of fetch, use verbose mode for cancel this.
     -+
     - List
     - ~~~~
     - 
     -
       ## scalar.c ##
      @@ scalar.c: void load_builtin_commands(const char *prefix, struct cmdnames *cmds)
       static int cmd_clone(int argc, const char **argv)
       {
       	const char *branch = NULL;
      -	int full_clone = 0, single_branch = 0;
     -+	int full_clone = 0, single_branch = 0, verbosity = 0;
     ++	int full_clone = 0, single_branch = 0, show_progress = isatty(2);
       	struct option clone_options[] = {
       		OPT_STRING('b', "branch", &branch, N_("<branch>"),
       			   N_("branch to checkout after clone")),
     -@@ scalar.c: static int cmd_clone(int argc, const char **argv)
     - 		OPT_BOOL(0, "single-branch", &single_branch,
     - 			 N_("only download metadata for the branch that will "
     - 			    "be checked out")),
     -+		OPT__VERBOSITY(&verbosity),
     - 		OPT_END(),
     - 	};
     - 	const char * const clone_usage[] = {
      @@ scalar.c: static int cmd_clone(int argc, const char **argv)
       	if (set_recommended_config(0))
       		return error(_("could not configure '%s'"), dir);
       
      -	if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
     -+	if ((res = run_git("fetch", "origin",
     -+			   verbosity ? NULL : "--quiet",
     -+			   NULL))) {
     ++	if ((res = run_git("fetch", "--quiet",
     ++				show_progress ? "--progress" : "--no-progress",
     ++				"origin", NULL))) {
       		warning(_("partial clone failed; attempting full clone"));
       
       		if (set_config("remote.origin.promisor") ||
     @@ scalar.c: static int cmd_clone(int argc, const char **argv)
       		}
       
      -		if ((res = run_git("fetch", "--quiet", "origin", NULL)))
     -+		if ((res = run_git("fetch", "origin",
     -+				   verbosity ? NULL : "--quiet",
     -+				   NULL)))
     ++		if ((res = run_git("fetch", "--quiet",
     ++					show_progress ? "--progress" : "--no-progress",
     ++					"origin", NULL)))
       			goto cleanup;
       	}
       
     +
     + ## t/t9211-scalar-clone.sh ##
     +@@
     + test_description='test the `scalar clone` subcommand'
     + 
     + . ./test-lib.sh
     ++. "${TEST_DIRECTORY}/lib-terminal.sh"
     + 
     + GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true"
     + export GIT_TEST_MAINT_SCHEDULER
     +@@ t/t9211-scalar-clone.sh: test_expect_success '--no-single-branch clones all branches' '
     + 	cleanup_clone $enlistment
     + '
     + 
     ++test_expect_success TTY 'progress with tty' '
     ++	enlistment=progress1 &&
     ++
     ++	test_config -C to-clone uploadpack.allowfilter true &&
     ++	test_config -C to-clone uploadpack.allowanysha1inwant true &&
     ++
     ++	test_terminal env GIT_PROGRESS_DELAY=0 \
     ++		scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
     ++	grep --count "Enumerating objects" stderr >actual &&
     ++	echo 2 >expected &&
     ++	test_cmp expected actual &&
     ++	cleanup_clone $enlistment
     ++'
     ++
     ++test_expect_success 'progress without tty' '
     ++	enlistment=progress2 &&
     ++
     ++	test_config -C to-clone uploadpack.allowfilter true &&
     ++	test_config -C to-clone uploadpack.allowanysha1inwant true &&
     ++
     ++	scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
     ++	! grep "Enumerating objects" stderr &&
     ++	! grep "Updating files" stderr &&
     ++	cleanup_clone $enlistment
     ++'
     + test_done


 scalar.c                | 10 +++++++---
 t/t9211-scalar-clone.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/scalar.c b/scalar.c
index 6c52243cdf1..e5cc554c537 100644
--- a/scalar.c
+++ b/scalar.c
@@ -404,7 +404,7 @@ void load_builtin_commands(const char *prefix, struct cmdnames *cmds)
 static int cmd_clone(int argc, const char **argv)
 {
 	const char *branch = NULL;
-	int full_clone = 0, single_branch = 0;
+	int full_clone = 0, single_branch = 0, show_progress = isatty(2);
 	struct option clone_options[] = {
 		OPT_STRING('b', "branch", &branch, N_("<branch>"),
 			   N_("branch to checkout after clone")),
@@ -499,7 +499,9 @@ static int cmd_clone(int argc, const char **argv)
 	if (set_recommended_config(0))
 		return error(_("could not configure '%s'"), dir);
 
-	if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
+	if ((res = run_git("fetch", "--quiet",
+				show_progress ? "--progress" : "--no-progress",
+				"origin", NULL))) {
 		warning(_("partial clone failed; attempting full clone"));
 
 		if (set_config("remote.origin.promisor") ||
@@ -508,7 +510,9 @@ static int cmd_clone(int argc, const char **argv)
 			goto cleanup;
 		}
 
-		if ((res = run_git("fetch", "--quiet", "origin", NULL)))
+		if ((res = run_git("fetch", "--quiet",
+					show_progress ? "--progress" : "--no-progress",
+					"origin", NULL)))
 			goto cleanup;
 	}
 
diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
index dd33d87e9be..49f054d5917 100755
--- a/t/t9211-scalar-clone.sh
+++ b/t/t9211-scalar-clone.sh
@@ -3,6 +3,7 @@
 test_description='test the `scalar clone` subcommand'
 
 . ./test-lib.sh
+. "${TEST_DIRECTORY}/lib-terminal.sh"
 
 GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true"
 export GIT_TEST_MAINT_SCHEDULER
@@ -148,4 +149,29 @@ test_expect_success '--no-single-branch clones all branches' '
 	cleanup_clone $enlistment
 '
 
+test_expect_success TTY 'progress with tty' '
+	enlistment=progress1 &&
+
+	test_config -C to-clone uploadpack.allowfilter true &&
+	test_config -C to-clone uploadpack.allowanysha1inwant true &&
+
+	test_terminal env GIT_PROGRESS_DELAY=0 \
+		scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
+	grep --count "Enumerating objects" stderr >actual &&
+	echo 2 >expected &&
+	test_cmp expected actual &&
+	cleanup_clone $enlistment
+'
+
+test_expect_success 'progress without tty' '
+	enlistment=progress2 &&
+
+	test_config -C to-clone uploadpack.allowfilter true &&
+	test_config -C to-clone uploadpack.allowanysha1inwant true &&
+
+	scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
+	! grep "Enumerating objects" stderr &&
+	! grep "Updating files" stderr &&
+	cleanup_clone $enlistment
+'
 test_done

base-commit: 2e71cbbddd64695d43383c25c7a054ac4ff86882
-- 
gitgitgadget

Re: [PATCH v2] scalar: show progress if stderr refer to a terminal

[Derrick Stolee]

On 12/25/22 8:29 AM, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>

Sorry for the long wait in getting back to reviewing.

> Sometimes when users use scalar to download a monorepo
> with a long commit history, they want to check the
> progress bar to know how long they still need to wait
> during the fetch process, but scalar suppresses this
> output by default.
> 
> So let's check whether scalar stderr refer to a terminal,
> if so, show progress, otherwise disable it.

Thanks for updating to this strategy. I think it's an
easier change to swallow. We can consider options like
--progress, --verbose, or --quiet later while this
change does the good work of showing terminal users
helpful progress.

> +	int full_clone = 0, single_branch = 0, show_progress = isatty(2);

> -	if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
> +	if ((res = run_git("fetch", "--quiet",
> +				show_progress ? "--progress" : "--no-progress",
> +				"origin", NULL))) {
>  		warning(_("partial clone failed; attempting full clone"));
>  
>  		if (set_config("remote.origin.promisor") ||
> @@ -508,7 +510,9 @@ static int cmd_clone(int argc, const char **argv)
>  			goto cleanup;
>  		}
>  
> -		if ((res = run_git("fetch", "--quiet", "origin", NULL)))
> +		if ((res = run_git("fetch", "--quiet",
> +					show_progress ? "--progress" : "--no-progress",
> +					"origin", NULL)))
Implementation looks correct.

> +test_expect_success TTY 'progress with tty' '
> +	enlistment=progress1 &&
> +
> +	test_config -C to-clone uploadpack.allowfilter true &&
> +	test_config -C to-clone uploadpack.allowanysha1inwant true &&
> +
> +	test_terminal env GIT_PROGRESS_DELAY=0 \
> +		scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&

Thank you for creating this test!

> +	grep --count "Enumerating objects" stderr >actual &&
> +	echo 2 >expected &&
> +	test_cmp expected actual &&

I think you could use "test_line_count = 2 actual" here.

> +	cleanup_clone $enlistment
> +'
> +
> +test_expect_success 'progress without tty' '
> +	enlistment=progress2 &&
> +
> +	test_config -C to-clone uploadpack.allowfilter true &&
> +	test_config -C to-clone uploadpack.allowanysha1inwant true &&
> +
> +	scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
> +	! grep "Enumerating objects" stderr &&
> +	! grep "Updating files" stderr &&

Here, it would be good to still have the GIT_PROGRESS_DELAY=0
environment variable on the 'scalar clone' command to be sure
we are not getting these lines because progress is turned off
and not because it's running too quickly.

> +	cleanup_clone $enlistment
> +'
>  test_done

A nit: there should be an empty line between the end quote of
the last test and "test_done".

Thanks,
-Stolee

Re: [PATCH v2] scalar: show progress if stderr refer to a terminal

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

>> +test_expect_success 'progress without tty' '
>> +	enlistment=progress2 &&
>> +
>> +	test_config -C to-clone uploadpack.allowfilter true &&
>> +	test_config -C to-clone uploadpack.allowanysha1inwant true &&
>> +
>> +	scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
>> +	! grep "Enumerating objects" stderr &&
>> +	! grep "Updating files" stderr &&
>
> Here, it would be good to still have the GIT_PROGRESS_DELAY=0
> environment variable on the 'scalar clone' command to be sure
> we are not getting these lines because progress is turned off
> and not because it's running too quickly.

Good point.

Re: [PATCH v2] scalar: show progress if stderr refer to a terminal

[ZheNing Hu]

Derrick Stolee <derrickstolee@github.com> 于2023年1月6日周五 03:19写道：
>
> On 12/25/22 8:29 AM, ZheNing Hu via GitGitGadget wrote:
> > From: ZheNing Hu <adlternative@gmail.com>
>
> Sorry for the long wait in getting back to reviewing.
>

Ah, It's okay.

> > Sometimes when users use scalar to download a monorepo
> > with a long commit history, they want to check the
> > progress bar to know how long they still need to wait
> > during the fetch process, but scalar suppresses this
> > output by default.
> >
> > So let's check whether scalar stderr refer to a terminal,
> > if so, show progress, otherwise disable it.
>
> Thanks for updating to this strategy. I think it's an
> easier change to swallow. We can consider options like
> --progress, --verbose, or --quiet later while this
> change does the good work of showing terminal users
> helpful progress.
>

Yes, but I think something like `--quiet` is difficult to implement.
We cannot just add `--no-progress` or `--quiet` to the git checkout
for suppressing the progress. Because git checkout will not use it
to suppress the fetch progress, but only the checkout's progress.

> > +     int full_clone = 0, single_branch = 0, show_progress = isatty(2);
>
> > -     if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
> > +     if ((res = run_git("fetch", "--quiet",
> > +                             show_progress ? "--progress" : "--no-progress",
> > +                             "origin", NULL))) {
> >               warning(_("partial clone failed; attempting full clone"));
> >
> >               if (set_config("remote.origin.promisor") ||
> > @@ -508,7 +510,9 @@ static int cmd_clone(int argc, const char **argv)
> >                       goto cleanup;
> >               }
> >
> > -             if ((res = run_git("fetch", "--quiet", "origin", NULL)))
> > +             if ((res = run_git("fetch", "--quiet",
> > +                                     show_progress ? "--progress" : "--no-progress",
> > +                                     "origin", NULL)))
> Implementation looks correct.
>
> > +test_expect_success TTY 'progress with tty' '
> > +     enlistment=progress1 &&
> > +
> > +     test_config -C to-clone uploadpack.allowfilter true &&
> > +     test_config -C to-clone uploadpack.allowanysha1inwant true &&
> > +
> > +     test_terminal env GIT_PROGRESS_DELAY=0 \
> > +             scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
>
> Thank you for creating this test!
>
> > +     grep --count "Enumerating objects" stderr >actual &&
> > +     echo 2 >expected &&
> > +     test_cmp expected actual &&
>
> I think you could use "test_line_count = 2 actual" here.
>

Oh, good suggestion. I should also use grep without `--count` too.

> > +     cleanup_clone $enlistment
> > +'
> > +
> > +test_expect_success 'progress without tty' '
> > +     enlistment=progress2 &&
> > +
> > +     test_config -C to-clone uploadpack.allowfilter true &&
> > +     test_config -C to-clone uploadpack.allowanysha1inwant true &&
> > +
> > +     scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
> > +     ! grep "Enumerating objects" stderr &&
> > +     ! grep "Updating files" stderr &&
>
> Here, it would be good to still have the GIT_PROGRESS_DELAY=0
> environment variable on the 'scalar clone' command to be sure
> we are not getting these lines because progress is turned off
> and not because it's running too quickly.
>

OK, that makes sense.

> > +     cleanup_clone $enlistment
> > +'
> >  test_done
>
> A nit: there should be an empty line between the end quote of
> the last test and "test_done".
>
> Thanks,
> -Stolee

Thanks,
-ZheNing Hu

[PATCH v3] scalar: show progress if stderr refer to a terminal

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

Sometimes when users use scalar to download a monorepo
with a long commit history, they want to check the
progress bar to know how long they still need to wait
during the fetch process, but scalar suppresses this
output by default.

So let's check whether scalar stderr refer to a terminal,
if so, show progress, otherwise disable it.

Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    scalar: show progress if stderr refer to a terminal
    
    When users use scalar to download a monorepo with a long commit history
    (or the client and server network communication is very poor), we often
    need to spend a long time in the fetch phase of scalar, some users may
    want to check this progress bar To understand the progress of fetch and
    how long they have to wait, so we should enable scalar to display fetch
    progress.
    
    v1. add [--verbose| -v] to scalar clone.
    
    v2.
    
     1. remove --verbose option.
     2. check if scalar stderr refer to terminal, if so, show progress.
    
    v3.
    
     1. fix some tests suggested by Derrick Stolee.
    
    Note: output look like this:
    
    $ scalar clone git@github.com:git/git.git
    Initialized empty Git repository in /home/adl/test/git/src/.git/
    remote: Enumerating objects: 208997, done.
    remote: Counting objects: 100% (870/870), done.
    remote: Compressing objects: 100% (870/870), done.
    remote: Total 208991 (delta 0), reused 870 (delta 0), pack-reused 208121
    remote: Enumerating objects: 470, done.
    remote: Counting objects: 100% (418/418), done.
    remote: Compressing objects: 100% (418/418), done.
    remote: Total 470 (delta 1), reused 0 (delta 0), pack-reused 52
    Receiving objects: 100% (470/470), 1.96 MiB | 1.64 MiB/s, done.
    Resolving deltas: 100% (1/1), done.
    Updating files: 100% (471/471), done.
    branch 'master' set up to track 'origin/master'.
    Switched to a new branch 'master'
    Your branch is up to date with 'origin/master'.
    
    
    "new branch", "new tag" output is a bit annoying, it would be better to
    suppress them, but keep the progress.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1441%2Fadlternative%2Fzh%2Fscalar-verbosity-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1441/adlternative/zh/scalar-verbosity-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1441

Range-diff vs v2:

 1:  2e4c296bd19 ! 1:  38e7e0d44d1 scalar: show progress if stderr refer to a terminal
     @@ t/t9211-scalar-clone.sh: test_expect_success '--no-single-branch clones all bran
      +
      +	test_terminal env GIT_PROGRESS_DELAY=0 \
      +		scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
     -+	grep --count "Enumerating objects" stderr >actual &&
     -+	echo 2 >expected &&
     -+	test_cmp expected actual &&
     ++	grep "Enumerating objects" stderr >actual &&
     ++	test_line_count = 2 actual &&
      +	cleanup_clone $enlistment
      +'
      +
     -+test_expect_success 'progress without tty' '
     ++test_expect_success TTY 'progress without tty' '
      +	enlistment=progress2 &&
      +
      +	test_config -C to-clone uploadpack.allowfilter true &&
      +	test_config -C to-clone uploadpack.allowanysha1inwant true &&
      +
     -+	scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
     ++	GIT_PROGRESS_DELAY=0 scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
      +	! grep "Enumerating objects" stderr &&
      +	! grep "Updating files" stderr &&
      +	cleanup_clone $enlistment
      +'
     ++
       test_done


 scalar.c                | 10 +++++++---
 t/t9211-scalar-clone.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/scalar.c b/scalar.c
index 6c52243cdf1..e5cc554c537 100644
--- a/scalar.c
+++ b/scalar.c
@@ -404,7 +404,7 @@ void load_builtin_commands(const char *prefix, struct cmdnames *cmds)
 static int cmd_clone(int argc, const char **argv)
 {
 	const char *branch = NULL;
-	int full_clone = 0, single_branch = 0;
+	int full_clone = 0, single_branch = 0, show_progress = isatty(2);
 	struct option clone_options[] = {
 		OPT_STRING('b', "branch", &branch, N_("<branch>"),
 			   N_("branch to checkout after clone")),
@@ -499,7 +499,9 @@ static int cmd_clone(int argc, const char **argv)
 	if (set_recommended_config(0))
 		return error(_("could not configure '%s'"), dir);
 
-	if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
+	if ((res = run_git("fetch", "--quiet",
+				show_progress ? "--progress" : "--no-progress",
+				"origin", NULL))) {
 		warning(_("partial clone failed; attempting full clone"));
 
 		if (set_config("remote.origin.promisor") ||
@@ -508,7 +510,9 @@ static int cmd_clone(int argc, const char **argv)
 			goto cleanup;
 		}
 
-		if ((res = run_git("fetch", "--quiet", "origin", NULL)))
+		if ((res = run_git("fetch", "--quiet",
+					show_progress ? "--progress" : "--no-progress",
+					"origin", NULL)))
 			goto cleanup;
 	}
 
diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
index dd33d87e9be..2da8ca6f2bb 100755
--- a/t/t9211-scalar-clone.sh
+++ b/t/t9211-scalar-clone.sh
@@ -3,6 +3,7 @@
 test_description='test the `scalar clone` subcommand'
 
 . ./test-lib.sh
+. "${TEST_DIRECTORY}/lib-terminal.sh"
 
 GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true"
 export GIT_TEST_MAINT_SCHEDULER
@@ -148,4 +149,29 @@ test_expect_success '--no-single-branch clones all branches' '
 	cleanup_clone $enlistment
 '
 
+test_expect_success TTY 'progress with tty' '
+	enlistment=progress1 &&
+
+	test_config -C to-clone uploadpack.allowfilter true &&
+	test_config -C to-clone uploadpack.allowanysha1inwant true &&
+
+	test_terminal env GIT_PROGRESS_DELAY=0 \
+		scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
+	grep "Enumerating objects" stderr >actual &&
+	test_line_count = 2 actual &&
+	cleanup_clone $enlistment
+'
+
+test_expect_success TTY 'progress without tty' '
+	enlistment=progress2 &&
+
+	test_config -C to-clone uploadpack.allowfilter true &&
+	test_config -C to-clone uploadpack.allowanysha1inwant true &&
+
+	GIT_PROGRESS_DELAY=0 scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
+	! grep "Enumerating objects" stderr &&
+	! grep "Updating files" stderr &&
+	cleanup_clone $enlistment
+'
+
 test_done

base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
-- 
gitgitgadget

Re: [PATCH v3] scalar: show progress if stderr refer to a terminal

[Derrick Stolee]

On 1/11/2023 8:14 AM, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>

> Range-diff vs v2:

>      -+test_expect_success 'progress without tty' '
>      ++test_expect_success TTY 'progress without tty' '

I think this addition of the TTY prerequisite is not necessary...

> +test_expect_success TTY 'progress without tty' '
> +	enlistment=progress2 &&
> +
> +	test_config -C to-clone uploadpack.allowfilter true &&
> +	test_config -C to-clone uploadpack.allowanysha1inwant true &&
> +
> +	GIT_PROGRESS_DELAY=0 scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
> +	! grep "Enumerating objects" stderr &&
> +	! grep "Updating files" stderr &&
> +	cleanup_clone $enlistment
> +'

...because the test doesn't use the environment details for
mimicing a TTY. The point is that stderr is redirected to a
file and isatty(2) would report false.

I don't think this is worth a re-roll, though, so I'm happy
with this version.

Thanks,
-Stolee

Re: [PATCH v3] scalar: show progress if stderr refer to a terminal

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 1/11/2023 8:14 AM, ZheNing Hu via GitGitGadget wrote:
>> From: ZheNing Hu <adlternative@gmail.com>
>
>> Range-diff vs v2:
>
>>      -+test_expect_success 'progress without tty' '
>>      ++test_expect_success TTY 'progress without tty' '
>
> I think this addition of the TTY prerequisite is not necessary...
>
>> +test_expect_success TTY 'progress without tty' '
>> +	enlistment=progress2 &&
>> +
>> +	test_config -C to-clone uploadpack.allowfilter true &&
>> +	test_config -C to-clone uploadpack.allowanysha1inwant true &&
>> +
>> +	GIT_PROGRESS_DELAY=0 scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
>> +	! grep "Enumerating objects" stderr &&
>> +	! grep "Updating files" stderr &&
>> +	cleanup_clone $enlistment
>> +'
>
> ...because the test doesn't use the environment details for
> mimicing a TTY. The point is that stderr is redirected to a
> file and isatty(2) would report false.

Yup, the prerequisite was uttering misleading.  I may queue it with
local tweaks, but if I forget please send in an update.

Thanks.

> I don't think this is worth a re-roll, though, so I'm happy
> with this version.
>
> Thanks,
> -Stolee