Re: [PATCH v4 1/2] progress: create GIT_PROGRESS_DELAY
- From
Jeff King <peff@peff.net>
- Date
- Nov 22, 2019, 07:15 UTC
- Message-ID
- <20191122071536.GC23225@sigill.intra.peff.net>
- In-Reply-To
- <a7acdf9c8f8c85f9f39750315716f21e83ce67c6.1574351516.git.gitgitgadget@gmail.com>
On Thu, Nov 21, 2019 at 03:51:55PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 13 quoted lines
> @@ -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.
Show 12 quoted lines
> 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