On 12/25/22 8:29 AM, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@xxxxxxxxx>
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
