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

[ZheNing Hu <adlternative@gmail.com>]

Taylor Blau <me@ttaylorr.com> 于2022年12月8日周四 06:10写道：
>
> On Wed, Dec 07, 2022 at 06:10:56PM +0000, ZheNing Hu via GitGitGadget wrote:
> > So add `[--verbose| -v]` to scalar clone, to enable
> > fetch's output.
>
> Seems reasonable.
>
> > @@ -84,6 +84,11 @@ cloning. If the HEAD at the remote did not point at any branch when
> >       A sparse-checkout is initialized by default. This behavior can be
> >       turned off via `--full-clone`.
> >
> > +-v::
> > +--verbose::
> > +     When scalar executes `git fetch`, `--quiet` is used by default to
> > +     suppress the output of fetch, use verbose mode for cancel this.
> > +
>
> This description may be exposing a few too many implementation details
> for our liking. E.g., scalar happens to use `git fetch`, but it might
> not always. That is probably academic, but a more practical reason to do
> some hiding here might just be that it's unnecessary detail to expose in
> our documentation.
>

Hmmm. There are two steps to downloading data from scalar clone:
the first step is to let "git fetch partial clone"  to  download commits,
trees, tags, and the second step is download the blobs corresponding
to the top-level files of the repository during git checkout. So I'm not sure
if I should mention "fetch" here, since the progress bar for the "checkout"
step is able to be displayed.

> Perhaps something like:
>
>     -v::
>     --verbose::
>      Enable more verbose output when cloning a repository.
>

Just mentioning "clone" is fine... But I'm not sure if users will be
confused, why they will "more verbose" instead of two options
"full verbose" or "not verbose".

> Or something simple like that.
>
> >  List
> >  ~~~~
> >
> > diff --git a/scalar.c b/scalar.c
> > index 6c52243cdf1..b1d4504d136 100644
> > --- a/scalar.c
> > +++ b/scalar.c
> > @@ -404,7 +404,7 @@ void load_builtin_commands(const char *prefix, struct cmdnames *cmds)
> >  static int cmd_clone(int argc, const char **argv)
> >  {
> >       const char *branch = NULL;
> > -     int full_clone = 0, single_branch = 0;
> > +     int full_clone = 0, single_branch = 0, verbosity = 0;
> >       struct option clone_options[] = {
> >               OPT_STRING('b', "branch", &branch, N_("<branch>"),
> >                          N_("branch to checkout after clone")),
> > @@ -413,6 +413,7 @@ static int cmd_clone(int argc, const char **argv)
> >               OPT_BOOL(0, "single-branch", &single_branch,
> >                        N_("only download metadata for the branch that will "
> >                           "be checked out")),
> > +             OPT__VERBOSITY(&verbosity),
> >               OPT_END(),
> >       };
> >       const char * const clone_usage[] = {
>
> Looking good.
>
> > @@ -499,7 +500,9 @@ static int cmd_clone(int argc, const char **argv)
> >       if (set_recommended_config(0))
> >               return error(_("could not configure '%s'"), dir);
> >
> > -     if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
> > +     if ((res = run_git("fetch", "origin",
> > +                        verbosity ? NULL : "--quiet",
> > +                        NULL))) {
>
> Hmmph. This and below are a little strange in that they will end up
> calling:
>
>     run_git("fetch", "origin", NULL, NULL);
>
> when running without `--verbose`. `run_git()` will still do the right
> thing and stop reading its arguments after the first NULL that it sees.
> So I doubt that it's a huge deal in practice, but felt worth calling out
> nonetheless.
>

The reason I'm doing this is seeing that toggle_maintenance() already
does this, and it's not buggy, but it's really inelegant.

My personal understanding is that the original intention of run_git()
is to help developers simply put git parameters into the variable parameters
of the function, and run_git() has no good way to understand null values.
Here we put it in run_git () The last is an act of desperation.

> Is there an opportunity to easily test this new code?
>

It's a bit cumbersome, but I will try.

> Thanks,
> Taylor

Thanks,
ZheNing Hu