Re: [PATCH] scalar: use verbose mode in clone
- From
ZheNing Hu <adlternative@gmail.com>
- Date
- Dec 8, 2022, 15:54 UTC
- Message-ID
- <CAOLTT8Qb0euc5WLdi7v_3ovXT4jJ1-t8BO40jjdM4Rj0Ok8Etw@mail.gmail.com>
- In-Reply-To
- <Y5EPVpb511wk5Uw/@nand.local>
Taylor Blau <me@ttaylorr.com> 于2022年12月8日周四 06:10写道：
Show 23 quoted lines
> > 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.
Show 6 quoted lines
> 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".
Show 48 quoted lines
> 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