{"thread":{"id":"61623","subject":"[PATCH] commit-graph: increment progress indicator","startedAt":"2024-06-11T15:09:18Z","lastAt":"2024-06-12T14:43:53Z","messageCount":4,"participants":["Johannes Schindelin via GitGitGadget","Patrick Steinhardt","Johannes Schindelin","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"496925","messageId":"pull.1743.git.1718118555197.gitgitgadget@gmail.com","threadId":"61623","inReplyTo":null,"subject":"[PATCH] commit-graph: increment progress indicator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-11T15:09:15Z","receivedAt":"2024-06-11T15:09:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nThis fixes a bug that was introduced by 368d19b0b7 (commit-graph:\nrefactor compute_topological_levels(), 2023-03-20): Previously, the\nprogress indicator was updated from `i + 1` where `i` is the loop\nvariable of the enclosing `for` loop. After this patch, the update used\n`info->progress_cnt + 1` instead, however, unlike `i`, the\n`progress_cnt` attribute was not incremented. Let's increment it.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    commit-graph: fix a progress indicator bug\n    \n    Stolee noticed this bug when integrating the for-each-ref --ahead-behind\n    patches into GitHub's internal fork of Git, and fixed it. For a variety\n    of reasons, upstreaming this fix fell between the cracks. Until now.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1743%2Fdscho%2Fincrement-progress-in-commit-graph-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1743/dscho/increment-progress-in-commit-graph-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1743\n\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex e5dd3553dfe..41a2e1b4c6d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1597,7 +1597,7 @@ static void compute_reachable_generation_numbers(\n \t\ttimestamp_t gen;\n \t\trepo_parse_commit(info->r, c);\n \t\tgen = info->get_generation(c, info->data);\n-\t\tdisplay_progress(info->progress, info->progress_cnt + 1);\n+\t\tdisplay_progress(info->progress, ++info->progress_cnt);\n \n \t\tif (gen != GENERATION_NUMBER_ZERO && gen != GENERATION_NUMBER_INFINITY)\n \t\t\tcontinue;\n\nbase-commit: 7b0defb3915eaa0bd118f0996e8c00b4eb2dc1ca\n-- \ngitgitgadget\n"},{"id":"496987","messageId":"ZmlK59HYWY_aXgv5@tanuki","threadId":"61623","inReplyTo":"pull.1743.git.1718118555197.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: increment progress indicator","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-12T07:14:47Z","receivedAt":"2024-06-12T07:14:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 11, 2024 at 03:09:15PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Derrick Stolee <derrickstolee@github.com>\n> \n> This fixes a bug that was introduced by 368d19b0b7 (commit-graph:\n> refactor compute_topological_levels(), 2023-03-20): Previously, the\n> progress indicator was updated from `i + 1` where `i` is the loop\n> variable of the enclosing `for` loop. After this patch, the update used\n> `info->progress_cnt + 1` instead, however, unlike `i`, the\n> `progress_cnt` attribute was not incremented. Let's increment it.\n> \n> Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>     commit-graph: fix a progress indicator bug\n>     \n>     Stolee noticed this bug when integrating the for-each-ref --ahead-behind\n>     patches into GitHub's internal fork of Git, and fixed it. For a variety\n>     of reasons, upstreaming this fix fell between the cracks. Until now.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1743%2Fdscho%2Fincrement-progress-in-commit-graph-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1743/dscho/increment-progress-in-commit-graph-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1743\n> \n>  commit-graph.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index e5dd3553dfe..41a2e1b4c6d 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1597,7 +1597,7 @@ static void compute_reachable_generation_numbers(\n>  \t\ttimestamp_t gen;\n>  \t\trepo_parse_commit(info->r, c);\n>  \t\tgen = info->get_generation(c, info->data);\n> -\t\tdisplay_progress(info->progress, info->progress_cnt + 1);\n> +\t\tdisplay_progress(info->progress, ++info->progress_cnt);\n>  \n>  \t\tif (gen != GENERATION_NUMBER_ZERO && gen != GENERATION_NUMBER_INFINITY)\n>  \t\t\tcontinue;\n\nThe fix looks obviously correct. Do we also want to amend tests? We have\ne.g. \"t6500-gc.sh\", \"gc --no-quiet\", where we already grep for the\nprogress report without verifying numbers. The output there is:\n\n    Computing commit graph topological levels:  25% (1/4), done.\n    Computing commit graph generation numbers:  25% (1/4), done.\n\n, which clearly demonstrates the bug for both callsites of the buggy\nfunction.\n\nThe following change would thus detect such regressions in the future:\n\n    diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n    index 43d40175f8..1b5909d1b7 100755\n    --- a/t/t6500-gc.sh\n    +++ b/t/t6500-gc.sh\n    @@ -158,7 +158,7 @@ test_expect_success TTY 'with TTY: gc --no-quiet' '\n            git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&\n        test_must_be_empty stdout &&\n        test_grep \"Enumerating objects\" stderr &&\n    -\ttest_grep \"Computing commit graph generation numbers\" stderr\n    +\ttest_grep \"Computing commit graph generation numbers: 100% (4/4), done.\" stderr\n     '\n     \n     test_expect_success 'gc --quiet' '\n\nPatrick\n"},{"id":"496996","messageId":"515d055e-a785-6d25-237d-bdfe06a2a69c@gmx.de","threadId":"61623","inReplyTo":"ZmlK59HYWY_aXgv5@tanuki","subject":"Re: [PATCH] commit-graph: increment progress indicator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-06-12T08:26:33Z","receivedAt":"2024-06-12T08:26:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Wed, 12 Jun 2024, Patrick Steinhardt wrote:\n\n> On Tue, Jun 11, 2024 at 03:09:15PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > From: Derrick Stolee <derrickstolee@github.com>\n> >\n> > This fixes a bug that was introduced by 368d19b0b7 (commit-graph:\n> > refactor compute_topological_levels(), 2023-03-20): Previously, the\n> > progress indicator was updated from `i + 1` where `i` is the loop\n> > variable of the enclosing `for` loop. After this patch, the update used\n> > `info->progress_cnt + 1` instead, however, unlike `i`, the\n> > `progress_cnt` attribute was not incremented. Let's increment it.\n> >\n> > [...]\n>\n> The fix looks obviously correct. Do we also want to amend tests? We have\n> e.g. \"t6500-gc.sh\", \"gc --no-quiet\", where we already grep for the\n> progress report without verifying numbers. The output there is:\n>\n>     Computing commit graph topological levels:  25% (1/4), done.\n>     Computing commit graph generation numbers:  25% (1/4), done.\n>\n> , which clearly demonstrates the bug for both callsites of the buggy\n> function.\n>\n> The following change would thus detect such regressions in the future:\n>\n>     diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n>     index 43d40175f8..1b5909d1b7 100755\n>     --- a/t/t6500-gc.sh\n>     +++ b/t/t6500-gc.sh\n>     @@ -158,7 +158,7 @@ test_expect_success TTY 'with TTY: gc --no-quiet' '\n>             git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&\n>         test_must_be_empty stdout &&\n>         test_grep \"Enumerating objects\" stderr &&\n>     -\ttest_grep \"Computing commit graph generation numbers\" stderr\n>     +\ttest_grep \"Computing commit graph generation numbers: 100% (4/4), done.\" stderr\n>      '\n>\n>      test_expect_success 'gc --quiet' '\n\nGood idea!\n\nThank you,\nJohannes\n"},{"id":"497037","messageId":"Zmm0JcJRguLqxeif@nand.local","threadId":"61623","inReplyTo":"pull.1743.git.1718118555197.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: increment progress indicator","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-12T14:43:49Z","receivedAt":"2024-06-12T14:43:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Johannes,\n\nOn Tue, Jun 11, 2024 at 03:09:15PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> This fixes a bug that was introduced by 368d19b0b7 (commit-graph:\n> refactor compute_topological_levels(), 2023-03-20): Previously, the\n> progress indicator was updated from `i + 1` where `i` is the loop\n> variable of the enclosing `for` loop. After this patch, the update used\n> `info->progress_cnt + 1` instead, however, unlike `i`, the\n> `progress_cnt` attribute was not incremented. Let's increment it.\n\nNice find and fix. I remember vaguely working on what became upstream\n368d19b0b7 with Stolee, and I'm glad to see the bug fix he wrote on top\nis also going upstream.\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> index e5dd3553dfe..41a2e1b4c6d 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1597,7 +1597,7 @@ static void compute_reachable_generation_numbers(\n>  \t\ttimestamp_t gen;\n>  \t\trepo_parse_commit(info->r, c);\n>  \t\tgen = info->get_generation(c, info->data);\n> -\t\tdisplay_progress(info->progress, info->progress_cnt + 1);\n> +\t\tdisplay_progress(info->progress, ++info->progress_cnt);\n\nIt looks like this info->progress_cnt is only used in\ncompute_reachable_generation_numbers() here, so I wonder if it may be\nworth it to do the following on top (ideally squashed into your patch\nhere):\n\n--- 8< ---\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 41a2e1b4c6..0410f6a9c3 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1558,7 +1558,6 @@ struct compute_generation_info {\n \tstruct repository *r;\n \tstruct packed_commit_list *commits;\n \tstruct progress *progress;\n-\tint progress_cnt;\n\n \ttimestamp_t (*get_generation)(struct commit *c, void *data);\n \tvoid (*set_generation)(struct commit *c, timestamp_t gen, void *data);\n@@ -1597,7 +1596,7 @@ static void compute_reachable_generation_numbers(\n \t\ttimestamp_t gen;\n \t\trepo_parse_commit(info->r, c);\n \t\tgen = info->get_generation(c, info->data);\n-\t\tdisplay_progress(info->progress, ++info->progress_cnt);\n+\t\tdisplay_progress(info->progress, i + 1);\n\n \t\tif (gen != GENERATION_NUMBER_ZERO && gen != GENERATION_NUMBER_INFINITY)\n \t\t\tcontinue;\n--- >8 ---\n\nThat would get rid of the info->progress_cnt field entirely, which seems\nbeneficial since it's only used by this single function, and we already\nhave 'i' which we can use as a replacement (as you note, effectively the\npre-image behavior of 368d19b0b7).\n\nBut I do not feel strongly either way, so no worries if you'd prefer to\nkeep this as-is.\n\nThanks,\nTaylor\n"}]}