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

[Derrick Stolee <derrickstolee@github.com>]

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