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

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

[Derrick Stolee via GitGitGadget]

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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Derrick Stolee]

On 11/5/2019 11:05 AM, 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.

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

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

[Derrick Stolee via GitGitGadget]

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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Jeff King]

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.

-Peff

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

[Derrick Stolee]

On 11/5/2019 11:09 PM, Jeff King wrote:
> 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

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

[Jeff King]

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)


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

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

[Derrick Stolee]

On 11/7/2019 1:40 AM, Jeff King wrote:
> 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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Thu, Nov 07, 2019 at 01:37:52PM +0900, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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.

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

[Derrick Stolee via GitGitGadget]

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

[PATCH v3 1/2] progress: create GIT_PROGRESS_DELAY

[Derrick Stolee via GitGitGadget]

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);
 }
 
 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

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

[Jeff King]

On Thu, Nov 07, 2019 at 05:46:57PM +0000, Derrick Stolee via GitGitGadget wrote:

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

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

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

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

[SZEDER Gábor]

On Thu, Nov 07, 2019 at 05:46:57PM +0000, Derrick Stolee via GitGitGadget wrote:
> 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.

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

>  	test_i18ngrep "Computing commit graph generation numbers" stderr
>  '
>  
> -- 
> gitgitgadget
> 

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Jeff King]

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

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

>  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

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

[SZEDER Gábor]

On Thu, Nov 07, 2019 at 04:26:14PM -0500, Jeff King wrote:
> 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.

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

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

[Derrick Stolee via GitGitGadget]

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

[PATCH v4 1/2] progress: create GIT_PROGRESS_DELAY

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Jeff King]

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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Jeff King]

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.

-Peff

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

[Derrick Stolee]

On 11/22/2019 2:17 AM, Jeff King wrote:
> 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

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

[Derrick Stolee via GitGitGadget]

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

[PATCH v5 1/2] progress: create GIT_PROGRESS_DELAY

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Jeff King]

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

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

[Derrick Stolee]

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!

-Stolee

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

[Junio C Hamano]

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. :)
>> 
>> 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'?

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

[SZEDER Gábor]

On Sat, Nov 30, 2019 at 06:36:57AM -0800, Junio C Hamano wrote:
> 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.