| Message ID | pull.1441.git.1670436656379.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scalar: use verbose mode in clone | expand |
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. Perhaps something like: -v:: --verbose:: Enable more verbose output when cloning a repository. 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. Is there an opportunity to easily test this new code? Thanks, Taylor
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
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. 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. You might want to add the "--progress" option in the verbose case. 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, -Stolee
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
diff --git a/Documentation/scalar.txt b/Documentation/scalar.txt index f33436c7f65..7ff37b43945 100644 --- a/Documentation/scalar.txt +++ b/Documentation/scalar.txt @@ -8,7 +8,7 @@ scalar - A tool for managing large Git repositories SYNOPSIS -------- [verse] -scalar clone [--single-branch] [--branch <main-branch>] [--full-clone] <url> [<enlistment>] +scalar clone [--single-branch] [--branch <main-branch>] [--verbose | -v] [--full-clone] <url> [<enlistment>] scalar list scalar register [<enlistment>] scalar unregister [<enlistment>] @@ -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. + 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[] = { @@ -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))) { 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))) goto cleanup; }