Re: [PATCH v4 1/2] progress: create GIT_PROGRESS_DELAY
From: Jeff King <hidden>
Date: 2019-11-22 07:15:39
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Thu, Nov 21, 2019 at 03:51:55PM +0000, Derrick Stolee via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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