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

Commit Message

Johannes Schindelin via GitGitGadget Nov. 21, 2019, 3:51 p.m. UTC

From: Derrick Stolee <[e-mail]>

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 <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
 commit-graph.c | 2 +-
 t/t6500-gc.sh  | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

Comments

Jeff King Nov. 22, 2019, 7:17 a.m. UTC | #1

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

Derrick Stolee Nov. 25, 2019, 6:57 p.m. UTC | #2

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

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