threads / patch / 52193

patchcommit-graph: use start_delayed_progress()

Subject: [PATCH 0/1] commit-graph: use start_delayed_progress()

## tl;dr

32 messages between Nov 5, 2019 and Dec 1, 2019. Diffs are folded; open one to read it.

replies: 31people: 4as markdown or json

Derrick Stolee via GitGitGadget· Nov 5, 2019, 16:05 UTC · lore
Thanks, Ryenus, for reporting this problem.
Derrick Stolee (1):
  commit-graph: use start_delayed_progress()
 commit-graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-450%2Fderrickstolee%2Fcommit-graph-progress-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-450/derrickstolee/commit-graph-progress-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/450
-- 
gitgitgadget
Derrick Stolee via GitGitGadget· Nov 5, 2019, 16:05 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 1/1] commit-graph: use start_delayed_progress()

From: Derrick Stolee <dstolee@microsoft.com>

When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.

However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.

Reported-by: ryenus <ryenus@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit-graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to commit-graph.c +1 −1
diff --git a/commit-graph.c b/commit-graph.c
index 0aea7b2ae5..071e1c6e9b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
 	struct commit_list *list = NULL;
 
 	if (ctx->report_progress)
-		ctx->progress = start_progress(
+		ctx->progress = start_delayed_progress(
 					_("Computing commit graph generation numbers"),
 					ctx->commits.nr);
 	for (i = 0; i < ctx->commits.nr; i++) {
-- 
gitgitgadget
Derrick Stolee· Nov 5, 2019, 17:38 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 1/1] commit-graph: use start_delayed_progress()

On 11/5/2019 11:05 AM, Derrick Stolee via GitGitGadget wrote:
Show 10 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> When writing a commit-graph, we show progress along several commit
> walks. When we use start_delayed_progress(), the progress line will
> only appear if that step takes a decent amount of time.
> 
> However, one place was missed: computing generation numbers. This is
> normally a very fast operation as all commits have been parsed in a
> previous step. But, this is showing up for all users no matter how few
> commits are being added.

For course, now that we do not force at least one progress line to show up, the tests that check the `--progress` option (or `--no-quiet` for GC) will fail with this patch.

v2 coming soon.
-Stolee
Derrick Stolee via GitGitGadget· Nov 5, 2019, 20:14 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 0/1] commit-graph: use start_delayed_progress()

Thanks, Ryenus, for reporting this problem.
Derrick Stolee (1):
  commit-graph: use start_delayed_progress()
 commit-graph.c          |  2 +-
 t/t5318-commit-graph.sh |  6 ------
 t/t6500-gc.sh           | 14 --------------
 3 files changed, 1 insertion(+), 21 deletions(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-450%2Fderrickstolee%2Fcommit-graph-progress-fix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-450/derrickstolee/commit-graph-progress-fix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/450
Range-diff vs v1:
 1:  174c05bf29 ! 1:  78bd6bc2c0 commit-graph: use start_delayed_progress()
     @@ -11,6 +11,10 @@
          previous step. But, this is showing up for all users no matter how few
          commits are being added.
      
     +    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.
     +
          Reported-by: ryenus <ryenus@gmail.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
     @@ -26,3 +30,45 @@
       					_("Computing commit graph generation numbers"),
       					ctx->commits.nr);
       	for (i = 0; i < ctx->commits.nr; i++) {
     +
     + diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
     + --- a/t/t5318-commit-graph.sh
     + +++ b/t/t5318-commit-graph.sh
     +@@
     + 	test_line_count = 0 err
     + '
     + 
     +-test_expect_success 'commit-graph write force progress on for stderr' '
     +-	cd "$TRASH_DIRECTORY/full" &&
     +-	git commit-graph write --progress 2>err &&
     +-	test_file_not_empty err
     +-'
     +-
     + test_expect_success 'commit-graph write with the --no-progress option' '
     + 	cd "$TRASH_DIRECTORY/full" &&
     + 	git commit-graph write --no-progress 2>err &&
     +
     + diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
     + --- a/t/t6500-gc.sh
     + +++ b/t/t6500-gc.sh
     +@@
     + 	test_line_count = 2 new # There is one new pack and its .idx
     + '
     + 
     +-test_expect_success 'gc --no-quiet' '
     +-	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
     +-'
     +-
     +-test_expect_success TTY 'with TTY: gc --no-quiet' '
     +-	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
     +-	test_must_be_empty stdout &&
     +-	test_i18ngrep "Enumerating objects" stderr &&
     +-	test_i18ngrep "Computing commit graph generation numbers" stderr
     +-'
     +-
     + test_expect_success 'gc --quiet' '
     + 	git -c gc.writeCommitGraph=true gc --quiet >stdout 2>stderr &&
     + 	test_must_be_empty stdout &&
-- 
gitgitgadget
Derrick Stolee via GitGitGadget· Nov 5, 2019, 20:14 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 1/1] commit-graph: use start_delayed_progress()

