threads / patch / 58918

patchscalar: use verbose mode in clone

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

## tl;dr

12 messages between Dec 7, 2022 and Jan 13, 2023. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

ZheNing Hu via GitGitGadget· Dec 7, 2022, 18:10 UTC · lore
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(-)
Show changes to 2 files +14 −4

Documentation/scalar.txt, scalar.c

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
Taylor Blau· Dec 7, 2022, 22:10 UTC · re: ZheNing Hu via GitGitGadget · lore

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

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.
Show 9 quoted lines
> @@ -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.
Show 24 quoted lines
>  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.
Show 8 quoted lines
> @@ -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

ZheNing Hu· Dec 8, 2022, 15:54 UTC · re: Taylor Blau · lore

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

Taylor Blau <me@ttaylorr.com> 于2022年12月8日周四 06:10写道：
Show 23 quoted lines
>
> 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.

Show 6 quoted lines
> 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".

Show 48 quoted lines
> 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

Derrick Stolee· Dec 8, 2022, 16:30 UTC · re: ZheNing Hu via GitGitGadget · lore

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

On 12/7/2022 1:10 PM, ZheNing Hu via GitGitGadget wrote:
Show 7 quoted lines
> 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.

Show 15 quoted lines
> -	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

ZheNing Hu· Dec 13, 2022, 16:37 UTC · re: Derrick Stolee · lore

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

Hi,
Derrick Stolee <derrickstolee@github.com> 于2022年12月9日周五 00:30写道：
Show 31 quoted lines
>
> 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.

Show 23 quoted lines
> 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.
Show 11 quoted lines
> 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

ZheNing Hu via GitGitGadget· Dec 25, 2022, 13:29 UTC · re: ZheNing Hu via GitGitGadget · lore

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

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(-)
Show changes to 2 files +33 −3

scalar.c, t/t9211-scalar-clone.sh

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
Derrick Stolee· Jan 5, 2023, 19:19 UTC · re: ZheNing Hu via GitGitGadget · lore

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

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.
Show 8 quoted lines
> 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);
Show 15 quoted lines
> -	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.
Show 8 quoted lines
> +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.
Show 12 quoted lines
> +	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

Junio C Hamano· Jan 6, 2023, 12:30 UTC · re: Derrick Stolee · lore

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

Derrick Stolee <derrickstolee@github.com> writes:
Show 14 quoted lines
>> +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.
ZheNing Hu· Jan 11, 2023, 11:59 UTC · re: Derrick Stolee · lore

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

Derrick Stolee <derrickstolee@github.com> 于2023年1月6日周五 03:19写道：
Show 6 quoted lines
>
> 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.
Show 15 quoted lines
> > 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.

Show 36 quoted lines
> > +     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.
Show 18 quoted lines
> > +     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.
Show 9 quoted lines
> > +     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

ZheNing Hu via GitGitGadget· Jan 11, 2023, 13:14 UTC · re: ZheNing Hu via GitGitGadget · lore

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

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(-)
Show changes to 2 files +33 −3

scalar.c, t/t9211-scalar-clone.sh

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
Derrick Stolee· Jan 11, 2023, 14:55 UTC · re: ZheNing Hu via GitGitGadget · lore

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

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...
Show 11 quoted lines
> +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

Junio C Hamano· Jan 13, 2023, 19:52 UTC · re: Derrick Stolee · lore

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

Derrick Stolee <derrickstolee@github.com> writes:
Show 25 quoted lines
> 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.
Show 5 quoted lines
> I don't think this is worth a re-roll, though, so I'm happy
> with this version.
>
> Thanks,
> -Stolee

← back to recent threads