{"thread":{"id":"57668","subject":"[PATCH v3] blame: report correct number of lines in progress when using ranges","startedAt":"2022-04-05T21:50:01Z","lastAt":"2022-04-06T20:19:15Z","messageCount":5,"participants":["Edmundo Carmona Antoranz","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"453130","messageId":"20220405165806.842520-1-eantoranz@gmail.com","threadId":"57668","inReplyTo":null,"subject":"[PATCH v3] blame: report correct number of lines in progress when using ranges","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2022-04-05T16:58:06Z","receivedAt":"2022-04-05T21:50:01Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"When using ranges, use the range sizes as the limit for progress\ninstead of the size of the full file.\n\nBefore:\n$ git blame --progress builtin/blame.c > /dev/null\nBlaming lines: 100% (1210/1210), done.\n$ git blame --progress -L 100,120 -L 200,300 builtin/blame.c > /dev/null\nBlaming lines:  10% (122/1210), done.\n$\n\nAfter:\n$ ./git blame --progress builtin/blame.c > /dev/null\nBlaming lines: 100% (1210/1210), done.\n$ ./git blame --progress -L 100,120 -L 200,300 builtin/blame.c > /dev/null\nBlaming lines: 100% (122/122), done.\n$\n\nNote: Shamelessly copied show_cr from t0500-progress-display.sh\nSigned-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>\n---\n builtin/blame.c     |  6 ++++-\n t/annotate-tests.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 58 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 8d15b68afc..e33372c56b 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -898,6 +898,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tunsigned int range_i;\n \tlong anchor;\n \tconst int hexsz = the_hash_algo->hexsz;\n+\tlong num_lines = 0;\n \n \tsetup_default_color_by_age();\n \tgit_config(git_blame_config, &output_option);\n@@ -1129,7 +1130,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tfor (range_i = ranges.nr; range_i > 0; --range_i) {\n \t\tconst struct range *r = &ranges.ranges[range_i - 1];\n \t\tent = blame_entry_prepend(ent, r->start, r->end, o);\n+\t\tnum_lines += (r->end - r->start);\n \t}\n+\tif (!num_lines)\n+\t\tnum_lines = sb.num_lines;\n \n \to->suspects = ent;\n \tprio_queue_put(&sb.commits, o->commit);\n@@ -1158,7 +1162,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tsb.found_guilty_entry = &found_guilty_entry;\n \tsb.found_guilty_entry_data = &pi;\n \tif (show_progress)\n-\t\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines);\n+\t\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), num_lines);\n \n \tassign_blame(&sb, opt);\n \ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex 09e86f9ba0..90932e304b 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -12,6 +12,10 @@ else\n   }\n fi\n \n+show_cr () {\n+\ttr '\\015' Q | sed -e \"s/Q/<CR>\\\\$LF/g\"\n+}\n+\n check_count () {\n \thead= &&\n \tfile='file' &&\n@@ -604,3 +608,52 @@ test_expect_success 'blame -L X,-N (non-numeric N)' '\n test_expect_success 'blame -L ,^/RE/' '\n \ttest_must_fail $PROG -L1,^/99/ file\n '\n+\n+test_expect_success 'blame progress on a full file' '\n+\tcat >expect <<-\\EOF &&\n+\tBlaming lines:  10% (1/10)<CR>\n+\tBlaming lines:  50% (5/10)<CR>\n+\tBlaming lines:  60% (6/10)<CR>\n+\tBlaming lines:  90% (9/10)<CR>\n+\tBlaming lines: 100% (10/10)<CR>\n+\tBlaming lines: 100% (10/10), done.\n+\tEOF\n+\n+\tGIT_PROGRESS_DELAY=0 \\\n+\tgit blame --progress hello.c 2>stderr &&\n+\n+\tshow_cr <stderr >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'blame progress on a single range' '\n+\tcat >expect <<-\\EOF &&\n+\tBlaming lines:  25% (1/4)<CR>\n+\tBlaming lines:  75% (3/4)<CR>\n+\tBlaming lines: 100% (4/4)<CR>\n+\tBlaming lines: 100% (4/4), done.\n+\tEOF\n+\n+\tGIT_PROGRESS_DELAY=0 \\\n+\tgit blame --progress -L 3,6 hello.c 2>stderr &&\n+\n+\tshow_cr <stderr >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'blame progress on multiple ranges' '\n+\tcat >expect <<-\\EOF &&\n+\tBlaming lines:  42% (3/7)<CR>\n+\tBlaming lines:  57% (4/7)<CR>\n+\tBlaming lines:  85% (6/7)<CR>\n+\tBlaming lines: 100% (7/7)<CR>\n+\tBlaming lines: 100% (7/7), done.\n+\tEOF\n+\n+\tGIT_PROGRESS_DELAY=0 \\\n+\tgit blame --progress -L 3,6 -L 8,10 hello.c 2>stderr &&\n+\n+\tcp stderr /home/antoranz/proyectos/git/git/stderr &&\n+\tshow_cr <stderr >actual &&\n+\ttest_cmp expect actual\n+'\n-- \n2.35.1\n\n"},{"id":"453164","messageId":"220406.86wng2eh0l.gmgdl@evledraar.gmail.com","threadId":"57668","inReplyTo":"20220405165806.842520-1-eantoranz@gmail.com","subject":"Re: [PATCH v3] blame: report correct number of lines in progress when using ranges","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-06T08:35:43Z","receivedAt":"2022-04-06T12:37:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 05 2022, Edmundo Carmona Antoranz wrote:\n\nHint: use --in-reply-to on re-rolls, this is in reply to v2 here:\nhttps://lore.kernel.org/git/20220404182129.33992-1-eantoranz@gmail.com/\n\nAlso the --range-diff option to git-format-patch is really helpful,\nit'll make a diff between v2 and this v3 and attach it after \"--\".\n\nAnyway...\n\n> Note: Shamelessly copied show_cr from t0500-progress-display.sh\n> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>\n\nFormatting: \\n before the Signed-off-by.\n\n> ---\n>  builtin/blame.c     |  6 ++++-\n>  t/annotate-tests.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 58 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 8d15b68afc..e33372c56b 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -898,6 +898,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n>  \tunsigned int range_i;\n>  \tlong anchor;\n>  \tconst int hexsz = the_hash_algo->hexsz;\n> +\tlong num_lines = 0;\n>  \n>  \tsetup_default_color_by_age();\n>  \tgit_config(git_blame_config, &output_option);\n> @@ -1129,7 +1130,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n>  \tfor (range_i = ranges.nr; range_i > 0; --range_i) {\n>  \t\tconst struct range *r = &ranges.ranges[range_i - 1];\n>  \t\tent = blame_entry_prepend(ent, r->start, r->end, o);\n> +\t\tnum_lines += (r->end - r->start);\n>  \t}\n> +\tif (!num_lines)\n> +\t\tnum_lines = sb.num_lines;\n>  \n>  \to->suspects = ent;\n>  \tprio_queue_put(&sb.commits, o->commit);\n> @@ -1158,7 +1162,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n>  \tsb.found_guilty_entry = &found_guilty_entry;\n>  \tsb.found_guilty_entry_data = &pi;\n>  \tif (show_progress)\n> -\t\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines);\n> +\t\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), num_lines);\n>  \n>  \tassign_blame(&sb, opt);\n>  \n> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\n> index 09e86f9ba0..90932e304b 100644\n> --- a/t/annotate-tests.sh\n> +++ b/t/annotate-tests.sh\n> @@ -12,6 +12,10 @@ else\n>    }\n>  fi\n>  \n> +show_cr () {\n> +\ttr '\\015' Q | sed -e \"s/Q/<CR>\\\\$LF/g\"\n> +}\n> +\n>  check_count () {\n>  \thead= &&\n>  \tfile='file' &&\n> @@ -604,3 +608,52 @@ test_expect_success 'blame -L X,-N (non-numeric N)' '\n>  test_expect_success 'blame -L ,^/RE/' '\n>  \ttest_must_fail $PROG -L1,^/99/ file\n>  '\n> +\n> +test_expect_success 'blame progress on a full file' '\n> +\tcat >expect <<-\\EOF &&\n> +\tBlaming lines:  10% (1/10)<CR>\n> +\tBlaming lines:  50% (5/10)<CR>\n> +\tBlaming lines:  60% (6/10)<CR>\n> +\tBlaming lines:  90% (9/10)<CR>\n> +\tBlaming lines: 100% (10/10)<CR>\n> +\tBlaming lines: 100% (10/10), done.\n> +\tEOF\n> +\n> +\tGIT_PROGRESS_DELAY=0 \\\n> +\tgit blame --progress hello.c 2>stderr &&\n> +\n> +\tshow_cr <stderr >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'blame progress on a single range' '\n> +\tcat >expect <<-\\EOF &&\n> +\tBlaming lines:  25% (1/4)<CR>\n> +\tBlaming lines:  75% (3/4)<CR>\n> +\tBlaming lines: 100% (4/4)<CR>\n> +\tBlaming lines: 100% (4/4), done.\n> +\tEOF\n> +\n> +\tGIT_PROGRESS_DELAY=0 \\\n> +\tgit blame --progress -L 3,6 hello.c 2>stderr &&\n> +\n> +\tshow_cr <stderr >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'blame progress on multiple ranges' '\n> +\tcat >expect <<-\\EOF &&\n> +\tBlaming lines:  42% (3/7)<CR>\n> +\tBlaming lines:  57% (4/7)<CR>\n> +\tBlaming lines:  85% (6/7)<CR>\n> +\tBlaming lines: 100% (7/7)<CR>\n> +\tBlaming lines: 100% (7/7), done.\n> +\tEOF\n> +\n> +\tGIT_PROGRESS_DELAY=0 \\\n> +\tgit blame --progress -L 3,6 -L 8,10 hello.c 2>stderr &&\n> +\n> +\tcp stderr /home/antoranz/proyectos/git/git/stderr &&\n> +\tshow_cr <stderr >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nWe had a small thread that I notice now was off-list in reply to\nhttps://lore.kernel.org/git/220405.86o81flve1.gmgdl@evledraar.gmail.com/. Quoted\nbelow. I assume that was both of our mistakes:\n\n\tOn Tue, Apr 05 2022, Ævar Arnfjörð Bjarmason wrote:\n\t\n\t> On Tue, Apr 05 2022, Edmundo Carmona Antoranz wrote:\n\t>\n\t>> On Tue, Apr 5, 2022 at 11:41 AM Ævar Arnfjörð Bjarmason\n\t>> <avarab@gmail.com> wrote:\n\t>>>\n\t>>>\n\t>>> Let's use test_cmp here instead, as we expect nothing else on stderr,\n\t>>> and with grep one wonders why it's not ^$ anchored, but just:\n\t>>>\n\t>>>     echo \"Blaming lines: 100% (6/6), done.\" >expect &&\n\t>>>     git blame ... 2>actual &&\n\t>>>     test_cmp expect actual\n\t>>>\n\t>>> is better, both because it's more exhaustive as a test, and because\n\t>>> it'll give better debug (diff) output on failure than grep will (just no\n\t>>> output at all).\n\t>>>\n\t>>\n\t>> The problem is that progress output is using CRs to write each line\n\t>> so, when checking the output, if you tried with ^$ with the last line,\n\t>> it wouldn't match anyway. I switched to match progress output as a\n\t>> whole using the same technique that is used in\n\t>> t0500-progress-display.sh.\n\t>>\n\t>> v3 is already out there.\n\t>\n\t> Ah yes, I forgot about that. Nevermind\n\nI.e. the test_cmp here is now, given what you mentioned I'd have been\nfine with the grep, but stealing the show_cr also works.\n\nI suppose it's also abetter as a targeted fix, since the point of this\npatch is specifically to fix a bug where we wouldn't do the right\n\"steps\" in-between with the progress bar, in addition to the end-state\nnot being correct.\n\nAre the small number of missing steps above expected? E.g. 1-2/7 and 5/7\nin the last tets above, ditto the rest?\n\nMm, yes, looking at assign_blame() in blame.c we'll \"skip\" some.\n\nSo if we ever change that algorithm we'll need to adjust these, but it's\nprobably good to notice that then, even if the test_cmp here does\nimplicitly encode a bit of internal implementation details, i.e. when\nexactly we update the progress bar.\n"},{"id":"453204","messageId":"xmqq7d82yx06.fsf@gitster.g","threadId":"57668","inReplyTo":"220406.86wng2eh0l.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3] blame: report correct number of lines in progress when using ranges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-06T16:52:57Z","receivedAt":"2022-04-06T18:13:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Are the small number of missing steps above expected? E.g. 1-2/7 and 5/7\n> in the last tets above, ditto the rest?\n>\n> Mm, yes, looking at assign_blame() in blame.c we'll \"skip\" some.\n>\n> So if we ever change that algorithm we'll need to adjust these, but it's\n> probably good to notice that then, even if the test_cmp here does\n> implicitly encode a bit of internal implementation details, i.e. when\n> exactly we update the progress bar.\n\nI do not think that is something we want to test, or distract the\ndevelopers by forcing them to update the skipping hardcoded in the\ntest.\n\nIs it easy to grab the last progress message from the output?  What\nwe care, and have cared in the past, is that we expect that the 100%\nis paired with the number of lines in the ranges, and the progress\nactually ends at 100% (we had different bugs that did not bring the\nmeter to 100% at the end), so checking that end result would be what\nmatters most, e.g.\n\n    tr '\\015' '\\012' output | tail -n 1\n\nor something?\n\n"},{"id":"453212","messageId":"CAOc6etZkOTs-XonAxEWPt8SepUALg8yprtKA_6=H1AbjOtmwCw@mail.gmail.com","threadId":"57668","inReplyTo":"xmqq7d82yx06.fsf@gitster.g","subject":"Re: [PATCH v3] blame: report correct number of lines in progress when using ranges","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2022-04-06T17:46:22Z","receivedAt":"2022-04-06T20:02:18Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Wed, Apr 6, 2022 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n>\n> I do not think that is something we want to test, or distract the\n> developers by forcing them to update the skipping hardcoded in the\n> test.\n>\n> Is it easy to grab the last progress message from the output?  What\n> we care, and have cared in the past, is that we expect that the 100%\n> is paired with the number of lines in the ranges, and the progress\n> actually ends at 100% (we had different bugs that did not bring the\n> meter to 100% at the end), so checking that end result would be what\n> matters most, e.g.\n>\n>     tr '\\015' '\\012' output | tail -n 1\n>\n> or something?\n>\n\nLet me send the next version of the patch that will hopefully get the best\nof all the ideas we have been throwing around.\n"},{"id":"453216","messageId":"CAOc6etbgsgb6J+0RW-y6HrOvPzpt1daNOeuMnSc8oiRwXbCcnQ@mail.gmail.com","threadId":"57668","inReplyTo":"220406.86wng2eh0l.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3] blame: report correct number of lines in progress when using ranges","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2022-04-06T17:57:07Z","receivedAt":"2022-04-06T20:19:15Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Wed, Apr 6, 2022 at 10:46 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> Hint: use --in-reply-to on re-rolls, this is in reply to v2 here:\n> https://lore.kernel.org/git/20220404182129.33992-1-eantoranz@gmail.com/\n>\n> Also the --range-diff option to git-format-patch is really helpful,\n> it'll make a diff between v2 and this v3 and attach it after \"--\".\n>\n> Anyway...\n>\n\nThanks for the tips. I realized I left a garbage line in the v3 patch\nthat I would like to never see moving forward so I will send a clean\npatch for v4 just like the previous ones, hope that's not a problem. I\nwill keep the tips in mind for a future time.\n"}]}