From: Derrick Stolee <dstolee@microsoft.com>

When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.

However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.

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.

Reported-by: ryenus <ryenus@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit-graph.c          |  2 +-
 t/t5318-commit-graph.sh |  6 ------
 t/t6500-gc.sh           | 14 --------------
 3 files changed, 1 insertion(+), 21 deletions(-)
Show changes to 3 files +1 −21

commit-graph.c, t/t5318-commit-graph.sh, t/t6500-gc.sh

diff --git a/commit-graph.c b/commit-graph.c
index 0aea7b2ae5..071e1c6e9b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
 	struct commit_list *list = NULL;
 
 	if (ctx->report_progress)
-		ctx->progress = start_progress(
+		ctx->progress = start_delayed_progress(
 					_("Computing commit graph generation numbers"),
 					ctx->commits.nr);
 	for (i = 0; i < ctx->commits.nr; i++) {
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index d42b3efe39..8759ff0f3a 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -130,12 +130,6 @@ test_expect_success 'commit-graph write progress off for redirected stderr' '
 	test_line_count = 0 err
 '
 
-test_expect_success 'commit-graph write force progress on for stderr' '
-	cd "$TRASH_DIRECTORY/full" &&
-	git commit-graph write --progress 2>err &&
-	test_file_not_empty err
-'
-
 test_expect_success 'commit-graph write with the --no-progress option' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph write --no-progress 2>err &&
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index c0f04dc6b0..23faa5345f 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -102,20 +102,6 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre
 	test_line_count = 2 new # There is one new pack and its .idx
 '
 
-test_expect_success 'gc --no-quiet' '
-	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
-'
-
-test_expect_success TTY 'with TTY: gc --no-quiet' '
-	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
-	test_must_be_empty stdout &&
-	test_i18ngrep "Enumerating objects" stderr &&
-	test_i18ngrep "Computing commit graph generation numbers" stderr
-'
-
 test_expect_success 'gc --quiet' '
 	git -c gc.writeCommitGraph=true gc --quiet >stdout 2>stderr &&
 	test_must_be_empty stdout &&
-- 
gitgitgadget
Jeff King· Nov 6, 2019, 04:09 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH v2 1/1] commit-graph: use start_delayed_progress()

On Tue, Nov 05, 2019 at 08:14:02PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 10 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> When writing a commit-graph, we show progress along several commit
> walks. When we use start_delayed_progress(), the progress line will
> only appear if that step takes a decent amount of time.
> 
> However, one place was missed: computing generation numbers. This is
> normally a very fast operation as all commits have been parsed in a
> previous step. But, this is showing up for all users no matter how few
> commits are being added.

Yep, makes sense (especially that it should all the other progress as part of the same process).

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

-Peff
Derrick Stolee· Nov 6, 2019, 13:21 UTC · re: Jeff King · lore

Re: [PATCH v2 1/1] commit-graph: use start_delayed_progress()

On 11/5/2019 11:09 PM, Jeff King wrote:
Show 24 quoted lines
> On Tue, Nov 05, 2019 at 08:14:02PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> When writing a commit-graph, we show progress along several commit
>> walks. When we use start_delayed_progress(), the progress line will
>> only appear if that step takes a decent amount of time.
>>
>> However, one place was missed: computing generation numbers. This is
>> normally a very fast operation as all commits have been parsed in a
>> previous step. But, this is showing up for all users no matter how few
>> commits are being added.
> 
> Yep, makes sense (especially that it should all the other progress as
> part of the same process).
> 
>> 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.

The impact of doing that inside the progress code could be large, so perhaps that is best left for a follow-up.

Thanks, -Stolee

Jeff King· Nov 7, 2019, 06:40 UTC · re: Derrick Stolee · lore

Re: [PATCH v2 1/1] commit-graph: use start_delayed_progress()

On Wed, Nov 06, 2019 at 08:21:48AM -0500, Derrick Stolee wrote:
Show 12 quoted lines
> >> 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:

Show changes to progress.c +3 −2
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)


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.

-Peff
Derrick Stolee· Nov 7, 2019, 13:30 UTC · re: Jeff King · lore

Re: [PATCH v2 1/1] commit-graph: use start_delayed_progress()

On 11/7/2019 1:40 AM, Jeff King wrote:
Show 40 quoted lines
> On Wed, Nov 06, 2019 at 08:21:48AM -0500, Derrick Stolee wrote:
> 
>>>> 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
Junio C Hamano· Nov 7, 2019, 04:37 UTC · re: Jeff King · lore

Re: [PATCH v2 1/1] commit-graph: use start_delayed_progress()

Jeff King <peff@peff.net> writes:
> 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 did have the same reaction after seeing the change to 5318 where the expected output from "git commit-graph write --progress" has become unreliable.

