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

Commit Message

ZheNing Hu Jan. 11, 2023, 1:14 p.m. UTC

From: ZheNing Hu <[e-mail]>

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 <[e-mail]>
---
    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 [e-mail]: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(-)


base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1

Comments

Derrick Stolee Jan. 11, 2023, 2:55 p.m. UTC | #1

On 1/11/2023 8:14 AM, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <[e-mail]>

> 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

Junio C Hamano Jan. 13, 2023, 7:52 p.m. UTC | #2

Derrick Stolee <[e-mail]> writes:

> On 1/11/2023 8:14 AM, ZheNing Hu via GitGitGadget wrote:
>> From: ZheNing Hu <[e-mail]>
>
>> 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

Patch

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