Re: [PATCH v2 1/1] commit-graph: use start_delayed_progress()
From: Derrick Stolee <hidden>
Date: 2019-11-07 13:31:01
On 11/7/2019 1:40 AM, Jeff King wrote:
quoted hunk ↗ jump to hunk
On Wed, Nov 06, 2019 at 08:21:48AM -0500, Derrick Stolee wrote:quoted
quoted
quoted
Now that we changed this method, very fast commands show no progess at all. This means we need to stop testing for seeing these progress lines in the test suite.I think this is OK for now, though it does make me wonder if "--progress" ought to perhaps override "delayed" in some instances, since it's a positive signal from the caller that they're interested in seeing progress.I was thinking that we could start with a GIT_TEST_PROGRESS environment variable to force all delayed progress to act like non-delayed progress. That would at least give us confirmation on these kinds of tests.I think this could actually be a non-test variable. E.g., something like this:diff --git a/progress.c b/progress.c index 0063559aab..74b90e8898 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,8 @@ 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); + int 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)
I like this idea. It allows us to force the progress on in tests, and for users to tweak their preferred delay. That includes _increasing_ the delay if they want to.
which lets you ask for more verbose progress. There are times when I'd use something like this for general debugging. Though these days I might suggest that something like GIT_TRACE2_PERF hook the progress code to output. That's a bit more complicated to implement, though.
Would it make sense to make delay_in_secs a local static variable, so we remember it between calls? That would allow us to check the environment only once (not that it is usually expensive). -Stolee