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

[ZheNing Hu <adlternative@gmail.com>]

Hi,

Derrick Stolee <derrickstolee@github.com> 于2022年12月9日周五 00:30写道：
>
> 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.
>

Good point that we should care about atty.

I guess you mean is to add a parameter to run_git(), which can
control if git commands show stderr/stdout... This solution
may be better. Because git checkout should also have the
same behavior as git fetch: quiet or verbose.

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

Sorry, but I don't understand: I deleted "--quiet", and the progress bar can
also be displayed. Why do you say "not change the way we are
calling 'git fetch'"？

> You might want to add the "--progress" option in the verbose
> case.
>

Good advice.

> 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 for the reminder, I will pay attention to this "TTY" and
"GIT_PROGRESS_DELAY" when I write tests later.

> Thanks,
> -Stolee

Thanks,
-ZheNing Hu