scalar: use verbose mode in clone
PATCH 1/1 scalar: use verbose mode in clone
From: ZheNing Hu <[email protected]>
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 add [--verbose| -v] to scalar clone, to enable fetch's output.
---
scalar: use verbose mode in clone
When users use scalar to download a monorepo with a long commit history (or the client and server network communication is very poor), we often need to spend a long time in the fetch phase of scalar, some users may want to check this progress bar To understand the progress of fetch and how long they have to wait, so we should enable scalar to display fetch progress.
v1. add [--verbose| -v] to scalar clone.
Note: output look like this:
$ scalar clone -v [email protected]:git/git.git
Initialized empty Git repository in /Users/adl/test/git/src/.git/
remote: Enumerating objects: 209091, done.
remote: Counting objects: 100% (991/991), done.
remote: Compressing objects: 100% (944/944), done.
Receiving objects: 100% (209085/209085), 81.39 MiB | 126.00 KiB/s, done.
remote: Total 209085 (delta 54), reused 979 (delta 47), pack-reused 208094 Resolving deltas: 100% (134000/134000), done.
From github.com:git/git
* [new branch] jch -> origin/jch
* [new branch] main -> origin/main
* [new branch] maint -> origin/maint
* [new branch] master -> origin/master
* [new branch] next -> origin/next
* [new branch] seen -> origin/seen
* [new branch] todo -> origin/todo
* [new tag] v2.39.0-rc2 -> v2.39.0-rc2
* [new tag] gitgui-0.10.0 -> gitgui-0.10.0
* [new tag] gitgui-0.10.1 -> gitgui-0.10.1
* [new tag] gitgui-0.10.2 -> gitgui-0.10.2
* [new tag] gitgui-0.11.0 -> gitgui-0.11.0
...
"new branch", "new tag" output is a bit annoying, it would be better to suppress them, but keep the progress.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1441%2Fadlternative%2Fzh%2Fscalar-verbosity-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1441/adlternative/zh/scalar-verbosity-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1441
Documentation/scalar.txt | 7 ++++++- scalar.c | 11 ++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-)
Documentation/scalar.txt +6 -1
@@ -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 ~~~~
scalar.c +8 -3
@@ -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; }
base-commit: 2e71cbbddd64695d43383c25c7a054ac4ff86882
Signature · 1 line
gitgitgadget
Quoted 2 lines · ZheNing Hu via GitGitGadget
So add
[--verbose| -v]to scalar clone, to enable fetch's output.
Seems reasonable.
Documentation/scalar.txt cloning. If the HEAD at the remote did not point at any branch when · line 91
Quoted 9 lines
Documentation/scalar.txt +5
-0@@ -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.
Quoted 17 lines
(quoted diff) +0
-0List ~~~~scalar.c +1
-1@@ -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" )),
scalar.c static int cmd_clone(int argc, const char **argv) · line 419
Quoted 7 lines
scalar.c +1
-0@@ -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.
scalar.c static int cmd_clone(int argc, const char **argv) · line 505
Quoted 8 lines
scalar.c +3
-1@@ -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
Quoted 11 lines · Taylor Blau <[email protected]> 于2022年12月8日周四 06:10写道：
Quoted 2 lines · ZheNing Hu via GitGitGadget
So add
[--verbose| -v]to scalar clone, to enable fetch's output.Seems reasonable.
Quoted 4 lines
Documentation/scalar.txt +0
-0@@ -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`.
Quoted 12 lines
Documentation/scalar.txt cloning. If the HEAD at the remote did not point at any branch when · line 91
Quoted 5 lines
Documentation/scalar.txt +5
-0+-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.
Quoted 6 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".
Quoted 41 lines
Or something simple like that.
Quoted 17 lines
(quoted diff) +0
-0List ~~~~scalar.c +1
-1@@ -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" )),scalar.c static int cmd_clone(int argc, const char **argv) · line 419
Quoted 7 lines
scalar.c +1
-0@@ -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.
scalar.c static int cmd_clone(int argc, const char **argv) · line 505
Quoted 8 lines
scalar.c +3
-1@@ -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:
Quoted 7 lines
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.
Quoted 2 lines
Is there an opportunity to easily test this new code?
It's a bit cumbersome, but I will try.
Quoted 2 lines
Thanks,
Taylor
Thanks,
ZheNing Hu
Quoted 7 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
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.
Quoted 2 lines
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.
Quoted 8 lines
scalar.c +3
-1-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") ||
scalar.c static int cmd_clone(int argc, const char **argv) · line 516
Quoted 7 lines
scalar.c +3
-1@@ -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,
Quoted 27 lines · Derrick Stolee <[email protected]> 于2022年12月9日周五 00:30写道：
Quoted 7 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
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.
Quoted 2 lines
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).
Quoted 4 lines
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.
Quoted 14 lines
Also, I'm not sure your implementation is doing the right thing.
Quoted 11 lines
scalar.c +3
-1-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; }
Quoted 9 lines
scalar.c static int cmd_clone(int argc, const char **argv) · line 516
Quoted 4 lines
scalar.c +3
-1-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'"？
Quoted 3 lines
You might want to add the "--progress" option in the verbose case.
Good advice.
Quoted 11 lines
As Taylor mentioned, a test might be helpful. Here's an example from
t7700-repack.shthat 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_successTTY '--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.
Quoted 2 lines
Thanks,
-Stolee
Thanks,
-ZheNing Hu
PATCH v2 1/1 scalar: show progress if stderr refer to a terminal
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
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.
---
scalar: show progress if stderr refer to a terminal
When users use scalar to download a monorepo with a long commit history (or the client and server network communication is very poor), we often need to spend a long time in the fetch phase of scalar, some users may want to check this progress bar To understand the progress of fetch and how long they have to wait, so we should enable scalar to display fetch progress.
v1. add [--verbose| -v] to scalar clone.
v2.
1. remove --verbose option.
2. check if scalar stderr refer to terminal, if so, show progress.
Note: output look like this:
$ scalar clone [email protected]:git/git.git
Initialized empty Git repository in /home/adl/test/git/src/.git/
remote: Enumerating objects: 208997, done.
remote: Counting objects: 100% (870/870), done.
remote: Compressing objects: 100% (870/870), done.
remote: Total 208991 (delta 0), reused 870 (delta 0), pack-reused 208121
remote: Enumerating objects: 470, done.
remote: Counting objects: 100% (418/418), done.
remote: Compressing objects: 100% (418/418), done.
remote: Total 470 (delta 1), reused 0 (delta 0), pack-reused 52 Receiving objects: 100% (470/470), 1.96 MiB | 1.64 MiB/s, done. Resolving deltas: 100% (1/1), done.
Updating files: 100% (471/471), done.
branch 'master' set up to track 'origin/master'.
Switched to a new branch 'master'
Your branch is up to date with 'origin/master'.
"new branch", "new tag" output is a bit annoying, it would be better to suppress them, but keep the progress.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1441%2Fadlternative%2Fzh%2Fscalar-verbosity-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1441/adlternative/zh/scalar-verbosity-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1441
Range-diff vs v1:
1: 6169841190a ! 1: 2e4c296bd19 scalar: use verbose mode in clone
@@ Metadata
Author: ZheNing Hu <[email protected]>
## Commit message ##
- scalar: use verbose mode in clone
+ scalar: show progress if stderr refer to a terminal
Sometimes when users use scalar to download a monorepo
with a long commit history, they want to check the
@@ Commit message
during the fetch process, but scalar suppresses this
output by default.
- So add `[--verbose| -v]` to scalar clone, to enable
- fetch's output.
+ So let's check whether scalar stderr refer to a terminal,
+ if so, show progress, otherwise disable it.
Signed-off-by: ZheNing Hu <[email protected]>
- ## Documentation/scalar.txt ##
-@@ Documentation/scalar.txt: 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>]
-@@ Documentation/scalar.txt: 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
- ~~~~
-
-
## scalar.c ##
@@ scalar.c: 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;
++ int full_clone = 0, single_branch = 0, show_progress = isatty(2);
struct option clone_options[] = {
OPT_STRING('b', "branch", &branch, N_("<branch>"),
N_("branch to checkout after clone")),
-@@ scalar.c: 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[] = {
@@ scalar.c: 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))) {
++ 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") ||
@@ scalar.c: static int cmd_clone(int argc, const char **argv)
}
- if ((res = run_git("fetch", "--quiet", "origin", NULL)))
-+ if ((res = run_git("fetch", "origin",
-+ verbosity ? NULL : "--quiet",
-+ NULL)))
++ if ((res = run_git("fetch", "--quiet",
++ show_progress ? "--progress" : "--no-progress",
++ "origin", NULL)))
goto cleanup;
}
+
+ ## t/t9211-scalar-clone.sh ##
+@@
+ test_description='test the `scalar clone` subcommand'
+
+ . ./test-lib.sh
++. "${TEST_DIRECTORY}/lib-terminal.sh"
+
+ GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true"
+ export GIT_TEST_MAINT_SCHEDULER
+@@ t/t9211-scalar-clone.sh: test_expect_success '--no-single-branch clones all branches' '
+ cleanup_clone $enlistment
+ '
+
++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 &&
++ grep --count "Enumerating objects" stderr >actual &&
++ echo 2 >expected &&
++ test_cmp expected actual &&
++ 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 &&
++ cleanup_clone $enlistment
++'
+ test_donescalar.c | 10 +++++++--- t/t9211-scalar-clone.sh | 26 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-)
scalar.c +7 -3
@@ -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 , show_progress =isatty (2 );struct option clone_options[] = {OPT_STRING ('b' ,"branch" , &branch,N_ ("<branch>" ),N_ ("branch to checkout after clone" )),
@@ -499,7 +499,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" ,"--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 ))) goto cleanup; }
t/t9211-scalar-clone.sh +26 -0
@@ -3,6 +3,7 @@ test_description='test the `scalar clone` subcommand' . ./test-lib.sh +. "${TEST_DIRECTORY}/lib-terminal.sh" GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true" export GIT_TEST_MAINT_SCHEDULER
@@ -148,4 +149,29 @@ test_expect_success '--no-single-branch clones all branches' ' cleanup_clone $enlistment ' +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 && + grep --count "Enumerating objects" stderr >actual && + echo 2 >expected && + test_cmp expected actual && + 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 && + cleanup_clone $enlistment +' test_done
base-commit: 2e71cbbddd64695d43383c25c7a054ac4ff86882
Signature · 1 line
gitgitgadget
Re: [PATCH v2] scalar: show progress if stderr refer to a terminal
Quoted 1 line · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
Sorry for the long wait in getting back to reviewing.
Quoted 8 lines
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.
Quoted 10 lines
scalar.c +4
-1+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") ||
scalar.c static int cmd_clone(int argc, const char **argv) · line 515
Quoted 7 lines
scalar.c +3
-1@@ -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.
t/t9211-scalar-clone.sh test_expect_success '--no-single-branch clones all branches' ' · line 159
Quoted 8 lines
t/t9211-scalar-clone.sh +8
-0+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!
t/t9211-scalar-clone.sh test_expect_success '--no-single-branch clones all branches' ' · line 162
Quoted 3 lines
t/t9211-scalar-clone.sh +3
-0+ grep --count "Enumerating objects" stderr >actual && + echo 2 >expected && + test_cmp expected actual &&
I think you could use "test_line_count = 2 actual" here.
t/t9211-scalar-clone.sh test_expect_success '--no-single-branch clones all branches' ' · line 174
Quoted 12 lines
t/t9211-scalar-clone.sh +12
-0+ 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.
t/t9211-scalar-clone.sh test_expect_success '--no-single-branch clones all branches' ' · line 177
Quoted 3 lines
t/t9211-scalar-clone.sh +2
-0+ 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
Re: [PATCH v2] scalar: show progress if stderr refer to a terminal
Quoted 6 lines · Derrick Stolee
Quoted 6 lines
t/t9211-scalar-clone.sh +6
-0+test_expect_success 'progress without tty' ' + enlistment=progress2 && + + test_config -C to-clone uploadpack.allowfilter true && + test_config -C to-clone uploadpack.allowanysha1inwant true && +
Quoted 8 lines
t/t9211-scalar-clone.sh test_expect_success '--no-single-branch clones all branches' ' · line 174
Quoted 3 lines
t/t9211-scalar-clone.sh +3
-0+ 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.
Good point.
Re: [PATCH v2] scalar: show progress if stderr refer to a terminal
In reply to Derrick StoleeQuoted 6 lines · Derrick Stolee <[email protected]> 于2023年1月6日周五 03:19写道：
Quoted 1 line · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
Sorry for the long wait in getting back to reviewing.
Ah, It's okay.
Quoted 6 lines
Quoted 6 lines
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.
Quoted 9 lines
Quoted 2 lines
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.
Quoted 30 lines
Quoted 10 lines
scalar.c +4
-1+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") ||scalar.c static int cmd_clone(int argc, const char **argv) · line 515
Quoted 7 lines
scalar.c +3
-1@@ -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.
t/t9211-scalar-clone.sh test_expect_success '--no-single-branch clones all branches' ' · line 159
Quoted 8 lines
t/t9211-scalar-clone.sh +8
-0+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!
Quoted 6 lines
t/t9211-scalar-clone.sh test_expect_success '--no-single-branch clones all branches' ' · line 162
Quoted 3 lines
t/t9211-scalar-clone.sh +3
-0+ 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.
Quoted 9 lines
Quoted 9 lines
t/t9211-scalar-clone.sh +9
-0+ 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 && +
Quoted 9 lines
t/t9211-scalar-clone.sh test_expect_success '--no-single-branch clones all branches' ' · line 174
Quoted 3 lines
t/t9211-scalar-clone.sh +3
-0+ 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.
Quoted 9 lines
Quoted 3 lines
+
cleanup_clone$enlistment
+'
test_doneA nit: there should be an empty line between the end quote of the last test and "
test_done".Thanks,
-Stolee
Thanks,
-ZheNing Hu
PATCH v3 1/1 scalar: show progress if stderr refer to a terminal
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
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.
---
scalar: show progress if stderr refer to a terminal
When users use scalar to download a monorepo with a long commit history (or the client and server network communication is very poor), we often need to spend a long time in the fetch phase of scalar, some users may want to check this progress bar To understand the progress of fetch and how long they have to wait, so we should enable scalar to display fetch progress.
v1. add [--verbose| -v] to scalar clone.
v2.
1. remove --verbose option.
2. check if scalar stderr refer to terminal, if so, show progress.
v3.
1. fix some tests suggested by Derrick Stolee.
Note: output look like this:
$ scalar clone [email protected]:git/git.git
Initialized empty Git repository in /home/adl/test/git/src/.git/
remote: Enumerating objects: 208997, done.
remote: Counting objects: 100% (870/870), done.
remote: Compressing objects: 100% (870/870), done.
remote: Total 208991 (delta 0), reused 870 (delta 0), pack-reused 208121
remote: Enumerating objects: 470, done.
remote: Counting objects: 100% (418/418), done.
remote: Compressing objects: 100% (418/418), done.
remote: Total 470 (delta 1), reused 0 (delta 0), pack-reused 52 Receiving objects: 100% (470/470), 1.96 MiB | 1.64 MiB/s, done. Resolving deltas: 100% (1/1), done.
Updating files: 100% (471/471), done.
branch 'master' set up to track 'origin/master'.
Switched to a new branch 'master'
Your branch is up to date with 'origin/master'.
"new branch", "new tag" output is a bit annoying, it would be better to suppress them, but keep the progress.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1441%2Fadlternative%2Fzh%2Fscalar-verbosity-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1441/adlternative/zh/scalar-verbosity-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1441
Range-diff vs v2:
1: 2e4c296bd19 ! 1: 38e7e0d44d1 scalar: show progress if stderr refer to a terminal
@@ t/t9211-scalar-clone.sh: test_expect_success '--no-single-branch clones all bran
+
+ test_terminal env GIT_PROGRESS_DELAY=0 \
+ scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
-+ grep --count "Enumerating objects" stderr >actual &&
-+ echo 2 >expected &&
-+ test_cmp expected actual &&
++ grep "Enumerating objects" stderr >actual &&
++ test_line_count = 2 actual &&
+ cleanup_clone $enlistment
+'
+
-+test_expect_success 'progress without tty' '
++test_expect_success TTY '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 &&
++ GIT_PROGRESS_DELAY=0 scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
+ ! grep "Enumerating objects" stderr &&
+ ! grep "Updating files" stderr &&
+ cleanup_clone $enlistment
+'
++
test_donescalar.c | 10 +++++++--- t/t9211-scalar-clone.sh | 26 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-)
scalar.c +7 -3
@@ -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 , show_progress =isatty (2 );struct option clone_options[] = {OPT_STRING ('b' ,"branch" , &branch,N_ ("<branch>" ),N_ ("branch to checkout after clone" )),
@@ -499,7 +499,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" ,"--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 ))) goto cleanup; }
t/t9211-scalar-clone.sh +26 -0
@@ -3,6 +3,7 @@ test_description='test the `scalar clone` subcommand' . ./test-lib.sh +. "${TEST_DIRECTORY}/lib-terminal.sh" GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true" export GIT_TEST_MAINT_SCHEDULER
@@ -148,4 +149,29 @@ test_expect_success '--no-single-branch clones all branches' ' cleanup_clone $enlistment ' +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 && + grep "Enumerating objects" stderr >actual && + test_line_count = 2 actual && + cleanup_clone $enlistment +' + +test_expect_success TTY 'progress without tty' ' + enlistment=progress2 && + + test_config -C to-clone uploadpack.allowfilter true && + test_config -C to-clone uploadpack.allowanysha1inwant true && + + GIT_PROGRESS_DELAY=0 scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr && + ! grep "Enumerating objects" stderr && + ! grep "Updating files" stderr && + cleanup_clone $enlistment +' + test_done
base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
Signature · 1 line
gitgitgadget
Re: [PATCH v3] scalar: show progress if stderr refer to a terminal
Quoted 6 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
Range-diff vs v2:
-+test_expect_success 'progress without tty' ' ++test_expect_success TTY 'progress without tty' '
I think this addition of the TTY prerequisite is not necessary...
t/t9211-scalar-clone.sh test_expect_success '--no-single-branch clones all branches' ' · line 175
Quoted 11 lines
t/t9211-scalar-clone.sh +11
-0+test_expect_success TTY 'progress without tty' ' + enlistment=progress2 && + + test_config -C to-clone uploadpack.allowfilter true && + test_config -C to-clone uploadpack.allowanysha1inwant true && + + GIT_PROGRESS_DELAY=0 scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr && + ! grep "Enumerating objects" stderr && + ! grep "Updating files" stderr && + cleanup_clone $enlistment +'
...because the test doesn't use the environment details for mimicing a TTY. The point is that stderr is redirected to a file and isatty(2) would report false.
I don't think this is worth a re-roll, though, so I'm happy with this version.
Thanks,
-Stolee
Re: [PATCH v3] scalar: show progress if stderr refer to a terminal
Quoted 10 lines · Derrick Stolee
Quoted 6 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
Range-diff vs v2:
-+test_expect_success 'progress without tty' ' ++test_expect_success TTY 'progress without tty' '
I think this addition of the TTY prerequisite is not necessary...
Quoted 15 lines
t/t9211-scalar-clone.sh test_expect_success '--no-single-branch clones all branches' ' · line 175
Quoted 11 lines
t/t9211-scalar-clone.sh +11
-0+test_expect_success TTY 'progress without tty' ' + enlistment=progress2 && + + test_config -C to-clone uploadpack.allowfilter true && + test_config -C to-clone uploadpack.allowanysha1inwant true && + + GIT_PROGRESS_DELAY=0 scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr && + ! grep "Enumerating objects" stderr && + ! grep "Updating files" stderr && + cleanup_clone $enlistment +'...because the test doesn't use the environment details for mimicing a TTY. The point is that stderr is redirected to a file and isatty(2) would report false.
Yup, the prerequisite was uttering misleading. I may queue it with local tweaks, but if I forget please send in an update.
Thanks.
Quoted 5 lines
I don't think this is worth a re-roll, though, so I'm happy with this version.
Thanks,
-Stolee