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

[ZheNing Hu <adlternative@gmail.com>]

Derrick Stolee <derrickstolee@github.com> 于2023年1月6日周五 03:19写道：
>
> 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.
>

Ah, It's okay.

> > 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.
>

Yes, but I think something like `--quiet` is difficult to implement.
We cannot just add `--no-progress` or `--quiet` to the git checkout
for suppressing the progress. Because git checkout will not use it
to suppress the fetch progress, but only the checkout's 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.
>

Oh, good suggestion. I should also use grep without `--count` too.

> > +     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.
>

OK, that makes sense.

> > +     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

Thanks,
-ZheNing Hu