I think there are possibly three kinds of folks:
 - I do not want the output smudged with any progress (e.g. I am a
   script);
 - I want to see progress if it takes very long, but do not waste
   vertical screen real estate if it does not make me wait (e.g. I
   am an interactive user who occasionally wants a cue to leave the
   keyboard to grab coffee); and
 - I want to see all progress (... now who am I?  Taking a
   screenshot to write a tutorial or something???).

In the ideal world, the three choices above should probably be "--progress=(no|auto|always)" where not having any defaults to one of them (probably "auto", as the code can use isatty() to further turn it to "no").

Making "--progress" to mean "--progress=always" is OK, but it leaves no way to override an earlier --[no-]progress on the command line, which feels somewhat satisfying.

Thanks.
Jeff King· Nov 7, 2019, 06:43 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 1/1] commit-graph: use start_delayed_progress()

On Thu, Nov 07, 2019 at 01:37:52PM +0900, Junio C Hamano wrote:
Show 23 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > 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 did have the same reaction after seeing the change to 5318 where
> the expected output from "git commit-graph write --progress" has
> become unreliable.
> 
> I think there are possibly three kinds of folks:
> 
>  - I do not want the output smudged with any progress (e.g. I am a
>    script);
> 
>  - I want to see progress if it takes very long, but do not waste
>    vertical screen real estate if it does not make me wait (e.g. I
>    am an interactive user who occasionally wants a cue to leave the
>    keyboard to grab coffee); and
> 
>  - I want to see all progress (... now who am I?  Taking a
>    screenshot to write a tutorial or something???).

I think type 3 may be people who want to understand more about the program flow, and where it's at when it sees an error.

> In the ideal world, the three choices above should probably be
> "--progress=(no|auto|always)" where not having any defaults to one
> of them (probably "auto", as the code can use isatty() to further
> turn it to "no").

I think any no/auto/always here is tricky, because it already has a meaning: to use or disregard isatty(2). And overriding that might be independent of the "type" (think pack-objects on a server generating output that's going over the wire; we have to tell it "yes, definitely show progress even though there is no terminal", but that has nothing to do with any "delay" decisions).

-Peff
Junio C Hamano· Nov 7, 2019, 09:51 UTC · re: Jeff King · lore

Re: [PATCH v2 1/1] commit-graph: use start_delayed_progress()

Jeff King <peff@peff.net> writes:
> I think any no/auto/always here is tricky, because it already has a
> meaning: to use or disregard isatty(2). And overriding that might be
> independent of the "type".

Yes, your "let the users optionally give the delay" in the other subthread makes a lot more sense.

Thanks.
Derrick Stolee via GitGitGadget· Nov 7, 2019, 17:46 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v3 0/2] commit-graph: use start_delayed_progress()

Thanks, Ryenus, for reporting this problem.
Update in V3:

Based on our discussion, I've added the suggested GIT_PROGRESS_DELAY environment variable. This allowed the existing tests to stick around with one exception in the GC tests. The test remains, but we can no longer look at the commit-graph output.

Derrick Stolee (2):
  progress: create GIT_PROGRESS_DELAY
  commit-graph: use start_delayed_progress()
 Documentation/git.txt   | 4 ++++
 commit-graph.c          | 2 +-
 progress.c              | 8 +++++++-
 t/t5318-commit-graph.sh | 4 ++--
 t/t6500-gc.sh           | 6 ++----
 5 files changed, 16 insertions(+), 8 deletions(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-450%2Fderrickstolee%2Fcommit-graph-progress-fix-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-450/derrickstolee/commit-graph-progress-fix-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/450
Range-diff vs v2:
 -:  ---------- > 1:  656dba5afb progress: create GIT_PROGRESS_DELAY
 1:  78bd6bc2c0 ! 2:  3c0c9675e1 commit-graph: use start_delayed_progress()
     @@ -11,9 +11,13 @@
          previous step. But, this is showing up for all users no matter how few
          commits are being added.
      
     -    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.
     +    The tests that check for the progress output have already been updated
     +    to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there
     +    is one test in t6500-gc.sh that uses the test_terminal method. This
     +    mechanism does not preserve the GIT_PROGRESS_DELAY environment variable,
     +    so we need to modify check on the output. We still watch for the
     +    "Enumerating objects" progress but no longer look for "Computing
     +    commit graph generation numbers".
      
          Reported-by: ryenus <ryenus@gmail.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
     @@ -31,44 +35,16 @@
       					ctx->commits.nr);
       	for (i = 0; i < ctx->commits.nr; i++) {
      
     - diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
     - --- a/t/t5318-commit-graph.sh
     - +++ b/t/t5318-commit-graph.sh
     -@@
     - 	test_line_count = 0 err
     - '
     - 
     --test_expect_success 'commit-graph write force progress on for stderr' '
     --	cd "$TRASH_DIRECTORY/full" &&
     --	git commit-graph write --progress 2>err &&
     --	test_file_not_empty err
     --'
     --
     - test_expect_success 'commit-graph write with the --no-progress option' '
     - 	cd "$TRASH_DIRECTORY/full" &&
     - 	git commit-graph write --no-progress 2>err &&
     -
       diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
       --- a/t/t6500-gc.sh
       +++ b/t/t6500-gc.sh
      @@
     - 	test_line_count = 2 new # There is one new pack and its .idx
     - '
     - 
     --test_expect_success 'gc --no-quiet' '
     --	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
     --'
     --
     --test_expect_success TTY 'with TTY: gc --no-quiet' '
     --	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
     --	test_must_be_empty stdout &&
     + test_expect_success TTY 'with TTY: gc --no-quiet' '
     + 	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
     + 	test_must_be_empty stdout &&
      -	test_i18ngrep "Enumerating objects" stderr &&
      -	test_i18ngrep "Computing commit graph generation numbers" stderr
     --'
     --
     ++	test_i18ngrep "Enumerating objects" stderr
     + '
     + 
       test_expect_success 'gc --quiet' '
     - 	git -c gc.writeCommitGraph=true gc --quiet >stdout 2>stderr &&
     - 	test_must_be_empty stdout &&
-- 
gitgitgadget
Derrick Stolee via GitGitGadget· Nov 7, 2019, 17:46 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v3 1/2] progress: create GIT_PROGRESS_DELAY

