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

[Derrick Stolee <derrickstolee@github.com>]

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