{"thread":{"id":"40876","subject":"[PATCH v5] blame: add support for --[no-]progress option","startedAt":"2015-11-25T04:36:05Z","lastAt":"2015-12-10T07:43:14Z","messageCount":4,"participants":["Edmundo Carmona Antoranz","Eric Sunshine"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"273700","messageId":"1448426165-5139-1-git-send-email-eantoranz@gmail.com","threadId":"40876","inReplyTo":null,"subject":"[PATCH v5] blame: add support for --[no-]progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-25T04:36:05Z","receivedAt":"2015-11-25T04:36:05Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"* created struct progress_info in builtin/blame.c\n  this struct holds the information used to display progress so\n  that only one additional parameter is passed to\n  found_guilty_entry().\n\n* --[no-]progress option is also inherited by git-annotate.\n\nSigned-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>\n---\n Documentation/blame-options.txt |  7 +++++++\n Documentation/git-blame.txt     |  3 ++-\n builtin/blame.c                 | 40 ++++++++++++++++++++++++++++++++++++----\n 3 files changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 760eab7..ef642b9 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. Progress information won't be displayed if using\n+\t`--porcelain` or `--incremental`.\n+\n -M|<num>|::\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..b0154c2 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 <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]\n-\t    [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>] [--] <file>\n+\t    [--[no-]progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>]\n+\t    [--] <file>\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 83612f5..954d32c 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 [<options>] [<rev-opts>] [<rev>] [--] <file>\");\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@@ -127,6 +129,11 @@ struct origin {\n \tchar path[FLEX_ARRAY];\n };\n \n+struct progress_info {\n+\tstruct progress *progress;\n+\tint blamed_lines;\n+};\n+\n static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen,\n \t\t      xdl_emit_hunk_consume_func_t hunk_func, void *cb_data)\n {\n@@ -1746,7 +1753,8 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)\n  * The blame_entry is found to be guilty for the range.\n  * Show it in incremental output.\n  */\n-static void found_guilty_entry(struct blame_entry *ent)\n+static void found_guilty_entry(struct blame_entry *ent,\n+\t\t\t   struct progress_info *pi)\n {\n \tif (incremental) {\n \t\tstruct origin *suspect = ent->suspect;\n@@ -1758,6 +1766,8 @@ static void found_guilty_entry(struct blame_entry *ent)\n \t\twrite_filename_info(suspect->path);\n \t\tmaybe_flush_or_die(stdout, \"stdout\");\n \t}\n+\tif (pi)\n+\t\tdisplay_progress(pi->progress, pi->blamed_lines += ent->num_lines);\n }\n \n /*\n@@ -1768,6 +1778,16 @@ 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_info *pi = NULL;\n+\n+\tif (show_progress) {\n+\t\tpi = malloc(sizeof(*pi));\n+\t\tif (pi) {\n+\t\t\tpi->progress = start_progress_delay(_(\"Blaming lines\"),\n+\t\t\t                                    sb->num_lines, 50, 1);\n+\t\t\tpi->blamed_lines = 0;\n+\t\t}\n+\t}\n \n \twhile (commit) {\n \t\tstruct blame_entry *ent;\n@@ -1809,7 +1829,7 @@ static void assign_blame(struct scoreboard *sb, int opt)\n \t\t\tsuspect->guilty = 1;\n \t\t\tfor (;;) {\n \t\t\t\tstruct blame_entry *next = ent->next;\n-\t\t\t\tfound_guilty_entry(ent);\n+\t\t\t\tfound_guilty_entry(ent, pi);\n \t\t\t\tif (next) {\n \t\t\t\t\tent = next;\n \t\t\t\t\tcontinue;\n@@ -1825,6 +1845,11 @@ 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 (pi) {\n+\t\tstop_progress(&pi->progress);\n+\t\tfree(pi);\n+\t};\n }\n \n static const char *format_time(unsigned long time, const char *tz_str,\n@@ -2520,6 +2545,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 +2581,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 +2606,11 @@ parse_done:\n \tDIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);\n \targc = parse_options_end(&ctx);\n \n+\tif (incremental || (output_option & OUTPUT_PORCELAIN))\n+\t\tshow_progress = 0;\n+\telse if (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 +2862,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":"274167","messageId":"CAPig+cTQdnrHZWJDD6fqu_mQSJQv3oTz9_0Cu8j1aksUiq0FbQ@mail.gmail.com","threadId":"40876","inReplyTo":"1448426165-5139-1-git-send-email-eantoranz@gmail.com","subject":"Re: [PATCH v5] blame: add support for --[no-]progress option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-08T08:22:32Z","receivedAt":"2015-12-08T08:22:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 24, 2015 at 11:36 PM, Edmundo Carmona Antoranz\n<eantoranz@gmail.com> wrote:\n> * created struct progress_info in builtin/blame.c\n>   this struct holds the information used to display progress so\n>   that only one additional parameter is passed to\n>   found_guilty_entry().\n\nCommit messages typically are composed of proper sentences and\nparagraphs rather than bullets points. Also, write in imperative mood:\n\"Create progress_info...\"\n\nIn this case, though, the information in this bullet point isn't all\nthat interesting for the commit message since it's a detail of\nimplementation easily gleaned by reading the patch itself. There's\nnothing particularly tricky about it, thus it doesn't really deserve\nto be called out as special in the commit message.\n\nWhat might be more interesting and deserve mention in the commit\nmessage is how this new option interacts with porcelain and\nincremental modes and why the choice was made.\n\nMore below...\n\n> * --[no-]progress option is also inherited by git-annotate.\n>\n> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>\n> ---\n> diff --git a/Documentation/blame-options.txt 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. Progress information won't be displayed if using\n> +       `--porcelain` or `--incremental`.\n\nIs silently ignoring --progress a good idea when combined with\n--porcelain or --incremental, or would it be better to error out with\na \"mutually exclusive options\" diagnostic? (Genuine question.)\n\n> diff --git a/Documentation/git-blame.txt 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>             [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]\n> -           [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>] [--] <file>\n> +           [--[no-]progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>]\n\nHmm, is [--[no-]progress] common in Git documentation? (Genuine\nquestion.) For the synopsis, I'd have expected to see only\n[--progress] and leave it to the more detailed description of the\noption to mention the possibility of negation (but I haven't\ndouble-checked other documentation, so my expectation may be skewed).\n\n>  DESCRIPTION\n>  -----------\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> @@ -127,6 +129,11 @@ struct origin {\n> +struct progress_info {\n> +       struct progress *progress;\n> +       int blamed_lines;\n> +};\n> @@ -1746,7 +1753,8 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)\n> -static void found_guilty_entry(struct blame_entry *ent)\n> +static void found_guilty_entry(struct blame_entry *ent,\n> +                          struct progress_info *pi)\n> @@ -1758,6 +1766,8 @@ static void found_guilty_entry(struct blame_entry *ent)\n> +       if (pi)\n> +               display_progress(pi->progress, pi->blamed_lines += ent->num_lines);\n\nThis is updating of 'blamed_lines' is rather subtle and easily\noverlooked. Splitting it out as a separate statement could improve\nreadability:\n\n    pi->blamed_lines += ent->num_lines;\n    display_progress(pi->progress, pi->blamed_lines);\n\n>  }\n> @@ -1768,6 +1778,16 @@ static void assign_blame(struct scoreboard *sb, int opt)\n>  {\n>         struct rev_info *revs = sb->revs;\n>         struct commit *commit = prio_queue_get(&sb->commits);\n> +       struct progress_info *pi = NULL;\n> +\n> +       if (show_progress) {\n> +               pi = malloc(sizeof(*pi));\n> +               if (pi) {\n\nxmalloc(), unlike malloc(), will die() upon allocation failure which\nwould allow you to avoid the \"if (pi)\" conditional.\n\n> +                       pi->progress = start_progress_delay(_(\"Blaming lines\"),\n> +                                                           sb->num_lines, 50, 1);\n> +                       pi->blamed_lines = 0;\n> +               }\n> +       }\n>\n>         while (commit) {\n>                 struct blame_entry *ent;\n> @@ -1809,7 +1829,7 @@ static void assign_blame(struct scoreboard *sb, int opt)\n>                         suspect->guilty = 1;\n>                         for (;;) {\n>                                 struct blame_entry *next = ent->next;\n> -                               found_guilty_entry(ent);\n> +                               found_guilty_entry(ent, pi);\n>                                 if (next) {\n>                                         ent = next;\n>                                         continue;\n> @@ -1825,6 +1845,11 @@ static void assign_blame(struct scoreboard *sb, int opt)\n>                 if (DEBUG) /* sanity */\n>                         sanity_check_refcnt(sb);\n>         }\n> +\n> +       if (pi) {\n> +               stop_progress(&pi->progress);\n> +               free(pi);\n> +       };\n\nStyle: drop semi-colon following closing brace\n\nOverall, use of malloc/free for the progress_info struct seems to\nmakes the code more complex rather than less. It's not clear why you\ndon't just use a 'struct progress_info' directly, which would lift the\nburden of freeing the allocated structure, and allow you to drop the\nconditional around stop_progress() since NULL progress is accepted. In\nother words, something like this (completely untested):\n\n    struct progress_info pi = { NULL, 0 };\n    if (show_progress) {\n        pi.progress = start_progress_delay(...);\n    ...\n    found_guilty_entry(ent, &pi);\n    ...\n    stop_progress(&pi.progress)\n\nwhich is more concise and less likely to leak resources. The code\nwithin found_guilty_entry() is also simplified since you can drop the\n\"if (pi)\" conditional entirely. This works because it's safe to call\ndisplay progress with NULL):\n\n    pi->blamed_lines += ent->num_lines;\n    display_progress(pi->progress, pi->blamed_lines);\n\n>  }\n>\n>  static const char *format_time(unsigned long time, const char *tz_str,\n> @@ -2579,6 +2606,11 @@ parse_done:\n>         DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);\n>         argc = parse_options_end(&ctx);\n>\n> +       if (incremental || (output_option & OUTPUT_PORCELAIN))\n> +               show_progress = 0;\n> +       else if (show_progress < 0)\n> +               show_progress = isatty(2);\n> +\n"},{"id":"274226","messageId":"CAOc6etYM5NA8sPw49yioa9u3SwvDB-w4mcWzrLaR3FkFdNGoJA@mail.gmail.com","threadId":"40876","inReplyTo":"CAPig+cTQdnrHZWJDD6fqu_mQSJQv3oTz9_0Cu8j1aksUiq0FbQ@mail.gmail.com","subject":"Re: [PATCH v5] blame: add support for --[no-]progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-12-10T04:20:09Z","receivedAt":"2015-12-10T04:20:09Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Hey, Eric!\n\nOn Tue, Dec 8, 2015 at 2:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Nov 24, 2015 at 11:36 PM, Edmundo Carmona Antoranz\n> <eantoranz@gmail.com> wrote:\n>> * created struct progress_info in builtin/blame.c\n>>   this struct holds the information used to display progress so\n>>   that only one additional parameter is passed to\n>>   found_guilty_entry().\n>\n> Commit messages typically are composed of proper sentences and\n> paragraphs rather than bullets points. Also, write in imperative mood:\n> \"Create progress_info...\"\n>\n> In this case, though, the information in this bullet point isn't all\n> that interesting for the commit message since it's a detail of\n> implementation easily gleaned by reading the patch itself. There's\n> nothing particularly tricky about it, thus it doesn't really deserve\n> to be called out as special in the commit message.\n>\n> What might be more interesting and deserve mention in the commit\n> message is how this new option interacts with porcelain and\n> incremental modes and why the choice was made.\n>\n> More below...\n\nOk\n\n>\n>> * --[no-]progress option is also inherited by git-annotate.\n>>\n>> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>\n>> ---\n>> diff --git a/Documentation/blame-options.txt 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. Progress information won't be displayed if using\n>> +       `--porcelain` or `--incremental`.\n>\n> Is silently ignoring --progress a good idea when combined with\n> --porcelain or --incremental, or would it be better to error out with\n> a \"mutually exclusive options\" diagnostic? (Genuine question.)\n>\n\nI think it makes sense (and so it's documented so people can know what\ncould be going on but detecting mutually exclusive options and\nerroring out could also make sense. Who could give some insight?\n\n>> diff --git a/Documentation/git-blame.txt 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>>             [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]\n>> -           [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>] [--] <file>\n>> +           [--[no-]progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>]\n>\n> Hmm, is [--[no-]progress] common in Git documentation? (Genuine\n> question.) For the synopsis, I'd have expected to see only\n> [--progress] and leave it to the more detailed description of the\n> option to mention the possibility of negation (but I haven't\n> double-checked other documentation, so my expectation may be skewed).\n>\n\nHmmm.... hadn't noticed that. Will remove it from this part.\n\n>>  DESCRIPTION\n>>  -----------\n>> diff --git a/builtin/blame.c b/builtin/blame.c\n>> @@ -127,6 +129,11 @@ struct origin {\n>> +struct progress_info {\n>> +       struct progress *progress;\n>> +       int blamed_lines;\n>> +};\n>> @@ -1746,7 +1753,8 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)\n>> -static void found_guilty_entry(struct blame_entry *ent)\n>> +static void found_guilty_entry(struct blame_entry *ent,\n>> +                          struct progress_info *pi)\n>> @@ -1758,6 +1766,8 @@ static void found_guilty_entry(struct blame_entry *ent)\n>> +       if (pi)\n>> +               display_progress(pi->progress, pi->blamed_lines += ent->num_lines);\n>\n> This is updating of 'blamed_lines' is rather subtle and easily\n> overlooked. Splitting it out as a separate statement could improve\n> readability:\n>\n>     pi->blamed_lines += ent->num_lines;\n>     display_progress(pi->progress, pi->blamed_lines);\n>\n\nOk.\n\n>>  }\n>> @@ -1768,6 +1778,16 @@ static void assign_blame(struct scoreboard *sb, int opt)\n>>  {\n>>         struct rev_info *revs = sb->revs;\n>>         struct commit *commit = prio_queue_get(&sb->commits);\n>> +       struct progress_info *pi = NULL;\n>> +\n>> +       if (show_progress) {\n>> +               pi = malloc(sizeof(*pi));\n>> +               if (pi) {\n>\n> xmalloc(), unlike malloc(), will die() upon allocation failure which\n> would allow you to avoid the \"if (pi)\" conditional.\n>\n\nOk.\n\n>> +                       pi->progress = start_progress_delay(_(\"Blaming lines\"),\n>> +                                                           sb->num_lines, 50, 1);\n>> +                       pi->blamed_lines = 0;\n>> +               }\n>> +       }\n>>\n>>         while (commit) {\n>>                 struct blame_entry *ent;\n>> @@ -1809,7 +1829,7 @@ static void assign_blame(struct scoreboard *sb, int opt)\n>>                         suspect->guilty = 1;\n>>                         for (;;) {\n>>                                 struct blame_entry *next = ent->next;\n>> -                               found_guilty_entry(ent);\n>> +                               found_guilty_entry(ent, pi);\n>>                                 if (next) {\n>>                                         ent = next;\n>>                                         continue;\n>> @@ -1825,6 +1845,11 @@ static void assign_blame(struct scoreboard *sb, int opt)\n>>                 if (DEBUG) /* sanity */\n>>                         sanity_check_refcnt(sb);\n>>         }\n>> +\n>> +       if (pi) {\n>> +               stop_progress(&pi->progress);\n>> +               free(pi);\n>> +       };\n>\n> Style: drop semi-colon following closing brace\n>\n\nOk.\n\n> Overall, use of malloc/free for the progress_info struct seems to\n> makes the code more complex rather than less. It's not clear why you\n> don't just use a 'struct progress_info' directly, which would lift the\n> burden of freeing the allocated structure, and allow you to drop the\n> conditional around stop_progress() since NULL progress is accepted. In\n> other words, something like this (completely untested):\n>\n>     struct progress_info pi = { NULL, 0 };\n\nI like it! I'll adjust to not use the pointer.\n\n>     if (show_progress) {\n>         pi.progress = start_progress_delay(...);\n>     ...\n>     found_guilty_entry(ent, &pi);\n>     ...\n>     stop_progress(&pi.progress)\n>\n> which is more concise and less likely to leak resources. The code\n> within found_guilty_entry() is also simplified since you can drop the\n> \"if (pi)\" conditional entirely. This works because it's safe to call\n> display progress with NULL):\n>\n>     pi->blamed_lines += ent->num_lines;\n>     display_progress(pi->progress, pi->blamed_lines);\n>\n>>  }\n>>\n>>  static const char *format_time(unsigned long time, const char *tz_str,\n>> @@ -2579,6 +2606,11 @@ parse_done:\n>>         DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);\n>>         argc = parse_options_end(&ctx);\n>>\n>> +       if (incremental || (output_option & OUTPUT_PORCELAIN))\n>> +               show_progress = 0;\n>> +       else if (show_progress < 0)\n>> +               show_progress = isatty(2);\n>> +\n\n\nOfficially, I think not a single line of the patch survived. :-D\n\nThanks, man... I'll go through the stuff and let's wait for feedback\nabout the detection/error-out in case of using \"mutually exclusive\"\noptions.\n\nCheers!\n"},{"id":"274228","messageId":"CAPig+cS8BwYboyRcEGPkGQm3nNvUrz9rf8HNuFyEdF83jXvi6Q@mail.gmail.com","threadId":"40876","inReplyTo":"CAOc6etYM5NA8sPw49yioa9u3SwvDB-w4mcWzrLaR3FkFdNGoJA@mail.gmail.com","subject":"Re: [PATCH v5] blame: add support for --[no-]progress option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-10T07:43:14Z","receivedAt":"2015-12-10T07:43:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 9, 2015 at 11:20 PM, Edmundo Carmona Antoranz\n<eantoranz@gmail.com> wrote:\n> On Tue, Dec 8, 2015 at 2:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Tue, Nov 24, 2015 at 11:36 PM, Edmundo Carmona Antoranz\n>> <eantoranz@gmail.com> wrote:\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. Progress information won't be displayed if using\n>>> +       `--porcelain` or `--incremental`.\n>>\n>> Is silently ignoring --progress a good idea when combined with\n>> --porcelain or --incremental, or would it be better to error out with\n>> a \"mutually exclusive options\" diagnostic? (Genuine question.)\n>\n> I think it makes sense (and so it's documented so people can know what\n> could be going on but detecting mutually exclusive options and\n> erroring out could also make sense. Who could give some insight?\n\nIt's a bit difficult to imagine a case when --progress would make\nsense with --porcelain or --incremental, so I'd probably opt for\nemitting a \"mutually exclusive\" diagnostic to inform the user\nexplicitly that the combination is nonsensical.\n\n>> Overall, use of malloc/free for the progress_info struct seems to\n>> makes the code more complex rather than less. It's not clear why you\n>> don't just use a 'struct progress_info' directly, which would lift the\n>> burden of freeing the allocated structure, and allow you to drop the\n>> conditional around stop_progress() since NULL progress is accepted. In\n>> other words, something like this (completely untested):\n>>\n>>     struct progress_info pi = { NULL, 0 };\n>\n> I like it! I'll adjust to not use the pointer.\n>\n>>     if (show_progress) {\n>>         pi.progress = start_progress_delay(...);\n>>     ...\n>>     found_guilty_entry(ent, &pi);\n>>     ...\n>>     stop_progress(&pi.progress)\n>>\n>> which is more concise and less likely to leak resources. The code\n>> within found_guilty_entry() is also simplified since you can drop the\n>> \"if (pi)\" conditional entirely. This works because it's safe to call\n>> display progress with NULL):\n>>\n>>     pi->blamed_lines += ent->num_lines;\n>>     display_progress(pi->progress, pi->blamed_lines);\n>\n> Officially, I think not a single line of the patch survived. :-D\n\nAdding Helped-by: footers before your Signed-off-by: is a way to\nacknowledge those who've helped shape the patch if you feel that such\nassistance had significant value.\n"}]}