From: Derrick Stolee <dstolee@microsoft.com>

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 <peff@peff.net>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git.txt   | 4 ++++
 progress.c              | 8 +++++++-
 t/t5318-commit-graph.sh | 4 ++--
 t/t6500-gc.sh           | 3 +--
 4 files changed, 14 insertions(+), 5 deletions(-)
Show changes to 4 files +14 −5

Documentation/git.txt, progress.c, t/t5318-commit-graph.sh, t/t6500-gc.sh

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..4ad1a3c6eb 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,12 @@ 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);
+	static int delay_in_secs = -1;
+
+	if (delay_in_secs < 0)
+		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)
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
 '
 
-- 
gitgitgadget
Jeff King· Nov 7, 2019, 21:22 UTC · re: Derrick Stolee via GitGitGadget · lore

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

On Thu, Nov 07, 2019 at 05:46:57PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 13 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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.

Thanks for wrapping this up. I obviously think this is a good direction to go. :) A few thoughts:

Show 11 quoted lines
> 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.

Not all progress meters use delay. I wonder if that might confuse some users, who would try:

  GIT_PROGRESS_DELAY=10 git repack -ad
or something, but still see "Enumerating objects".

I guess the key word in your documentation is "optional", but maybe it needs to be spelled out more clearly. I dunno.

Show 12 quoted lines
> @@ -269,7 +270,12 @@ 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);
> +	static int delay_in_secs = -1;
> +
> +	if (delay_in_secs < 0)
> +		delay_in_secs = git_env_ulong("GIT_PROGRESS_DELAY", 2);
> +
> +	return start_progress_delay(title, total, delay_in_secs, 0);
>  }

You asked earlier if it was worth memo-izing the git_env_ulong() call like this. I suspect it doesn't matter much either way, since progress only starts and stops a few times in a given program. But I'm certainly happy with it this way, as it matches most other environment lookups.

-Peff
SZEDER Gábor· Nov 11, 2019, 14:27 UTC · re: Derrick Stolee via GitGitGadget · lore

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

On Thu, Nov 07, 2019 at 05:46:57PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 62 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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 <peff@peff.net>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git.txt   | 4 ++++
>  progress.c              | 8 +++++++-
>  t/t5318-commit-graph.sh | 4 ++--
>  t/t6500-gc.sh           | 3 +--
>  4 files changed, 14 insertions(+), 5 deletions(-)
> 
> 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..4ad1a3c6eb 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,12 @@ 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);
> +	static int delay_in_secs = -1;
> +
> +	if (delay_in_secs < 0)
> +		delay_in_secs = git_env_ulong("GIT_PROGRESS_DELAY", 2);
> +
> +	return start_progress_delay(title, total, delay_in_secs, 0);
>  }

Note that there is the similar start_delayed_sparse_progress() function, which should respect GIT_PROGRESS_DELAY as well.

Show 12 quoted lines
> 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 &&

Good, I'm glad to see this "how many progress lines did we print?" check gone.

Show 6 quoted lines
>  	test_i18ngrep "Computing commit graph generation numbers" stderr
>  '
>  
> -- 
> gitgitgadget
> 
Derrick Stolee via GitGitGadget· Nov 7, 2019, 17:46 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v3 2/2] commit-graph: use start_delayed_progress()

From: Derrick Stolee <dstolee@microsoft.com>

When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.

However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.

The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there is one test in t6500-gc.sh that uses the test_terminal method. This mechanism does not preserve the GIT_PROGRESS_DELAY environment variable, so we need to modify check on the output. We still watch for the "Enumerating objects" progress but no longer look for "Computing commit graph generation numbers".

