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

Commit Message

Johannes Schindelin via GitGitGadget Nov. 7, 2019, 5:46 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. 7, 2019, 9:26 p.m. UTC | #1

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

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

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

SZEDER Gábor Nov. 21, 2019, 11:03 p.m. UTC | #2

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

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