{"thread":{"id":"40854","subject":"[PATCH v2] blame: add support for --[no-]progress option","startedAt":"2015-11-22T16:02:13Z","lastAt":"2015-11-23T02:47:57Z","messageCount":5,"participants":["Edmundo Carmona Antoranz","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"273589","messageId":"1448208133-29430-1-git-send-email-eantoranz@gmail.com","threadId":"40854","inReplyTo":null,"subject":"[PATCH v2] blame: add support for --[no-]progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-22T16:02:13Z","receivedAt":"2015-11-22T16:02:13Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Will also affect annotate\n\nSigned-off-by: Edmundo Carmona Antoranz \n---\n Documentation/blame-options.txt | 7 +++++++\n Documentation/git-blame.txt | 9 ++++++++-\n builtin/blame.c | 25 +++++++++++++++++++++++--\n 3 files changed, 38 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 760eab7..43f4f08 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -69,6 +69,13 @@ include::line-range-format.txt[]\n \tiso format is used. For supported values, see the discussion\n \tof the --date option at linkgit:git-log[1].\n \n+--[no-]progress::\n+\tProgress status is reported on the standard error stream\n+\tby default when it is attached to a terminal. This flag\n+\tenables progress reporting even if not attached to a\n+\tterminal.\n+\n+\n -M||::\n \tDetect moved or copied lines within a file. When a commit\n \tmoves or copies a block of lines (e.g. the original file\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex e6e947c..2e63397 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -10,7 +10,8 @@ SYNOPSIS\n [verse]\n 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]\n \t [-L ] [-S ] [-M] [-C] [-C] [-C] [--since=]\n-\t [--abbrev=] [ | --contents | --reverse ] [--] \n+\t [--[no-]progress] [--abbrev=] [ | --contents | --reverse ]\n+\t [--] \n \n DESCRIPTION\n -----------\n@@ -88,6 +89,12 @@ include::blame-options.txt[]\n \tabbreviated object name, use +1 digits. Note that 1 column\n \tis used for a caret to mark the boundary commit.\n \n+--[no-]progress::\n+\tProgress status is reported on the standard error stream\n+\tby default when it is attached to a terminal. This flag\n+\tenables progress reporting even if not attached to a\n+\tterminal.\n+\n \n THE PORCELAIN FORMAT\n --------------------\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 83612f5..480bb2d 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -28,6 +28,7 @@\n #include \"line-range.h\"\n #include \"line-log.h\"\n #include \"dir.h\"\n+#include \"progress.h\"\n \n static char blame_usage[] = N_(\"git blame [] [] [] [--] \");\n \n@@ -50,6 +51,7 @@ static int incremental;\n static int xdl_opts;\n static int abbrev = -1;\n static int no_whole_file_rename;\n+static int show_progress;\n \n static struct date_mode blame_date_mode = { DATE_ISO8601 };\n static size_t blame_date_width;\n@@ -1768,6 +1770,11 @@ static void assign_blame(struct scoreboard *sb, int opt)\n {\n \tstruct rev_info *revs = sb->revs;\n \tstruct commit *commit = prio_queue_get(&sb->commits);\n+\tstruct progress * progress = NULL;\n+\tint blamed_lines = 0;\n+\n+\tif (show_progress)\n+\t\tprogress = start_progress(_(\"Blaming lines\"), sb->num_lines);\n \n \twhile (commit) {\n \t\tstruct blame_entry *ent;\n@@ -1814,6 +1821,12 @@ static void assign_blame(struct scoreboard *sb, int opt)\n \t\t\t\t\tent = next;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n+\t\t\t\tif (progress) {\n+\t\t\t\t\tfor (next = suspect->suspects; next != NULL;\n+\t\t\t\t\t next = next->next)\n+\t\t\t\t\t\tblamed_lines += next->num_lines;\n+\t\t\t\t\tdisplay_progress(progress, blamed_lines);\n+\t\t\t\t}\n \t\t\t\tent->next = sb->ent;\n \t\t\t\tsb->ent = suspect->suspects;\n \t\t\t\tsuspect->suspects = NULL;\n@@ -1825,6 +1838,9 @@ static void assign_blame(struct scoreboard *sb, int opt)\n \t\tif (DEBUG) /* sanity */\n \t\t\tsanity_check_refcnt(sb);\n \t}\n+\n+\tif (progress)\n+\t\tstop_progress(&progress);\n }\n \n static const char *format_time(unsigned long time, const char *tz_str,\n@@ -2520,6 +2536,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('b', NULL, &blank_boundary, N_(\"Show blank SHA-1 for boundary commits (Default: off)\")),\n \t\tOPT_BOOL(0, \"root\", &show_root, N_(\"Do not treat root commits as boundaries (Default: off)\")),\n \t\tOPT_BOOL(0, \"show-stats\", &show_stats, N_(\"Show work cost statistics\")),\n+\t\tOPT_BOOL(0, \"progress\", &show_progress, N_(\"Force progress reporting\")),\n \t\tOPT_BIT(0, \"score-debug\", &output_option, N_(\"Show output score for blame entries\"), OUTPUT_SHOW_SCORE),\n \t\tOPT_BIT('f', \"show-name\", &output_option, N_(\"Show original filename (Default: auto)\"), OUTPUT_SHOW_NAME),\n \t\tOPT_BIT('n', \"show-number\", &output_option, N_(\"Show original linenumber (Default: off)\"), OUTPUT_SHOW_NUMBER),\n@@ -2555,6 +2572,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \n \tsave_commit_buffer = 0;\n \tdashdash_pos = 0;\n+\tshow_progress = -1;\n \n \tparse_options_start(&ctx, argc, argv, prefix, options,\n \t\t\t PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);\n@@ -2579,6 +2597,9 @@ parse_done:\n \tDIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);\n \targc = parse_options_end(&ctx);\n \n+\tif (show_progress < 0)\n+\t\tshow_progress = isatty(2);\n+\n \tif (0 < abbrev)\n \t\t/* one more abbrev length is needed for the boundary commit */\n \t\tabbrev++;\n@@ -2830,11 +2851,11 @@ parse_done:\n \n \tread_mailmap(&mailmap, NULL);\n \n+\tassign_blame(&sb, opt);\n+\n \tif (!incremental)\n \t\tsetup_pager();\n \n-\tassign_blame(&sb, opt);\n-\n \tfree(final_commit_name);\n \n \tif (incremental)\n-- \n2.6.2\n"},{"id":"273591","messageId":"56521E79.4080709@kdbg.org","threadId":"40854","inReplyTo":"1448208133-29430-1-git-send-email-eantoranz@gmail.com","subject":"Re: [PATCH v2] blame: add support for --[no-]progress option","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-11-22T19:58:49Z","receivedAt":"2015-11-22T19:58:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.11.2015 um 17:02 schrieb Edmundo Carmona Antoranz:\n> Will also affect annotate\n>\n> Signed-off-by: Edmundo Carmona Antoranz \n> ---\n> Documentation/blame-options.txt | 7 +++++++\n> Documentation/git-blame.txt | 9 ++++++++-\n> builtin/blame.c | 25 +++++++++++++++++++++++--\n> 3 files changed, 38 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> index 760eab7..43f4f08 100644\n> --- a/Documentation/blame-options.txt\n> +++ b/Documentation/blame-options.txt\n> @@ -69,6 +69,13 @@ include::line-range-format.txt[]\n> \tiso format is used. For supported values, see the discussion\n> \tof the --date option at linkgit:git-log[1].\n>\n> +--[no-]progress::\n> +\tProgress status is reported on the standard error stream\n> +\tby default when it is attached to a terminal. This flag\n> +\tenables progress reporting even if not attached to a\n> +\tterminal.\n> +\n> +\n> -M||::\n> \tDetect moved or copied lines within a file. When a commit\n> \tmoves or copies a block of lines (e.g. the original file\n> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\n> index e6e947c..2e63397 100644\n> --- a/Documentation/git-blame.txt\n> +++ b/Documentation/git-blame.txt\n> @@ -10,7 +10,8 @@ SYNOPSIS\n> [verse]\n> 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]\n> \t [-L ] [-S ] [-M] [-C] [-C] [-C] [--since=]\n> -\t [--abbrev=] [ | --contents | --reverse ] [--] \n> +\t [--[no-]progress] [--abbrev=] [ | --contents | --reverse ]\n> +\t [--] \n\nYou add the option to to the synopsis of git-blame.txt, but not to \ngit-annotate.txt.\n\n>\n> DESCRIPTION\n> -----------\n> @@ -88,6 +89,12 @@ include::blame-options.txt[]\n> \tabbreviated object name, use +1 digits. Note that 1 column\n> \tis used for a caret to mark the boundary commit.\n>\n> +--[no-]progress::\n> +\tProgress status is reported on the standard error stream\n> +\tby default when it is attached to a terminal. This flag\n> +\tenables progress reporting even if not attached to a\n> +\tterminal.\n> +\n\nAny particular reason you add this text twice? As can be seen on the \nhunk header, git-blame.txt includes blame-options.txt.\n\n-- Hannes\n"},{"id":"273592","messageId":"CAOc6etZzE030LBUVKZSEHsjbbCQJr_UbfCYKxBOWdjkqDdZYDg@mail.gmail.com","threadId":"40854","inReplyTo":"56521E79.4080709@kdbg.org","subject":"Re: [PATCH v2] blame: add support for --[no-]progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-22T20:28:33Z","receivedAt":"2015-11-22T20:28:33Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Hey, Johannes!\n\nAn oversight! git-annotate.txt took me to blame-options.txt.... and I\ndidn't notice it's also pointed to from git-blame.txt. Let's wait for\nsome comments about the coding (or blessings) so that I can send\nanother patch.\n\nThanks!\n\nOn Sun, Nov 22, 2015 at 1:58 PM, Johannes Sixt wrote:\n> Am 22.11.2015 um 17:02 schrieb Edmundo Carmona Antoranz:\n>>\n>> Will also affect annotate\n>>\n>> Signed-off-by: Edmundo Carmona Antoranz \n>> ---\n>> Documentation/blame-options.txt | 7 +++++++\n>> Documentation/git-blame.txt | 9 ++++++++-\n>> builtin/blame.c | 25 +++++++++++++++++++++++--\n>> 3 files changed, 38 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/Documentation/blame-options.txt\n>> b/Documentation/blame-options.txt\n>> index 760eab7..43f4f08 100644\n>> --- a/Documentation/blame-options.txt\n>> +++ b/Documentation/blame-options.txt\n>> @@ -69,6 +69,13 @@ include::line-range-format.txt[]\n>> iso format is used. For supported values, see the discussion\n>> of the --date option at linkgit:git-log[1].\n>>\n>> +--[no-]progress::\n>> + Progress status is reported on the standard error stream\n>> + by default when it is attached to a terminal. This flag\n>> + enables progress reporting even if not attached to a\n>> + terminal.\n>> +\n>> +\n>> -M||::\n>> Detect moved or copied lines within a file. When a commit\n>> moves or copies a block of lines (e.g. the original file\n>> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\n>> index e6e947c..2e63397 100644\n>> --- a/Documentation/git-blame.txt\n>> +++ b/Documentation/git-blame.txt\n>> @@ -10,7 +10,8 @@ SYNOPSIS\n>> [verse]\n>> 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w]\n>> [--incremental]\n>> [-L ] [-S ] [-M] [-C] [-C] [-C]\n>> [--since=]\n>> - [--abbrev=] [ | --contents | --reverse ]\n>> [--] \n>> + [--[no-]progress] [--abbrev=] [ | --contents |\n>> --reverse ]\n>> + [--] \n>\n>\n> You add the option to to the synopsis of git-blame.txt, but not to\n> git-annotate.txt.\n>\n>>\n>> DESCRIPTION\n>> -----------\n>> @@ -88,6 +89,12 @@ include::blame-options.txt[]\n>> abbreviated object name, use +1 digits. Note that 1 column\n>> is used for a caret to mark the boundary commit.\n>>\n>> +--[no-]progress::\n>> + Progress status is reported on the standard error stream\n>> + by default when it is attached to a terminal. This flag\n>> + enables progress reporting even if not attached to a\n>> + terminal.\n>> +\n>\n>\n> Any particular reason you add this text twice? As can be seen on the hunk\n> header, git-blame.txt includes blame-options.txt.\n>\n> -- Hannes\n>\n"},{"id":"273593","messageId":"xmqq610to4bl.fsf@gitster.mtv.corp.google.com","threadId":"40854","inReplyTo":"1448208133-29430-1-git-send-email-eantoranz@gmail.com","subject":"Re: [PATCH v2] blame: add support for --[no-]progress option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-23T02:08:30Z","receivedAt":"2015-11-23T02:08:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edmundo Carmona Antoranz writes:\n\n> Will also affect annotate\n\nIs that a good thing? In any case, make it understandable without\nthe title line (i.e. make it a full sentence, ending with a full\nstop).\n\n> +\t\t\t\tif (progress) {\n> +\t\t\t\t\tfor (next = suspect->suspects; next != NULL;\n> +\t\t\t\t\t next = next->next)\n> +\t\t\t\t\t\tblamed_lines += next->num_lines;\n> +\t\t\t\t\tdisplay_progress(progress, blamed_lines);\n> +\t\t\t\t}\n\nIs this math and the placement of the code correct? It would\nprobably be more obvious if this hunk is in found_guilty_entry(),\nwhich is already the dedicated function in which we report about a\ngroup of lines whose ultimate origin has become clear.\n\n> @@ -2830,11 +2851,11 @@ parse_done:\n> \n> \tread_mailmap(&mailmap, NULL);\n> \n> +\tassign_blame(&sb, opt);\n> +\n> \tif (!incremental)\n> \t\tsetup_pager();\n> \n> -\tassign_blame(&sb, opt);\n> -\n> \tfree(final_commit_name);\n> \n> \tif (incremental)\n\nTwo comments.\n\n * How does this interact with incremental or porcelain blame?\n Shouldn't progress be turned off when these modes are in use?\n\n * Shouldn't progress be turned off if the result comes very\n quickly, using start_progress_delay()?\n"},{"id":"273594","messageId":"CAOc6etY2+OkHhn1rt=y8ekjzcA8ngu0zvX_MZ5Pk6utYj09vPg@mail.gmail.com","threadId":"40854","inReplyTo":"xmqq610to4bl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] blame: add support for --[no-]progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-23T02:47:57Z","receivedAt":"2015-11-23T02:47:57Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Nice to see you back, Junio!\n\n\nOn Sun, Nov 22, 2015 at 8:08 PM, Junio C Hamano wrote:\n> Edmundo Carmona Antoranz writes:\n>\n>> Will also affect annotate\n>\n> Is that a good thing? In any case, make it understandable without\n> the title line (i.e. make it a full sentence, ending with a full\n> stop).\n>\n\nWill make the explanation a little more verbose. About being a bad\nthing, I don't see how, it's just the same functionality. You think I\nshould turn it off if using annotate?\n\n>> + if (progress) {\n>> + for (next = suspect->suspects; next != NULL;\n>> + next = next->next)\n>> + blamed_lines += next->num_lines;\n>> + display_progress(progress, blamed_lines);\n>> + }\n>\n> Is this math and the placement of the code correct? It would\n> probably be more obvious if this hunk is in found_guilty_entry(),\n> which is already the dedicated function in which we report about a\n> group of lines whose ultimate origin has become clear.\n>\n\nI'll see what I can do about it.\n\n>\n> Two comments.\n>\n> * How does this interact with incremental or porcelain blame?\n> Shouldn't progress be turned off when these modes are in use?\n\nGiven that they are supposed to be for machine consumption, I'll turn\nprogress off is using one of them.\n\n>\n> * Shouldn't progress be turned off if the result comes very\n> quickly, using start_progress_delay()?\n>\n\nOk. Default values as in checkout? 50, 1?\n"}]}