Reported-by: ryenus <ryenus@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit-graph.c | 2 +-
 t/t6500-gc.sh  | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)
Show changes to 2 files +2 −3

commit-graph.c, t/t6500-gc.sh

diff --git a/commit-graph.c b/commit-graph.c
index 0aea7b2ae5..071e1c6e9b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
 	struct commit_list *list = NULL;
 
 	if (ctx->report_progress)
-		ctx->progress = start_progress(
+		ctx->progress = start_delayed_progress(
 					_("Computing commit graph generation numbers"),
 					ctx->commits.nr);
 	for (i = 0; i < ctx->commits.nr; i++) {
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 7f79eedd1c..c68177510b 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -111,8 +111,7 @@ test_expect_success 'gc --no-quiet' '
 test_expect_success TTY 'with TTY: gc --no-quiet' '
 	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
 	test_must_be_empty stdout &&
-	test_i18ngrep "Enumerating objects" stderr &&
-	test_i18ngrep "Computing commit graph generation numbers" stderr
+	test_i18ngrep "Enumerating objects" stderr
 '
 
 test_expect_success 'gc --quiet' '
-- 
gitgitgadget
Jeff King· Nov 7, 2019, 21:26 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH v3 2/2] commit-graph: use start_delayed_progress()

On Thu, Nov 07, 2019 at 05:46:58PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 10 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> When writing a commit-graph, we show progress along several commit
> walks. When we use start_delayed_progress(), the progress line will
> only appear if that step takes a decent amount of time.
> 
> However, one place was missed: computing generation numbers. This is
> normally a very fast operation as all commits have been parsed in a
> previous step. But, this is showing up for all users no matter how few
> commits are being added.

This part of the patch is a good thing, and obviously correct. But I wondered...

> The tests that check for the progress output have already been updated
> to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there
> is one test in t6500-gc.sh that uses the test_terminal method. This
> mechanism does not preserve the GIT_PROGRESS_DELAY environment variable,

Why doesn't GIT_PROGRESS_DELAY make it through? Overall it's not that big a deal to me if it doesn't, but in this test:

Show 7 quoted lines
>  test_expect_success TTY 'with TTY: gc --no-quiet' '
>  	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
>  	test_must_be_empty stdout &&
> -	test_i18ngrep "Enumerating objects" stderr &&
> -	test_i18ngrep "Computing commit graph generation numbers" stderr
> +	test_i18ngrep "Enumerating objects" stderr
>  '

We're not actually checking anything related to gc.writeCommitGraph anymore.

> so we need to modify check on the output. We still watch for the
Minor typo: s/modify/& the/ or similar?
-Peff
SZEDER Gábor· Nov 21, 2019, 23:03 UTC · re: Jeff King · lore

Re: [PATCH v3 2/2] commit-graph: use start_delayed_progress()

On Thu, Nov 07, 2019 at 04:26:14PM -0500, Jeff King wrote:
Show 15 quoted lines
> On Thu, Nov 07, 2019 at 05:46:58PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
> > From: Derrick Stolee <dstolee@microsoft.com>
> > 
> > When writing a commit-graph, we show progress along several commit
> > walks. When we use start_delayed_progress(), the progress line will
> > only appear if that step takes a decent amount of time.
> > 
> > However, one place was missed: computing generation numbers. This is
> > normally a very fast operation as all commits have been parsed in a
> > previous step. But, this is showing up for all users no matter how few
> > commits are being added.
> 
> This part of the patch is a good thing, and obviously correct. But I
> wondered...
Agreed.
Show 7 quoted lines
> > The tests that check for the progress output have already been updated
> > to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there
> > is one test in t6500-gc.sh that uses the test_terminal method. This
> > mechanism does not preserve the GIT_PROGRESS_DELAY environment variable,
> 
> Why doesn't GIT_PROGRESS_DELAY make it through? Overall it's not that
> big a deal to me if it doesn't, but in this test:
But I was wondering this, too.  If I run the following test:
        (
                write_script script <<-\EOF &&
                echo "GPD: $GIT_PROGRESS_DELAY"
                EOF
                GIT_PROGRESS_DELAY=42 &&
                export GIT_PROGRESS_DELAY &&
                test_terminal ./script dummy-arg
        ) &&
then its output looks like this:
  + write_script script
  + GIT_PROGRESS_DELAY=42
  + export GIT_PROGRESS_DELAY
  + test_terminal ./script dummy-arg
  GPD: 42
So test_terminal does preserve GIT_PROGRESS_DELAY.
Derrick Stolee via GitGitGadget· Nov 21, 2019, 15:51 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v4 0/2] commit-graph: use start_delayed_progress()

Thanks, Ryenus, for reporting this problem.
Update in V3:

Based on our discussion, I've added the suggested GIT_PROGRESS_DELAY environment variable. This allowed the existing tests to stick around with one exception in the GC tests. The test remains, but we can no longer look at the commit-graph output.

