Re: [PATCH v4 1/2] progress: create GIT_PROGRESS_DELAY

[Jeff King <peff@peff.net>]

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