| Message ID | 656dba5afb818d0caa7616d0e58c9728803f8d04.1573148818.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: use start_delayed_progress() | expand |
On Thu, Nov 07, 2019 at 05:46:57PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > 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. Thanks for wrapping this up. I obviously think this is a good direction to go. :) A few thoughts: > diff --git a/Documentation/git.txt b/Documentation/git.txt > index 9b82564d1a..1c420da208 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -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. > @@ -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
On Thu, Nov 07, 2019 at 05:46:57PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > 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. > > Helped-by: Jeff King <peff@peff.net> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > 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(-) > > diff --git a/Documentation/git.txt b/Documentation/git.txt > index 9b82564d1a..1c420da208 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -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, > diff --git a/progress.c b/progress.c > index 0063559aab..4ad1a3c6eb 100644 > --- a/progress.c > +++ b/progress.c > @@ -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); > } Note that there is the similar start_delayed_sparse_progress() function, which should respect GIT_PROGRESS_DELAY as well. > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh > index c0f04dc6b0..7f79eedd1c 100755 > --- a/t/t6500-gc.sh > +++ b/t/t6500-gc.sh > @@ -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. > test_i18ngrep "Computing commit graph generation numbers" stderr > ' > > -- > gitgitgadget >
diff --git a/Documentation/git.txt b/Documentation/git.txt index 9b82564d1a..1c420da208 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -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, diff --git a/progress.c b/progress.c index 0063559aab..4ad1a3c6eb 100644 --- a/progress.c +++ b/progress.c @@ -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) diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index d42b3efe39..0824857e1f 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -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 ' diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index c0f04dc6b0..7f79eedd1c 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -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 '