Update in V4:

The GIT_PROGRESS_DELAY check is extracted to a helper method so it can be used with start_delayed_sparse_progress().

Derrick Stolee (2):
  progress: create GIT_PROGRESS_DELAY
  commit-graph: use start_delayed_progress()
 Documentation/git.txt   |  4 ++++
 commit-graph.c          |  2 +-
 progress.c              | 15 +++++++++++++--
 t/t5318-commit-graph.sh |  4 ++--
 t/t6500-gc.sh           |  6 ++----
 5 files changed, 22 insertions(+), 9 deletions(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-450%2Fderrickstolee%2Fcommit-graph-progress-fix-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-450/derrickstolee/commit-graph-progress-fix-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/450
Range-diff vs v3:
 1:  656dba5afb ! 1:  a7acdf9c8f progress: create GIT_PROGRESS_DELAY
     @@ -44,19 +44,35 @@
       #define TP_IDX_MAX      8
       
      @@
     + 	return progress;
     + }
       
     - struct progress *start_delayed_progress(const char *title, uint64_t total)
     - {
     --	return start_progress_delay(title, total, 2, 0);
     ++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 start_progress_delay(title, total, delay_in_secs, 0);
     ++	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)
     +@@
     + 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
       --- a/t/t5318-commit-graph.sh
 2:  3c0c9675e1 = 2:  e62dcc1ce5 commit-graph: use start_delayed_progress()
-- 
gitgitgadget
Derrick Stolee via GitGitGadget· Nov 21, 2019, 15:51 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v4 2/2] commit-graph: use start_delayed_progress()

From: Derrick Stolee <dstolee@microsoft.com>

When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.

However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.

The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there is one test in t6500-gc.sh that uses the test_terminal method. This mechanism does not preserve the GIT_PROGRESS_DELAY environment variable, so we need to modify check on the output. We still watch for the "Enumerating objects" progress but no longer look for "Computing commit graph generation numbers".

Reported-by: ryenus <ryenus@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit-graph.c | 2 +-
 t/t6500-gc.sh  | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)
Show changes to 2 files +2 −3

commit-graph.c, t/t6500-gc.sh

diff --git a/commit-graph.c b/commit-graph.c
index 0aea7b2ae5..071e1c6e9b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
 	struct commit_list *list = NULL;
 
 	if (ctx->report_progress)
-		ctx->progress = start_progress(
+		ctx->progress = start_delayed_progress(
 					_("Computing commit graph generation numbers"),
 					ctx->commits.nr);
 	for (i = 0; i < ctx->commits.nr; i++) {
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 7f79eedd1c..c68177510b 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -111,8 +111,7 @@ test_expect_success 'gc --no-quiet' '
 test_expect_success TTY 'with TTY: gc --no-quiet' '
 	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
 	test_must_be_empty stdout &&
-	test_i18ngrep "Enumerating objects" stderr &&
-	test_i18ngrep "Computing commit graph generation numbers" stderr
+	test_i18ngrep "Enumerating objects" stderr
 '
 
 test_expect_success 'gc --quiet' '
-- 
gitgitgadget
Jeff King· Nov 22, 2019, 07:17 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH v4 2/2] commit-graph: use start_delayed_progress()

On Thu, Nov 21, 2019 at 03:51:56PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 7 quoted lines
> The tests that check for the progress output have already been updated
> to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there
> is one test in t6500-gc.sh that uses the test_terminal method. This
> mechanism does not preserve the GIT_PROGRESS_DELAY environment variable,
> so we need to modify check on the output. We still watch for the
> "Enumerating objects" progress but no longer look for "Computing
> commit graph generation numbers".

I'm still puzzled by this paragraph. If I replace the test hunk in your patch with this:

Show changes to t/t6500-gc.sh +2 −2
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 7f79eedd1c..0a69a67117 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -109,7 +109,8 @@ test_expect_success 'gc --no-quiet' '
 '
 
 test_expect_success TTY 'with TTY: gc --no-quiet' '
-	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
+	test_terminal env GIT_PROGRESS_DELAY=0 \
+		git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
 	test_must_be_empty stdout &&
 	test_i18ngrep "Enumerating objects" stderr &&
 	test_i18ngrep "Computing commit graph generation numbers" stderr

the test works fine for me.

-Peff
Derrick Stolee· Nov 25, 2019, 18:57 UTC · re: Jeff King · lore

Re: [PATCH v4 2/2] commit-graph: use start_delayed_progress()

On 11/22/2019 2:17 AM, Jeff King wrote:
Show 29 quoted lines
> On Thu, Nov 21, 2019 at 03:51:56PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
>> The tests that check for the progress output have already been updated
>> to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there
>> is one test in t6500-gc.sh that uses the test_terminal method. This
>> mechanism does not preserve the GIT_PROGRESS_DELAY environment variable,
>> so we need to modify check on the output. We still watch for the
>> "Enumerating objects" progress but no longer look for "Computing
>> commit graph generation numbers".
> 
> I'm still puzzled by this paragraph. If I replace the test hunk in your
> patch with this:
> 
> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index 7f79eedd1c..0a69a67117 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -109,7 +109,8 @@ test_expect_success 'gc --no-quiet' '
>  '
>  
>  test_expect_success TTY 'with TTY: gc --no-quiet' '
> -	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
> +	test_terminal env GIT_PROGRESS_DELAY=0 \
> +		git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
>  	test_must_be_empty stdout &&
>  	test_i18ngrep "Enumerating objects" stderr &&
>  	test_i18ngrep "Computing commit graph generation numbers" stderr
> 
> the test works fine for me.

Thanks! I was having trouble getting that to work by only inserting "GIT_PROCESS_DELAY=0" somewhere. The added "env" is what I was missing.

-Stolee
Derrick Stolee via GitGitGadget· Nov 21, 2019, 15:51 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v4 1/2] progress: create GIT_PROGRESS_DELAY

