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

From: Derrick Stolee <derrickstolee@github.com>
Date: Dec 8, 2022, 16:30 UTC
Message-ID: <95e92f84-5f83-6439-0d02-f8060669a4d3@github.com>
In-Reply-To: <pull.1441.git.1670436656379.gitgitgadget@gmail.com>

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