{"thread":{"id":"57653","subject":"[PATCH] blame: report correct number of lines in progress when using ranges","startedAt":"2022-04-03T16:53:33Z","lastAt":"2022-04-04T21:23:47Z","messageCount":6,"participants":["Edmundo Carmona Antoranz","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"452996","messageId":"20220403165038.52803-1-eantoranz@gmail.com","threadId":"57653","inReplyTo":null,"subject":"[PATCH] blame: report correct number of lines in progress when using ranges","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2022-04-03T16:50:38Z","receivedAt":"2022-04-03T16:53:33Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"When using ranges, use their 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\nSigned-off-by: Edmundo Carmona Antoranz \n---\n builtin/blame.c | 6 +++++-\n 1 file changed, 5 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 = π\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-- \n2.35.1\n\n"},{"id":"453012","messageId":"xmqq5ynpu6uv.fsf@gitster.g","threadId":"57653","inReplyTo":"20220403165038.52803-1-eantoranz@gmail.com","subject":"Re: [PATCH] blame: report correct number of lines in progress when using ranges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-03T22:42:16Z","receivedAt":"2022-04-03T22:42:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edmundo Carmona Antoranz writes:\n\n> When using ranges, use their sizes as the limit for progress\n> instead of the size of the full file.\n>\n> Before:\n> $ git blame --progress builtin/blame.c > /dev/null\n> Blaming lines: 100% (1210/1210), done.\n> $ git blame --progress -L 100,120 -L 200,300 builtin/blame.c > /dev/null\n> Blaming lines: 10% (122/1210), done.\n> $\n>\n> After:\n> $ ./git blame --progress builtin/blame.c > /dev/null\n> Blaming lines: 100% (1210/1210), done.\n> $ ./git blame --progress -L 100,120 -L 200,300 builtin/blame.c > /dev/null\n> Blaming lines: 100% (122/122), done.\n> $\n>\n> Signed-off-by: Edmundo Carmona Antoranz \n> ---\n> builtin/blame.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n\nWow. I am reasonably sure that this was broken since the\nintroduction of the progress meter to \"git blame\". Thanks for\nfinding and fixing.\n\nCan we have a test, too, or is that too cumbersome to add for some\nreason?\n\nThanks.\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 = π\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"},{"id":"453018","messageId":"CAOc6etac3tQdOzY1bF7qBVOJ_ndns-KOSum8w-Jie06Ynz_cWw@mail.gmail.com","threadId":"57653","inReplyTo":"xmqq5ynpu6uv.fsf@gitster.g","subject":"Re: [PATCH] blame: report correct number of lines in progress when using ranges","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2022-04-04T05:27:26Z","receivedAt":"2022-04-04T05:27:43Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Mon, Apr 4, 2022 at 12:42 AM Junio C Hamano wrote:\n>\n> Wow. I am reasonably sure that this was broken since the\n> introduction of the progress meter to \"git blame\". Thanks for\n> finding and fixing.\n\nAs the person who originally wrote said support, I feel like I\nam just nurturing it. :-D\n\n>\n> Can we have a test, too, or is that too cumbersome to add for some\n> reason?\n\nCorrect me if I'm wrong but it could be a little tricky because \"progress\ndisplay\" shows up only if it happens to \"lose\" a race. Progress is\nskipped altogether if blame process goes too fast. Even if you run\nblame on a file with a lot of history, if box is fast enough and info is\ncached, it will fail to display progress. So, all in all, it would be like\ntrying to unit test output coming out of a race condition.\n\nLet me know what you think.\n\n>\n> Thanks.\nyw\n"},{"id":"453022","messageId":"220404.8635itl6h0.gmgdl@evledraar.gmail.com","threadId":"57653","inReplyTo":"20220403165038.52803-1-eantoranz@gmail.com","subject":"Re: [PATCH] blame: report correct number of lines in progress when using ranges","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-04T06:12:06Z","receivedAt":"2022-04-04T06:15:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Apr 03 2022, Edmundo Carmona Antoranz wrote:\n\n> When using ranges, use their sizes as the limit for progress\n> instead of the size of the full file.\n>\n> Before:\n> $ git blame --progress builtin/blame.c > /dev/null\n> Blaming lines: 100% (1210/1210), done.\n> $ git blame --progress -L 100,120 -L 200,300 builtin/blame.c > /dev/null\n> Blaming lines: 10% (122/1210), done.\n> $\n>\n> After:\n> $ ./git blame --progress builtin/blame.c > /dev/null\n> Blaming lines: 100% (1210/1210), done.\n> $ ./git blame --progress -L 100,120 -L 200,300 builtin/blame.c > /dev/null\n> Blaming lines: 100% (122/122), done.\n> $\n>\n> Signed-off-by: Edmundo Carmona Antoranz \n> ---\n> builtin/blame.c | 6 +++++-\n> 1 file changed, 5 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\nHere ranges's nr is unsigned int, and the \"num_lines\" is an int, and the\nargument to start_delayed_progress() is uint64_t, but both of \"start\"\nand \"end\" are \"long\".\n\nBut we appand multiple differences to num_lines, are we sure we won't\noverflow here?\n\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 = π\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"},{"id":"453023","messageId":"220404.86y20ljrui.gmgdl@evledraar.gmail.com","threadId":"57653","inReplyTo":"CAOc6etac3tQdOzY1bF7qBVOJ_ndns-KOSum8w-Jie06Ynz_cWw@mail.gmail.com","subject":"Re: [PATCH] blame: report correct number of lines in progress when using ranges","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-04T06:15:52Z","receivedAt":"2022-04-04T06:16:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Apr 04 2022, Edmundo Carmona Antoranz wrote:\n\n> On Mon, Apr 4, 2022 at 12:42 AM Junio C Hamano wrote:\n>>\n>> Wow. I am reasonably sure that this was broken since the\n>> introduction of the progress meter to \"git blame\". Thanks for\n>> finding and fixing.\n>\n> As the person who originally wrote said support, I feel like I\n> am just nurturing it. :-D\n>\n>>\n>> Can we have a test, too, or is that too cumbersome to add for some\n>> reason?\n>\n> Correct me if I'm wrong but it could be a little tricky because \"progress\n> display\" shows up only if it happens to \"lose\" a race. Progress is\n> skipped altogether if blame process goes too fast. Even if you run\n> blame on a file with a lot of history, if box is fast enough and info is\n> cached, it will fail to display progress. So, all in all, it would be like\n> trying to unit test output coming out of a race condition.\n>\n> Let me know what you think.\n\nYou can make it always show up by using GIT_PROGRESS_DELAY=0, grep for\nit in existing tests, skimming the code that coupled with --progress\nshould guarantee that it shows up.\n"},{"id":"453055","messageId":"xmqq5ynost82.fsf@gitster.g","threadId":"57653","inReplyTo":"220404.8635itl6h0.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] blame: report correct number of lines in progress when using ranges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-04T16:34:21Z","receivedAt":"2022-04-04T21:23:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\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> Here ranges's nr is unsigned int, and the \"num_lines\" is an int, and the\n> argument to start_delayed_progress() is uint64_t, but both of \"start\"\n> and \"end\" are \"long\".\n\nI see num_lines is a long, which I am guessing was made to match\nwhat start and end are, which in turn is to use parse_range_arg().\n\n> But we appand multiple differences to num_lines, are we sure we won't\n> overflow here?\n\nLooking at members of blame_entry and blame_scoreboard structures, I\nthink we make liberal use of \"int\" in the blame codepath without\nworrying about those with 16-bit int, or those with files longer\nthan 2G lines, and progress meter showing incorrect values to them\nis probably the least of our worries ;-)\n\n"}]}