From: Derrick Stolee <dstolee@microsoft.com>

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 <peff@peff.net>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 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(-)
Show changes to 4 files +20 −6

Documentation/git.txt, progress.c, t/t5318-commit-graph.sh, t/t6500-gc.sh

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
 '
 
-- 
gitgitgadget
Jeff King· Nov 22, 2019, 07:15 UTC · re: Derrick Stolee via GitGitGadget · lore

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

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:
Show changes to t/test-lib.sh +3 −1
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
Derrick Stolee via GitGitGadget· Nov 25, 2019, 21:28 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v5 0/2] commit-graph: use start_delayed_progress()

Thanks, Ryenus, for reporting this problem.
Update in V3:

Based on our discussion, I've added the suggested GIT_PROGRESS_DELAY environment variable. This allowed the existing tests to stick around with one exception in the GC tests. The test remains, but we can no longer look at the commit-graph output.

Update in V4:

The GIT_PROGRESS_DELAY check is extracted to a helper method so it can be used with start_delayed_sparse_progress().

Update in V5:
I took Peff's advice for using "env" to use this delay in the GC test.
Derrick Stolee (2):
  progress: create GIT_PROGRESS_DELAY
  commit-graph: use start_delayed_progress()
 Documentation/git.txt   |  4 ++++
 commit-graph.c          |  2 +-
 progress.c              | 15 +++++++++++++--
 t/t5318-commit-graph.sh |  4 ++--
 t/t6500-gc.sh           |  6 +++---
 5 files changed, 23 insertions(+), 8 deletions(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-450%2Fderrickstolee%2Fcommit-graph-progress-fix-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-450/derrickstolee/commit-graph-progress-fix-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/450
Range-diff vs v4:
 1:  a7acdf9c8f = 1:  a7acdf9c8f progress: create GIT_PROGRESS_DELAY
 2:  e62dcc1ce5 ! 2:  efe1419dc6 commit-graph: use start_delayed_progress()
     @@ -12,13 +12,9 @@
          commits are being added.
      
          The tests that check for the progress output have already been updated
     -    to use GIT_PROGRESS_DELAY=0 to force the expected output. However, there
     -    is one test in t6500-gc.sh that uses the test_terminal method. This
     -    mechanism does not preserve the GIT_PROGRESS_DELAY environment variable,
     -    so we need to modify check on the output. We still watch for the
     -    "Enumerating objects" progress but no longer look for "Computing
     -    commit graph generation numbers".
     +    to use GIT_PROGRESS_DELAY=0 to force the expected output.
      
     +    Helped-by: Jeff King <peff@peff.net>
          Reported-by: ryenus <ryenus@gmail.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
     @@ -39,12 +35,12 @@
       --- a/t/t6500-gc.sh
       +++ b/t/t6500-gc.sh
      @@
     - test_expect_success TTY 'with TTY: gc --no-quiet' '
     - 	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
     - 	test_must_be_empty stdout &&
     --	test_i18ngrep "Enumerating objects" stderr &&
     --	test_i18ngrep "Computing commit graph generation numbers" stderr
     -+	test_i18ngrep "Enumerating objects" stderr
       '
       
     - test_expect_success 'gc --quiet' '
     + test_expect_success TTY 'with TTY: gc --no-quiet' '
     +-	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
     ++	test_terminal env GIT_PROGRESS_DELAY=0 \
     ++		git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
     + 	test_must_be_empty stdout &&
     + 	test_i18ngrep "Enumerating objects" stderr &&
     + 	test_i18ngrep "Computing commit graph generation numbers" stderr
-- 
gitgitgadget
Derrick Stolee via GitGitGadget· Nov 25, 2019, 21:28 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v5 1/2] progress: create GIT_PROGRESS_DELAY

