[v4,1/2] progress: create GIT_PROGRESS_DELAY

Commit Message

Johannes Schindelin via GitGitGadget Nov. 21, 2019, 3:51 p.m. UTC

From: Derrick Stolee <[e-mail]>

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 <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
 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(-)

Comments

Jeff King Nov. 22, 2019, 7:15 a.m. UTC | #1

On Thu, Nov 21, 2019 at 03:51:55PM +0000, Derrick Stolee via GitGitGadget wrote:

> @@ -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.

> 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
>  '

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:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 46c4440843..63357ed6c4 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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

Patch

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..19805ac646 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
 
@@ -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)
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
 '