commit-graph: use start_delayed_progress()
cover commit-graph: use start_delayed_progress()
Thanks, Ryenus, for reporting this problem.
Derrick Stolee (1):
commit-graph: use start_delayed_progress()
commit-graph.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-450%2Fderrickstolee%2Fcommit-graph-progress-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-450/derrickstolee/commit-graph-progress-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/450
Signature · 1 line
gitgitgadget
PATCH 1/1 commit-graph: use start_delayed_progress()
From: Derrick Stolee <[email protected]>
When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.
However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.
--- commit-graph.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
commit-graph.c +1 -1
@@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)struct commit_list *list =NULL ;if (ctx->report_progress) - ctx->progress =start_progress ( + ctx->progress =start_delayed_progress (_ ("Computing commit graph generation numbers" ), ctx->commits.nr);for (i =0 ; i < ctx->commits.nr; i++) {
Signature · 1 line
gitgitgadget
Quoted 10 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
When writing a commit-graph, we show progress along several commit walks. When we use
start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.
For course, now that we do not force at least one progress line to show up, the tests that check the --progress option (or --no-quiet for GC) will fail with this patch.
v2 coming soon.
-Stolee
v2 cover commit-graph: use start_delayed_progress()
In reply to Derrick Stolee via GitGitGadgetThanks, Ryenus, for reporting this problem.
Derrick Stolee (1):
commit-graph: use start_delayed_progress()
commit-graph.c | 2 +- t/t5318-commit-graph.sh | 6 ------ t/t6500-gc.sh | 14 -------------- 3 files changed, 1 insertion(+), 21 deletions(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-450%2Fderrickstolee%2Fcommit-graph-progress-fix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-450/derrickstolee/commit-graph-progress-fix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/450
Range-diff vs v1:
1: 174c05bf29 ! 1: 78bd6bc2c0 commit-graph: use start_delayed_progress()
@@ -11,6 +11,10 @@
previous step. But, this is showing up for all users no matter how few commits are being added.
+ Now that we changed this method, very fast commands show no progess at
+ all. This means we need to stop testing for seeing these progress lines
+ in the test suite.
+
@@ -26,3 +30,45 @@
_("Computing commit graph generation numbers"), ctx->commits.nr);
for (i = 0; i < ctx->commits.nr; i++) {
+
+ diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
+ --- a/t/t5318-commit-graph.sh
+ +++ b/t/t5318-commit-graph.sh
+@@
+ test_line_count = 0 err
+ '
+
+-test_expect_success 'commit-graph write force progress on for stderr' '
+- cd "$TRASH_DIRECTORY/full" &&
+- git commit-graph write --progress 2>err &&
+- test_file_not_empty err
+-'
+-
+ test_expect_success 'commit-graph write with the --no-progress option' '
+ cd "$TRASH_DIRECTORY/full" &&
+ git commit-graph write --no-progress 2>err &&
+
+ diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
+ --- a/t/t6500-gc.sh
+ +++ b/t/t6500-gc.sh
+@@
+ test_line_count = 2 new # There is one new pack and its .idx
+ '
+
+-test_expect_success 'gc --no-quiet' '
+- git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
+- test_must_be_empty stdout &&
+- test_line_count = 1 stderr &&
+- test_i18ngrep "Computing commit graph generation numbers" stderr
+-'
+-
+-test_expect_success TTY 'with TTY: gc --no-quiet' '
+- test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
+- test_must_be_empty stdout &&
+- test_i18ngrep "Enumerating objects" stderr &&
+- test_i18ngrep "Computing commit graph generation numbers" stderr
+-'
+-
+ test_expect_success 'gc --quiet' '
+ git -c gc.writeCommitGraph=true gc --quiet >stdout 2>stderr &&
+ test_must_be_empty stdout &&
Signature · 1 line
gitgitgadget
PATCH v2 1/1 commit-graph: use start_delayed_progress()
From: Derrick Stolee <[email protected]>
When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.
However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.
Now that we changed this method, very fast commands show no progess at all. This means we need to stop testing for seeing these progress lines in the test suite.
--- commit-graph.c | 2 +- t/t5318-commit-graph.sh | 6 ------ t/t6500-gc.sh | 14 -------------- 3 files changed, 1 insertion(+), 21 deletions(-)
commit-graph.c +1 -1
@@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)struct commit_list *list =NULL ;if (ctx->report_progress) - ctx->progress =start_progress ( + ctx->progress =start_delayed_progress (_ ("Computing commit graph generation numbers" ), ctx->commits.nr);for (i =0 ; i < ctx->commits.nr; i++) {
t/t5318-commit-graph.sh +0 -6
@@ -130,12 +130,6 @@ test_expect_success 'commit-graph write progress off for redirected stderr' ' test_line_count = 0 err ' -test_expect_success 'commit-graph write force progress on for stderr' ' - cd "$TRASH_DIRECTORY/full" && - git commit-graph write --progress 2>err && - test_file_not_empty err -' - test_expect_success 'commit-graph write with the --no-progress option' ' cd "$TRASH_DIRECTORY/full" && git commit-graph write --no-progress 2>err &&
t/t6500-gc.sh +0 -14
@@ -102,20 +102,6 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre test_line_count = 2 new # There is one new pack and its .idx ' -test_expect_success 'gc --no-quiet' ' - git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && - test_must_be_empty stdout && - test_line_count = 1 stderr && - test_i18ngrep "Computing commit graph generation numbers" stderr -' - -test_expect_success TTY 'with TTY: gc --no-quiet' ' - test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && - test_must_be_empty stdout && - test_i18ngrep "Enumerating objects" stderr && - test_i18ngrep "Computing commit graph generation numbers" stderr -' - test_expect_success 'gc --quiet' ' git -c gc.writeCommitGraph=true gc --quiet >stdout 2>stderr && test_must_be_empty stdout &&
Signature · 1 line
gitgitgadget
Quoted 10 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
When writing a commit-graph, we show progress along several commit walks. When we use
start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.
Yep, makes sense (especially that it should all the other progress as part of the same process).
Quoted 3 lines
Now that we changed this method, very fast commands show no progess at all. This means we need to stop testing for seeing these progress lines in the test suite.
I think this is OK for now, though it does make me wonder if "--progress" ought to perhaps override "delayed" in some instances, since it's a positive signal from the caller that they're interested in seeing progress.
-Peff
Quoted 16 lines · Jeff King
Quoted 10 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
When writing a commit-graph, we show progress along several commit walks. When we use
start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.
Yep, makes sense (especially that it should all the other progress as part of the same process).
Quoted 8 lines
Quoted 3 lines
Now that we changed this method, very fast commands show no progess at all. This means we need to stop testing for seeing these progress lines in the test suite.
I think this is OK for now, though it does make me wonder if "--progress" ought to perhaps override "delayed" in some instances, since it's a positive signal from the caller that they're interested in seeing progress.
I was thinking that we could start with a GIT_TEST_PROGRESS environment variable to force all delayed progress to act like non-delayed progress. That would at least give us confirmation on these kinds of tests.
The impact of doing that inside the progress code could be large, so perhaps that is best left for a follow-up.
Thanks,
-Stolee
Quoted 4 lines · Jeff King
I think this is OK for now, though it does make me wonder if "--progress" ought to perhaps override "delayed" in some instances, since it's a positive signal from the caller that they're interested in seeing progress.
I did have the same reaction after seeing the change to 5318 where the expected output from "git commit-graph write --progress" has become unreliable.
I think there are possibly three kinds of folks:
- I do not want the output smudged with any progress (e.g. I am a script);
- I want to see progress if it takes very long, but do not waste vertical screen real estate if it does not make me wait (e.g. I am an interactive user who occasionally wants a cue to leave the keyboard to grab coffee); and
- I want to see all progress (... now who am I? Taking a screenshot to write a tutorial or something???).
In the ideal world, the three choices above should probably be "--progress=(no|auto|always)" where not having any defaults to one of them (probably "auto", as the code can use isatty() to further turn it to "no").
Making "--progress" to mean "--progress=always" is OK, but it leaves no way to override an earlier --[no-]progress on the command line, which feels somewhat satisfying.
Thanks.
Quoted 4 lines · Derrick Stolee
Quoted 4 lines
Quoted 3 lines
Now that we changed this method, very fast commands show no progess at all. This means we need to stop testing for seeing these progress lines in the test suite.
Quoted 8 lines
Quoted 4 lines
I think this is OK for now, though it does make me wonder if "--progress" ought to perhaps override "delayed" in some instances, since it's a positive signal from the caller that they're interested in seeing progress.
I was thinking that we could start with a
GIT_TEST_PROGRESSenvironment variable to force all delayed progress to act like non-delayed progress. That would at least give us confirmation on these kinds of tests.
I think this could actually be a non-test variable. E.g., something like this:
progress.c +3 -1
@@ -14,6 +14,7 @@#include "strbuf.h" #include "trace.h" #include "utf8.h" +#include "config.h" #define TP_IDX_MAX 8
@@ -269,7 +270,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,struct progress *start_delayed_progress (const char *title,uint64_t total) { -return start_progress_delay (title, total,2 ,0 ); +int delay_in_secs =git_env_ulong ("GIT_PROGRESS_DELAY" ,2 ); +return start_progress_delay (title, total, delay_in_secs,0 ); }struct progress *start_progress (const char *title,uint64_t total)
which lets you ask for more verbose progress. There are times when I'd use something like this for general debugging. Though these days I might suggest that something like GIT_TRACE2_PERF hook the progress code to output. That's a bit more complicated to implement, though.
-Peff
Quoted 16 lines · Junio C Hamano
Quoted 4 lines · Jeff King
I think this is OK for now, though it does make me wonder if "--progress" ought to perhaps override "delayed" in some instances, since it's a positive signal from the caller that they're interested in seeing progress.
I did have the same reaction after seeing the change to 5318 where the expected output from "git commit-graph write --progress" has become unreliable.
I think there are possibly three kinds of folks:
- I do not want the output smudged with any progress (e.g. I am a script);
Quoted 7 lines
- I want to see progress if it takes very long, but do not waste vertical screen real estate if it does not make me wait (e.g. I am an interactive user who occasionally wants a cue to leave the keyboard to grab coffee); and
- I want to see all progress (... now who am I? Taking a screenshot to write a tutorial or something???).
I think type 3 may be people who want to understand more about the program flow, and where it's at when it sees an error.
Quoted 4 lines
In the ideal world, the three choices above should probably be "--progress=(no|auto|always)" where not having any defaults to one of them (probably "auto", as the code can use
isatty()to further turn it to "no").
I think any no/auto/always here is tricky, because it already has a
meaning: to use or disregard isatty(2). And overriding that might be independent of the "type" (think pack-objects on a server generating output that's going over the wire; we have to tell it "yes, definitely show progress even though there is no terminal", but that has nothing to do with any "delay" decisions).
-Peff
Quoted 3 lines · Jeff King
I think any no/auto/always here is tricky, because it already has a
meaning: to use or disregard isatty(2). And overriding that might be independent of the "type".
Yes, your "let the users optionally give the delay" in the other subthread makes a lot more sense.
Thanks.
Quoted 31 lines · Jeff King
Quoted 4 lines · Derrick Stolee
Quoted 4 lines
Quoted 3 lines
Now that we changed this method, very fast commands show no progess at all. This means we need to stop testing for seeing these progress lines in the test suite.
Quoted 8 lines
Quoted 4 lines
I think this is OK for now, though it does make me wonder if "--progress" ought to perhaps override "delayed" in some instances, since it's a positive signal from the caller that they're interested in seeing progress.
I was thinking that we could start with a
GIT_TEST_PROGRESSenvironment variable to force all delayed progress to act like non-delayed progress. That would at least give us confirmation on these kinds of tests.I think this could actually be a non-test variable. E.g., something like this:
progress.c +1
-0@@ -14,6 +14,7 @@#include "strbuf.h" #include "trace.h" #include "utf8.h" +#include "config.h" #define TP_IDX_MAX 8
progress.c static struct progress *start_progress_delay(const char *title, uint64_t total, · line 277
Quoted 9 lines
progress.c +2
-1@@ -269,7 +270,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,struct progress *start_delayed_progress (const char *title,uint64_t total) { -return start_progress_delay (title, total,2 ,0 ); +int delay_in_secs =git_env_ulong ("GIT_PROGRESS_DELAY" ,2 ); +return start_progress_delay (title, total, delay_in_secs,0 ); }struct progress *start_progress (const char *title,uint64_t total)
I like this idea. It allows us to force the progress on in tests, and for users to tweak their preferred delay. That includes _increasing_ the delay if they want to.
Quoted 4 lines
which lets you ask for more verbose progress. There are times when I'd use something like this for general debugging. Though these days I might suggest that something like
GIT_TRACE2_PERFhook the progress code to output. That's a bit more complicated to implement, though.
Would it make sense to make delay_in_secs a local static variable, so we remember it between calls? That would allow us to check the environment only once (not that it is usually expensive).
-Stolee
v3 cover commit-graph: use start_delayed_progress()
In reply to Derrick Stolee via GitGitGadgetThanks, Ryenus, for reporting this problem.
Update in V3:
Based on our discussion, I've added the suggested GIT_PROGRESS_DELAY environment variable. This allowed the existing tests to stick around with one exception in the GC tests. The test remains, but we can no longer look at the commit-graph output.
Derrick Stolee (2):
progress: create GIT_PROGRESS_DELAY
commit-graph: use start_delayed_progress()
Documentation/git.txt | 4 ++++ commit-graph.c | 2 +- progress.c | 8 +++++++- t/t5318-commit-graph.sh | 4 ++-- t/t6500-gc.sh | 6 ++---- 5 files changed, 16 insertions(+), 8 deletions(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-450%2Fderrickstolee%2Fcommit-graph-progress-fix-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-450/derrickstolee/commit-graph-progress-fix-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/450
Range-diff vs v2:
-: ---------- > 1: 656dba5afb progress: create GIT_PROGRESS_DELAY 1: 78bd6bc2c0 ! 2: 3c0c9675e1 commit-graph: use start_delayed_progress()
@@ -11,9 +11,13 @@
previous step. But, this is showing up for all users no matter how few commits are being added.
- Now that we changed this method, very fast commands show no progess at
- all. This means we need to stop testing for seeing these progress lines
- in the test suite.
+ The tests that check for the progress output have already been updated
+ to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there
+ is one test in t6500-gc.sh that uses the test_terminal method. This
+ mechanism does not preserve the GIT_PROGRESS_DELAY environment variable,
+ so we need to modify check on the output. We still watch for the
+ "Enumerating objects" progress but no longer look for "Computing
+ commit graph generation numbers".
@@ -31,44 +35,16 @@
ctx->commits.nr);
for (i = 0; i < ctx->commits.nr; i++) {
- diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
- --- a/t/t5318-commit-graph.sh
- +++ b/t/t5318-commit-graph.sh
-@@
- test_line_count = 0 err
- '
-
--test_expect_success 'commit-graph write force progress on for stderr' '
-- cd "$TRASH_DIRECTORY/full" &&
-- git commit-graph write --progress 2>err &&
-- test_file_not_empty err
--'
--
- test_expect_success 'commit-graph write with the --no-progress option' '
- cd "$TRASH_DIRECTORY/full" &&
- git commit-graph write --no-progress 2>err &&
-
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@
- test_line_count = 2 new # There is one new pack and its .idx
- '
-
--test_expect_success 'gc --no-quiet' '
-- git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
-- test_must_be_empty stdout &&
-- test_line_count = 1 stderr &&
-- test_i18ngrep "Computing commit graph generation numbers" stderr
--'
--
--test_expect_success TTY 'with TTY: gc --no-quiet' '
-- test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
-- test_must_be_empty stdout &&
+ test_expect_success TTY 'with TTY: gc --no-quiet' '
+ test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
+ test_must_be_empty stdout &&
- test_i18ngrep "Enumerating objects" stderr &&
- test_i18ngrep "Computing commit graph generation numbers" stderr
--'
--
++ test_i18ngrep "Enumerating objects" stderr
+ '
+
test_expect_success 'gc --quiet' '
- git -c gc.writeCommitGraph=true gc --quiet >stdout 2>stderr &&
- test_must_be_empty stdout &&
Signature · 1 line
gitgitgadget
PATCH v3 1/2 progress: create GIT_PROGRESS_DELAY
From: Derrick Stolee <[email protected]>
The start_delayed_progress() method is a preferred way to show optional progress to users as it ignores steps that take less than two seconds. However, this makes testing unreliable as tests expect to be very fast.
In addition, users may want to decrease or increase this time interval depending on their preferences for terminal noise.
Create the GIT_PROGRESS_DELAY environment variable to control the delay set during start_delayed_progress(). Set the value in some tests to guarantee their output remains consistent.
--- Documentation/git.txt | 4 ++++ progress.c | 8 +++++++- t/t5318-commit-graph.sh | 4 ++-- t/t6500-gc.sh | 3 +-- 4 files changed, 14 insertions(+), 5 deletions(-)
Documentation/git.txt +4 -0
@@ -544,6 +544,10 @@ other a pager. See also the `core.pager` option in linkgit:git-config[1]. +`GIT_PROGRESS_DELAY`:: + A number controlling how many seconds to delay before showing + optional progress indicators. Defaults to 2. + `GIT_EDITOR`:: This environment variable overrides `$EDITOR` and `$VISUAL`. It is used by several Git commands when, on interactive mode,
progress.c +7 -1
@@ -14,6 +14,7 @@#include "strbuf.h" #include "trace.h" #include "utf8.h" +#include "config.h" #define TP_IDX_MAX 8
@@ -269,7 +270,12 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,struct progress *start_delayed_progress (const char *title,uint64_t total) { -return start_progress_delay (title, total,2 ,0 ); +static int delay_in_secs =-1 ; + +if (delay_in_secs <0 ) + delay_in_secs =git_env_ulong ("GIT_PROGRESS_DELAY" ,2 ); + +return start_progress_delay (title, total, delay_in_secs,0 ); }struct progress *start_progress (const char *title,uint64_t total)
t/t5318-commit-graph.sh +2 -2
@@ -132,7 +132,7 @@ test_expect_success 'commit-graph write progress off for redirected stderr' ' test_expect_success 'commit-graph write force progress on for stderr' ' cd "$TRASH_DIRECTORY/full" && - git commit-graph write --progress 2>err && + GIT_PROGRESS_DELAY=0 git commit-graph write --progress 2>err && test_file_not_empty err '
@@ -150,7 +150,7 @@ test_expect_success 'commit-graph verify progress off for redirected stderr' ' test_expect_success 'commit-graph verify force progress on for stderr' ' cd "$TRASH_DIRECTORY/full" && - git commit-graph verify --progress 2>err && + GIT_PROGRESS_DELAY=0 git commit-graph verify --progress 2>err && test_file_not_empty err '
t/t6500-gc.sh +1 -2
@@ -103,9 +103,8 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre ' test_expect_success 'gc --no-quiet' ' - git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && + GIT_PROGRESS_DELAY=0 git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && - test_line_count = 1 stderr && test_i18ngrep "Computing commit graph generation numbers" stderr '
Signature · 1 line
gitgitgadget
PATCH v3 2/2 commit-graph: use start_delayed_progress()
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.
However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.
The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there is one test in t6500-gc.sh that uses the test_terminal method. This mechanism does not preserve the GIT_PROGRESS_DELAY environment variable, so we need to modify check on the output. We still watch for the "Enumerating objects" progress but no longer look for "Computing commit graph generation numbers".
--- commit-graph.c | 2 +- t/t6500-gc.sh | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)
commit-graph.c +1 -1
@@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)struct commit_list *list =NULL ;if (ctx->report_progress) - ctx->progress =start_progress ( + ctx->progress =start_delayed_progress (_ ("Computing commit graph generation numbers" ), ctx->commits.nr);for (i =0 ; i < ctx->commits.nr; i++) {
t/t6500-gc.sh +1 -2
@@ -111,8 +111,7 @@ test_expect_success 'gc --no-quiet' ' test_expect_success TTY 'with TTY: gc --no-quiet' ' test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && - test_i18ngrep "Enumerating objects" stderr && - test_i18ngrep "Computing commit graph generation numbers" stderr + test_i18ngrep "Enumerating objects" stderr ' test_expect_success 'gc --quiet' '
Signature · 1 line
gitgitgadget
Re: [PATCH v3 1/2] progress: create GIT_PROGRESS_DELAY
In reply to Derrick Stolee via GitGitGadgetQuoted 7 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
The
start_delayed_progress()method is a preferred way to show optional progress to users as it ignores steps that take less than two seconds. However, this makes testing unreliable as tests expect to be very fast.
Quoted 6 lines
In addition, users may want to decrease or increase this time interval depending on their preferences for terminal noise.
Create the
GIT_PROGRESS_DELAYenvironment variable to control the delay set duringstart_delayed_progress(). Set the value in some tests to guarantee their output remains consistent.
Thanks for wrapping this up. I obviously think this is a good direction to go. :) A few thoughts:
Documentation/git.txt other · line 549
Quoted 11 lines
Documentation/git.txt +3
-0@@ -544,6 +544,10 @@ other a pager. See also the `core.pager` option in linkgit:git-config[1]. +`GIT_PROGRESS_DELAY`:: + A number controlling how many seconds to delay before showing + optional progress indicators. Defaults to 2.
Not all progress meters use delay. I wonder if that might confuse some users, who would try:
GIT_PROGRESS_DELAY=10 git repack -ad
or something, but still see "Enumerating objects".
I guess the key word in your documentation is "optional", but maybe it needs to be spelled out more clearly. I dunno.
Documentation/git.txt static struct progress *start_progress_delay(const char *title, uint64_t total, · line 279
Quoted 12 lines
Documentation/git.txt +6
-1@@ -269,7 +270,12 @@ static struct progress *start_progress_delay(const char *title, uint64_t total, struct progress *start_delayed_progress(const char *title, uint64_t total) { - return start_progress_delay(title, total, 2, 0); + static int delay_in_secs = -1; + + if (delay_in_secs < 0) + delay_in_secs = git_env_ulong("GIT_PROGRESS_DELAY", 2); + + return start_progress_delay(title, total, delay_in_secs, 0); }
You asked earlier if it was worth memo-izing the git_env_ulong() call like this. I suspect it doesn't matter much either way, since progress only starts and stops a few times in a given program. But I'm certainly happy with it this way, as it matches most other environment lookups.
-Peff
Quoted 10 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
When writing a commit-graph, we show progress along several commit walks. When we use
start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.
This part of the patch is a good thing, and obviously correct. But I wondered...
Quoted 4 lines
The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there is one test in
t6500-gc.shthat uses thetest_terminalmethod. This mechanism does not preserve theGIT_PROGRESS_DELAYenvironment variable,
Why doesn't GIT_PROGRESS_DELAY make it through? Overall it's not that big a deal to me if it doesn't, but in this test:
t/t6500-gc.sh test_expect_success 'gc --no-quiet' ' · line 115
Quoted 7 lines
test_expect_successTTY 'with TTY: gc --no-quiet' 't/t6500-gc.sh +1
-2test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && - test_i18ngrep "Enumerating objects" stderr && - test_i18ngrep "Computing commit graph generation numbers" stderr + test_i18ngrep "Enumerating objects" stderr '
We're not actually checking anything related to gc.writeCommitGraph anymore.
Quoted 1 line
so we need to modify check on the output. We still watch for the
Minor typo: s/modify/& the/ or similar?
-Peff
Re: [PATCH v3 1/2] progress: create GIT_PROGRESS_DELAY
In reply to Derrick Stolee via GitGitGadgetQuoted 52 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
The
start_delayed_progress()method is a preferred way to show optional progress to users as it ignores steps that take less than two seconds. However, this makes testing unreliable as tests expect to be very fast.In addition, users may want to decrease or increase this time interval depending on their preferences for terminal noise.
Create the
GIT_PROGRESS_DELAYenvironment variable to control the delay set duringstart_delayed_progress(). Set the value in some tests to guarantee their output remains consistent.Helped-by Jeff King <[email protected]>Signed-off-by Derrick Stolee <[email protected]>--- Documentation/git.txt | 4 ++++ progress.c | 8 +++++++- t/t5318-commit-graph.sh | 4 ++-- t/t6500-gc.sh | 3 +-- 4 files changed, 14 insertions(+), 5 deletions(-)Documentation/git.txt +4
-0@@ -544,6 +544,10 @@ other a pager. See also the `core.pager` option in linkgit:git-config[1]. +`GIT_PROGRESS_DELAY`:: + A number controlling how many seconds to delay before showing + optional progress indicators. Defaults to 2. + `GIT_EDITOR`:: This environment variable overrides `$EDITOR` and `$VISUAL`. It is used by several Git commands when, on interactive mode,progress.c +1
-0@@ -14,6 +14,7 @@#include "strbuf.h" #include "trace.h" #include "utf8.h" +#include "config.h" #define TP_IDX_MAX 8@@ -269,7 +270,12 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
progress.c static struct progress *start_progress_delay(const char *title, uint64_t total, · line 279
Quoted 10 lines
progress.c +6
-1struct progress *start_delayed_progress (const char *title,uint64_t total) { -return start_progress_delay (title, total,2 ,0 ); +static int delay_in_secs =-1 ; + +if (delay_in_secs <0 ) + delay_in_secs =git_env_ulong ("GIT_PROGRESS_DELAY" ,2 ); + +return start_progress_delay (title, total, delay_in_secs,0 ); }
Note that there is the similar start_delayed_sparse_progress() function, which should respect GIT_PROGRESS_DELAY as well.
t/t6500-gc.sh test_expect_success 'auto gc with too many loose objects does not attempt to cre · line 108
Quoted 12 lines
t/t6500-gc.sh +1
-2@@ -103,9 +103,8 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre ' test_expect_success 'gc --no-quiet' ' - git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && + GIT_PROGRESS_DELAY=0 git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && - test_line_count = 1 stderr &&
Good, I'm glad to see this "how many progress lines did we print?" check gone.
Quoted 6 lines
t/t6500-gc.sh +0
-0test_i18ngrep "Computing commit graph generation numbers" stderr 'Signature · 1 line
gitgitgadget
v4 cover commit-graph: use start_delayed_progress()
In reply to Derrick Stolee via GitGitGadgetThanks, Ryenus, for reporting this problem.
Update in V3:
Based on our discussion, I've added the suggested GIT_PROGRESS_DELAY environment variable. This allowed the existing tests to stick around with one exception in the GC tests. The test remains, but we can no longer look at the commit-graph output.
Update in V4:
The GIT_PROGRESS_DELAY check is extracted to a helper method so it can be used with start_delayed_sparse_progress().
Derrick Stolee (2):
progress: create GIT_PROGRESS_DELAY
commit-graph: use start_delayed_progress()
Documentation/git.txt | 4 ++++ commit-graph.c | 2 +- progress.c | 15 +++++++++++++-- t/t5318-commit-graph.sh | 4 ++-- t/t6500-gc.sh | 6 ++---- 5 files changed, 22 insertions(+), 9 deletions(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-450%2Fderrickstolee%2Fcommit-graph-progress-fix-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-450/derrickstolee/commit-graph-progress-fix-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/450
Range-diff vs v3:
1: 656dba5afb ! 1: a7acdf9c8f progress: create GIT_PROGRESS_DELAY
@@ -44,19 +44,35 @@
#define TP_IDX_MAX 8
@@
+ return progress;
+ }
- struct progress *start_delayed_progress(const char *title, uint64_t total)
- {
-- return start_progress_delay(title, total, 2, 0);
++static int get_default_delay(void)
++{
+ static int delay_in_secs = -1;
+
+ if (delay_in_secs < 0)
+ delay_in_secs = git_env_ulong("GIT_PROGRESS_DELAY", 2);
+
-+ return start_progress_delay(title, total, delay_in_secs, 0);
++ return delay_in_secs;
++}
++
+ struct progress *start_delayed_progress(const char *title, uint64_t total)
+ {
+- return start_progress_delay(title, total, 2, 0);
++ return start_progress_delay(title, total, get_default_delay(), 0);
}
struct progress *start_progress(const char *title, uint64_t total)
+@@
+ struct progress *start_delayed_sparse_progress(const char *title,
+ uint64_t total)
+ {
+- return start_progress_delay(title, total, 2, 1);
++ return start_progress_delay(title, total, get_default_delay(), 1);
+ }
+
+ static void finish_if_sparse(struct progress *progress)
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
--- a/t/t5318-commit-graph.sh 2: 3c0c9675e1 = 2: e62dcc1ce5 commit-graph: use start_delayed_progress()
Signature · 1 line
gitgitgadget
PATCH v4 1/2 progress: create GIT_PROGRESS_DELAY
From: Derrick Stolee <[email protected]>
The start_delayed_progress() method is a preferred way to show optional progress to users as it ignores steps that take less than two seconds. However, this makes testing unreliable as tests expect to be very fast.
In addition, users may want to decrease or increase this time interval depending on their preferences for terminal noise.
Create the GIT_PROGRESS_DELAY environment variable to control the delay set during start_delayed_progress(). Set the value in some tests to guarantee their output remains consistent.
--- Documentation/git.txt | 4 ++++ progress.c | 15 +++++++++++++-- t/t5318-commit-graph.sh | 4 ++-- t/t6500-gc.sh | 3 +-- 4 files changed, 20 insertions(+), 6 deletions(-)
Documentation/git.txt +4 -0
@@ -544,6 +544,10 @@ other a pager. See also the `core.pager` option in linkgit:git-config[1]. +`GIT_PROGRESS_DELAY`:: + A number controlling how many seconds to delay before showing + optional progress indicators. Defaults to 2. + `GIT_EDITOR`:: This environment variable overrides `$EDITOR` and `$VISUAL`. It is used by several Git commands when, on interactive mode,
progress.c +13 -2
@@ -14,6 +14,7 @@#include "strbuf.h" #include "trace.h" #include "utf8.h" +#include "config.h" #define TP_IDX_MAX 8
@@ -267,9 +268,19 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,return progress; } +static int get_default_delay (void ) +{ +static int delay_in_secs =-1 ; + +if (delay_in_secs <0 ) + delay_in_secs =git_env_ulong ("GIT_PROGRESS_DELAY" ,2 ); + +return delay_in_secs; +} +struct progress *start_delayed_progress (const char *title,uint64_t total) { -return start_progress_delay (title, total,2 ,0 ); +return start_progress_delay (title, total,get_default_delay (),0 ); }struct progress *start_progress (const char *title,uint64_t total)
@@ -294,7 +305,7 @@ struct progress *start_sparse_progress(const char *title, uint64_t total)struct progress *start_delayed_sparse_progress (const char *title,uint64_t total) { -return start_progress_delay (title, total,2 ,1 ); +return start_progress_delay (title, total,get_default_delay (),1 ); }static void finish_if_sparse (struct progress *progress)
t/t5318-commit-graph.sh +2 -2
@@ -132,7 +132,7 @@ test_expect_success 'commit-graph write progress off for redirected stderr' ' test_expect_success 'commit-graph write force progress on for stderr' ' cd "$TRASH_DIRECTORY/full" && - git commit-graph write --progress 2>err && + GIT_PROGRESS_DELAY=0 git commit-graph write --progress 2>err && test_file_not_empty err '
@@ -150,7 +150,7 @@ test_expect_success 'commit-graph verify progress off for redirected stderr' ' test_expect_success 'commit-graph verify force progress on for stderr' ' cd "$TRASH_DIRECTORY/full" && - git commit-graph verify --progress 2>err && + GIT_PROGRESS_DELAY=0 git commit-graph verify --progress 2>err && test_file_not_empty err '
t/t6500-gc.sh +1 -2
@@ -103,9 +103,8 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre ' test_expect_success 'gc --no-quiet' ' - git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && + GIT_PROGRESS_DELAY=0 git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && - test_line_count = 1 stderr && test_i18ngrep "Computing commit graph generation numbers" stderr '
Signature · 1 line
gitgitgadget
PATCH v4 2/2 commit-graph: use start_delayed_progress()
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.
However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.
The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there is one test in t6500-gc.sh that uses the test_terminal method. This mechanism does not preserve the GIT_PROGRESS_DELAY environment variable, so we need to modify check on the output. We still watch for the "Enumerating objects" progress but no longer look for "Computing commit graph generation numbers".
--- commit-graph.c | 2 +- t/t6500-gc.sh | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)
commit-graph.c +1 -1
@@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)struct commit_list *list =NULL ;if (ctx->report_progress) - ctx->progress =start_progress ( + ctx->progress =start_delayed_progress (_ ("Computing commit graph generation numbers" ), ctx->commits.nr);for (i =0 ; i < ctx->commits.nr; i++) {
t/t6500-gc.sh +1 -2
@@ -111,8 +111,7 @@ test_expect_success 'gc --no-quiet' ' test_expect_success TTY 'with TTY: gc --no-quiet' ' test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && - test_i18ngrep "Enumerating objects" stderr && - test_i18ngrep "Computing commit graph generation numbers" stderr + test_i18ngrep "Enumerating objects" stderr ' test_expect_success 'gc --quiet' '
Signature · 1 line
gitgitgadget
Quoted 8 lines · Jeff King
Quoted 6 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
When writing a commit-graph, we show progress along several commit walks. When we use
start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.
Quoted 7 lines
Quoted 4 lines
However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.
This part of the patch is a good thing, and obviously correct. But I wondered...
Agreed.
Quoted 7 lines
Quoted 4 lines
The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there is one test in
t6500-gc.shthat uses thetest_terminalmethod. This mechanism does not preserve theGIT_PROGRESS_DELAYenvironment variable,Why doesn't
GIT_PROGRESS_DELAYmake it through? Overall it's not that big a deal to me if it doesn't, but in this test:
But I was wondering this, too. If I run the following test:
(
write_script script <<-\EOF &&
echo "GPD: $GIT_PROGRESS_DELAY"
EOF
GIT_PROGRESS_DELAY=42 &&
export GIT_PROGRESS_DELAY &&
test_terminal ./script dummy-arg
) &&then its output looks like this:
+ write_script script
+ GIT_PROGRESS_DELAY=42
+ export GIT_PROGRESS_DELAY
+ test_terminal ./script dummy-arg
GPD: 42
So test_terminal does preserve GIT_PROGRESS_DELAY.
Re: [PATCH v4 1/2] progress: create GIT_PROGRESS_DELAY
In reply to Derrick Stolee via GitGitGadgetReview static struct progress *start_progress_delay(const char *title, uint64_t total, · line 279
Quoted 13 lines · Derrick Stolee via GitGitGadget
(quoted diff) +9
-0@@ -267,9 +268,19 @@ static struct progress *start_progress_delay(const char *title, uint64_t total, return progress; } +static int get_default_delay(void) +{ + static int delay_in_secs = -1; + + if (delay_in_secs < 0) + delay_in_secs = git_env_ulong("GIT_PROGRESS_DELAY", 2); + + return delay_in_secs; +}
Thanks, this factoring out looks good.
Since the only callers of start_progress_delay() pass in either the result of this function or "0", it _could_ become a bool flag and we could just resolve it inside that function. But I don't think there's a big advantage to doing so.
t/t5318-commit-graph.sh test_expect_success 'commit-graph write progress off for redirected stderr' ' · line 137
Quoted 12 lines
t/t5318-commit-graph.sh +1
-1@@ -132,7 +132,7 @@ test_expect_success 'commit-graph write progress off for redirected stderr' ' test_expect_success 'commit-graph write force progress on for stderr' ' cd "$TRASH_DIRECTORY/full" && - git commit-graph write --progress 2>err && + GIT_PROGRESS_DELAY=0 git commit-graph write --progress 2>err && test_file_not_empty err '
I'm tempted to suggest that we should just set GIT_PROGRESS_DELAY=0 for the whole test suite. That would root out any potentially racy tests, though given that the default is 2 seconds, it would probably take a pretty horribly loaded system to trigger such a race.
Curiously, doing this:
t/test-lib.sh +3 -0
@@ -423,6 +423,9 @@ export EDITOR GIT_TRACE_BARE=1 export GIT_TRACE_BARE +GIT_PROGRESS_DELAY=0 +export GIT_PROGRESS_DELAY + check_var_migration () { # the warnings and hints given from this helper depends # on end-user settings, which will disrupt the self-test
results in a few test failures. It looks like unpack-trees is eager to print the "Updating files" progress meter even when stderr is redirected to a file. Which seems like a bug. I don't mind if we put that off for now, though, in order to get your fix here merged more quickly.
-Peff
Quoted 7 lines · Derrick Stolee via GitGitGadget
The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there is one test in
t6500-gc.shthat uses thetest_terminalmethod. This mechanism does not preserve theGIT_PROGRESS_DELAYenvironment variable, so we need to modify check on the output. We still watch for the "Enumerating objects" progress but no longer look for "Computing commit graph generation numbers".
I'm still puzzled by this paragraph. If I replace the test hunk in your patch with this:
t/t6500-gc.sh +2 -1
@@ -109,7 +109,8 @@ test_expect_success 'gc --no-quiet' ' ' test_expect_success TTY 'with TTY: gc --no-quiet' ' - test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && + test_terminal env GIT_PROGRESS_DELAY=0 \ + git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && test_i18ngrep "Enumerating objects" stderr && test_i18ngrep "Computing commit graph generation numbers" stderr
the test works fine for me.
-Peff
Quoted 28 lines · Jeff King
Quoted 7 lines · Derrick Stolee via GitGitGadget
The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there is one test in
t6500-gc.shthat uses thetest_terminalmethod. This mechanism does not preserve theGIT_PROGRESS_DELAYenvironment variable, so we need to modify check on the output. We still watch for the "Enumerating objects" progress but no longer look for "Computing commit graph generation numbers".I'm still puzzled by this paragraph. If I replace the test hunk in your patch with this:
t/t6500-gc.sh +2
-1@@ -109,7 +109,8 @@ test_expect_success 'gc --no-quiet' ' ' test_expect_success TTY 'with TTY: gc --no-quiet' ' - test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && + test_terminal env GIT_PROGRESS_DELAY=0 \ + git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && test_i18ngrep "Enumerating objects" stderr && test_i18ngrep "Computing commit graph generation numbers" stderr
Quoted 1 line
the test works fine for me.
Thanks! I was having trouble getting that to work by only inserting "GIT_PROCESS_DELAY=0" somewhere. The added "env" is what I was missing.
-Stolee
v5 cover commit-graph: use start_delayed_progress()
In reply to Derrick Stolee via GitGitGadgetThanks, Ryenus, for reporting this problem.
Update in V3:
Based on our discussion, I've added the suggested GIT_PROGRESS_DELAY environment variable. This allowed the existing tests to stick around with one exception in the GC tests. The test remains, but we can no longer look at the commit-graph output.
Update in V4:
The GIT_PROGRESS_DELAY check is extracted to a helper method so it can be used with start_delayed_sparse_progress().
Update in V5:
I took Peff's advice for using "env" to use this delay in the GC test.
Derrick Stolee (2):
progress: create GIT_PROGRESS_DELAY
commit-graph: use start_delayed_progress()
Documentation/git.txt | 4 ++++ commit-graph.c | 2 +- progress.c | 15 +++++++++++++-- t/t5318-commit-graph.sh | 4 ++-- t/t6500-gc.sh | 6 +++--- 5 files changed, 23 insertions(+), 8 deletions(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-450%2Fderrickstolee%2Fcommit-graph-progress-fix-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-450/derrickstolee/commit-graph-progress-fix-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/450
Range-diff vs v4:
1: a7acdf9c8f = 1: a7acdf9c8f progress: create GIT_PROGRESS_DELAY 2: e62dcc1ce5 ! 2: efe1419dc6 commit-graph: use start_delayed_progress()
@@ -12,13 +12,9 @@
commits are being added.
The tests that check for the progress output have already been updated
- to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there
- is one test in t6500-gc.sh that uses the test_terminal method. This
- mechanism does not preserve the GIT_PROGRESS_DELAY environment variable,
- so we need to modify check on the output. We still watch for the
- "Enumerating objects" progress but no longer look for "Computing
- commit graph generation numbers".
+ to use GIT_PROGRESS_DELAY=0 to force the expected output.
+ Helped-by: Jeff King <[email protected]>
@@ -39,12 +35,12 @@
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@
- test_expect_success TTY 'with TTY: gc --no-quiet' '
- test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
- test_must_be_empty stdout &&
-- test_i18ngrep "Enumerating objects" stderr &&
-- test_i18ngrep "Computing commit graph generation numbers" stderr
-+ test_i18ngrep "Enumerating objects" stderr
'
- test_expect_success 'gc --quiet' '
+ test_expect_success TTY 'with TTY: gc --no-quiet' '
+- test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
++ test_terminal env GIT_PROGRESS_DELAY=0 \
++ git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
+ test_must_be_empty stdout &&
+ test_i18ngrep "Enumerating objects" stderr &&
+ test_i18ngrep "Computing commit graph generation numbers" stderr
Signature · 1 line
gitgitgadget
PATCH v5 1/2 progress: create GIT_PROGRESS_DELAY
From: Derrick Stolee <[email protected]>
The start_delayed_progress() method is a preferred way to show optional progress to users as it ignores steps that take less than two seconds. However, this makes testing unreliable as tests expect to be very fast.
In addition, users may want to decrease or increase this time interval depending on their preferences for terminal noise.
Create the GIT_PROGRESS_DELAY environment variable to control the delay set during start_delayed_progress(). Set the value in some tests to guarantee their output remains consistent.
--- Documentation/git.txt | 4 ++++ progress.c | 15 +++++++++++++-- t/t5318-commit-graph.sh | 4 ++-- t/t6500-gc.sh | 3 +-- 4 files changed, 20 insertions(+), 6 deletions(-)
Documentation/git.txt +4 -0
@@ -544,6 +544,10 @@ other a pager. See also the `core.pager` option in linkgit:git-config[1]. +`GIT_PROGRESS_DELAY`:: + A number controlling how many seconds to delay before showing + optional progress indicators. Defaults to 2. + `GIT_EDITOR`:: This environment variable overrides `$EDITOR` and `$VISUAL`. It is used by several Git commands when, on interactive mode,
progress.c +13 -2
@@ -14,6 +14,7 @@#include "strbuf.h" #include "trace.h" #include "utf8.h" +#include "config.h" #define TP_IDX_MAX 8
@@ -267,9 +268,19 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,return progress; } +static int get_default_delay (void ) +{ +static int delay_in_secs =-1 ; + +if (delay_in_secs <0 ) + delay_in_secs =git_env_ulong ("GIT_PROGRESS_DELAY" ,2 ); + +return delay_in_secs; +} +struct progress *start_delayed_progress (const char *title,uint64_t total) { -return start_progress_delay (title, total,2 ,0 ); +return start_progress_delay (title, total,get_default_delay (),0 ); }struct progress *start_progress (const char *title,uint64_t total)
@@ -294,7 +305,7 @@ struct progress *start_sparse_progress(const char *title, uint64_t total)struct progress *start_delayed_sparse_progress (const char *title,uint64_t total) { -return start_progress_delay (title, total,2 ,1 ); +return start_progress_delay (title, total,get_default_delay (),1 ); }static void finish_if_sparse (struct progress *progress)
t/t5318-commit-graph.sh +2 -2
@@ -132,7 +132,7 @@ test_expect_success 'commit-graph write progress off for redirected stderr' ' test_expect_success 'commit-graph write force progress on for stderr' ' cd "$TRASH_DIRECTORY/full" && - git commit-graph write --progress 2>err && + GIT_PROGRESS_DELAY=0 git commit-graph write --progress 2>err && test_file_not_empty err '
@@ -150,7 +150,7 @@ test_expect_success 'commit-graph verify progress off for redirected stderr' ' test_expect_success 'commit-graph verify force progress on for stderr' ' cd "$TRASH_DIRECTORY/full" && - git commit-graph verify --progress 2>err && + GIT_PROGRESS_DELAY=0 git commit-graph verify --progress 2>err && test_file_not_empty err '
t/t6500-gc.sh +1 -2
@@ -103,9 +103,8 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre ' test_expect_success 'gc --no-quiet' ' - git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && + GIT_PROGRESS_DELAY=0 git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && - test_line_count = 1 stderr && test_i18ngrep "Computing commit graph generation numbers" stderr '
Signature · 1 line
gitgitgadget
PATCH v5 2/2 commit-graph: use start_delayed_progress()
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.
However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.
The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output.
--- commit-graph.c | 2 +- t/t6500-gc.sh | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)
commit-graph.c +1 -1
@@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)struct commit_list *list =NULL ;if (ctx->report_progress) - ctx->progress =start_progress ( + ctx->progress =start_delayed_progress (_ ("Computing commit graph generation numbers" ), ctx->commits.nr);for (i =0 ; i < ctx->commits.nr; i++) {
t/t6500-gc.sh +2 -1
@@ -109,7 +109,8 @@ test_expect_success 'gc --no-quiet' ' ' test_expect_success TTY 'with TTY: gc --no-quiet' ' - test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && + test_terminal env GIT_PROGRESS_DELAY=0 \ + git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && test_i18ngrep "Enumerating objects" stderr && test_i18ngrep "Computing commit graph generation numbers" stderr
Signature · 1 line
gitgitgadget
Quoted 3 lines · Derrick Stolee via GitGitGadget
Update in V5:
I took Peff's advice for using "env" to use this delay in the GC test.
Thanks. No further complaints from me. :)
There's an open issue to investigate unpack-trees sending progress to a non-stderr tty (which you can see in the test suite by setting GIT_PROGRESS_DELAY=0), but I think that should be handled separately. My limited digging suggests that it goes back to 2007 or earlier.
-Peff
Quoted 4 lines · Jeff King
Quoted 2 lines · Derrick Stolee via GitGitGadget
Update in V5:
Quoted 8 lines
Quoted 1 line
I took Peff's advice for using "env" to use this delay in the GC test.
Thanks. No further complaints from me. :)
There's an open issue to investigate unpack-trees sending progress to a non-stderr tty (which you can see in the test suite by setting GIT_PROGRESS_DELAY=0), but I think that should be handled separately. My limited digging suggests that it goes back to 2007 or earlier.
This is on my personal to-do list. Thanks!
-Stolee
Quoted 7 lines · Derrick Stolee
Quoted 6 lines · Jeff King
Quoted 3 lines · Derrick Stolee via GitGitGadget
Update in V5:
I took Peff's advice for using "env" to use this delay in the GC test.
Quoted 8 lines
Quoted 6 lines
Thanks. No further complaints from me. :)
There's an open issue to investigate unpack-trees sending progress to a non-stderr tty (which you can see in the test suite by setting GIT_PROGRESS_DELAY=0), but I think that should be handled separately. My limited digging suggests that it goes back to 2007 or earlier.
This is on my personal to-do list. Thanks!
Thanks, all. It seems that this round is ready for 'next'?
Quoted 5 lines · Junio C Hamano
Quoted 3 lines · Derrick Stolee
Quoted 2 lines · Jeff King
On Mon, Nov 25, 2019 at 09:28:21PM +0000, Derrick Stolee via GitGitGadget wrote:
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Update in V5:
I took Peff's advice for using "env" to use this delay in the GC test.
Thanks. No further complaints from me. :)
Thanks, all. It seems that this round is ready for 'next'?
Yes, I think it is.