From: Derrick Stolee <dstolee@microsoft.com>

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 <peff@peff.net>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 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(-)
Show changes to 4 files +20 −6

Documentation/git.txt, progress.c, t/t5318-commit-graph.sh, t/t6500-gc.sh

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
 '
 
-- 
gitgitgadget
Derrick Stolee via GitGitGadget· Nov 25, 2019, 21:28 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v5 2/2] commit-graph: use start_delayed_progress()

From: Derrick Stolee <dstolee@microsoft.com>

When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time.

However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added.

The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output.

Helped-by: Jeff King <peff@peff.net>
Reported-by: ryenus <ryenus@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit-graph.c | 2 +-
 t/t6500-gc.sh  | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)
Show changes to 2 files +3 −2

commit-graph.c, t/t6500-gc.sh

diff --git a/commit-graph.c b/commit-graph.c
index 0aea7b2ae5..071e1c6e9b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
 	struct commit_list *list = NULL;
 
 	if (ctx->report_progress)
-		ctx->progress = start_progress(
+		ctx->progress = start_delayed_progress(
 					_("Computing commit graph generation numbers"),
 					ctx->commits.nr);
 	for (i = 0; i < ctx->commits.nr; i++) {
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 7f79eedd1c..0a69a67117 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -109,7 +109,8 @@ test_expect_success 'gc --no-quiet' '
 '
 
 test_expect_success TTY 'with TTY: gc --no-quiet' '
-	test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
+	test_terminal env GIT_PROGRESS_DELAY=0 \
+		git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
 	test_must_be_empty stdout &&
 	test_i18ngrep "Enumerating objects" stderr &&
 	test_i18ngrep "Computing commit graph generation numbers" stderr
-- 
gitgitgadget
Jeff King· Nov 26, 2019, 12:20 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH v5 0/2] commit-graph: use start_delayed_progress()

On Mon, Nov 25, 2019 at 09:28:21PM +0000, Derrick Stolee via GitGitGadget wrote:
> Update in V5:
> 
> I took Peff's advice for using "env" to use this delay in the GC test.
Thanks. No further complaints from me. :)

There's an open issue to investigate unpack-trees sending progress to a non-stderr tty (which you can see in the test suite by setting GIT_PROGRESS_DELAY=0), but I think that should be handled separately. My limited digging suggests that it goes back to 2007 or earlier.

-Peff
Derrick Stolee· Nov 26, 2019, 15:39 UTC · re: Jeff King · lore

Re: [PATCH v5 0/2] commit-graph: use start_delayed_progress()

On 11/26/2019 7:20 AM, Jeff King wrote:
Show 12 quoted lines
> On Mon, Nov 25, 2019 at 09:28:21PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
>> Update in V5:
>>
>> I took Peff's advice for using "env" to use this delay in the GC test.
> 
> Thanks. No further complaints from me. :)
> 
> There's an open issue to investigate unpack-trees sending progress to a
> non-stderr tty (which you can see in the test suite by setting
> GIT_PROGRESS_DELAY=0), but I think that should be handled separately.
> My limited digging suggests that it goes back to 2007 or earlier.
This is on my personal to-do list. Thanks!
-Stolee
Junio C Hamano· Nov 30, 2019, 14:36 UTC · re: Derrick Stolee · lore

Re: [PATCH v5 0/2] commit-graph: use start_delayed_progress()

Derrick Stolee <stolee@gmail.com> writes:
Show 15 quoted lines
> On 11/26/2019 7:20 AM, Jeff King wrote:
>> On Mon, Nov 25, 2019 at 09:28:21PM +0000, Derrick Stolee via GitGitGadget wrote:
>> 
>>> Update in V5:
>>>
>>> I took Peff's advice for using "env" to use this delay in the GC test.
>> 
>> Thanks. No further complaints from me. :)
>> 
>> There's an open issue to investigate unpack-trees sending progress to a
>> non-stderr tty (which you can see in the test suite by setting
>> GIT_PROGRESS_DELAY=0), but I think that should be handled separately.
>> My limited digging suggests that it goes back to 2007 or earlier.
>
> This is on my personal to-do list. Thanks!
Thanks, all.  It seems that this round is ready for 'next'?
SZEDER Gábor· Dec 1, 2019, 09:33 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 0/2] commit-graph: use start_delayed_progress()

On Sat, Nov 30, 2019 at 06:36:57AM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> Derrick Stolee <stolee@gmail.com> writes:
> 
> > On 11/26/2019 7:20 AM, Jeff King wrote:
> >> On Mon, Nov 25, 2019 at 09:28:21PM +0000, Derrick Stolee via GitGitGadget wrote:
> >> 
> >>> Update in V5:
> >>>
> >>> I took Peff's advice for using "env" to use this delay in the GC test.
> >> 
> >> Thanks. No further complaints from me. :)
> Thanks, all.  It seems that this round is ready for 'next'?
Yes, I think it is.

← back to recent threads