[PATCH v5] blame: add support for --[no-]progress option

[PATCH v5] blame: add support for --[no-]progress option

[Edmundo Carmona Antoranz]

* created struct progress_info in builtin/blame.c
  this struct holds the information used to display progress so
  that only one additional parameter is passed to
  found_guilty_entry().

* --[no-]progress option is also inherited by git-annotate.

Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
---
 Documentation/blame-options.txt |  7 +++++++
 Documentation/git-blame.txt     |  3 ++-
 builtin/blame.c                 | 40 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 760eab7..ef642b9 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -69,6 +69,13 @@ include::line-range-format.txt[]
 	iso format is used. For supported values, see the discussion
 	of the --date option at linkgit:git-log[1].
 
+--[no-]progress::
+	Progress status is reported on the standard error stream
+	by default when it is attached to a terminal. This flag
+	enables progress reporting even if not attached to a
+	terminal. Progress information won't be displayed if using
+	`--porcelain` or `--incremental`.
+
 -M|<num>|::
 	Detect moved or copied lines within a file. When a commit
 	moves or copies a block of lines (e.g. the original file
diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index e6e947c..b0154c2 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -10,7 +10,8 @@ SYNOPSIS
 [verse]
 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]
 	    [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]
-	    [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>] [--] <file>
+	    [--[no-]progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>]
+	    [--] <file>
 
 DESCRIPTION
 -----------
diff --git a/builtin/blame.c b/builtin/blame.c
index 83612f5..954d32c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -28,6 +28,7 @@
 #include "line-range.h"
 #include "line-log.h"
 #include "dir.h"
+#include "progress.h"
 
 static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
 
@@ -50,6 +51,7 @@ static int incremental;
 static int xdl_opts;
 static int abbrev = -1;
 static int no_whole_file_rename;
+static int show_progress;
 
 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -127,6 +129,11 @@ struct origin {
 	char path[FLEX_ARRAY];
 };
 
+struct progress_info {
+	struct progress *progress;
+	int blamed_lines;
+};
+
 static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen,
 		      xdl_emit_hunk_consume_func_t hunk_func, void *cb_data)
 {
@@ -1746,7 +1753,8 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)
  * The blame_entry is found to be guilty for the range.
  * Show it in incremental output.
  */
-static void found_guilty_entry(struct blame_entry *ent)
+static void found_guilty_entry(struct blame_entry *ent,
+			   struct progress_info *pi)
 {
 	if (incremental) {
 		struct origin *suspect = ent->suspect;
@@ -1758,6 +1766,8 @@ static void found_guilty_entry(struct blame_entry *ent)
 		write_filename_info(suspect->path);
 		maybe_flush_or_die(stdout, "stdout");
 	}
+	if (pi)
+		display_progress(pi->progress, pi->blamed_lines += ent->num_lines);
 }
 
 /*
@@ -1768,6 +1778,16 @@ static void assign_blame(struct scoreboard *sb, int opt)
 {
 	struct rev_info *revs = sb->revs;
 	struct commit *commit = prio_queue_get(&sb->commits);
+	struct progress_info *pi = NULL;
+
+	if (show_progress) {
+		pi = malloc(sizeof(*pi));
+		if (pi) {
+			pi->progress = start_progress_delay(_("Blaming lines"),
+			                                    sb->num_lines, 50, 1);
+			pi->blamed_lines = 0;
+		}
+	}
 
 	while (commit) {
 		struct blame_entry *ent;
@@ -1809,7 +1829,7 @@ static void assign_blame(struct scoreboard *sb, int opt)
 			suspect->guilty = 1;
 			for (;;) {
 				struct blame_entry *next = ent->next;
-				found_guilty_entry(ent);
+				found_guilty_entry(ent, pi);
 				if (next) {
 					ent = next;
 					continue;
@@ -1825,6 +1845,11 @@ static void assign_blame(struct scoreboard *sb, int opt)
 		if (DEBUG) /* sanity */
 			sanity_check_refcnt(sb);
 	}
+
+	if (pi) {
+		stop_progress(&pi->progress);
+		free(pi);
+	};
 }
 
 static const char *format_time(unsigned long time, const char *tz_str,
@@ -2520,6 +2545,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('b', NULL, &blank_boundary, N_("Show blank SHA-1 for boundary commits (Default: off)")),
 		OPT_BOOL(0, "root", &show_root, N_("Do not treat root commits as boundaries (Default: off)")),
 		OPT_BOOL(0, "show-stats", &show_stats, N_("Show work cost statistics")),
+		OPT_BOOL(0, "progress", &show_progress, N_("Force progress reporting")),
 		OPT_BIT(0, "score-debug", &output_option, N_("Show output score for blame entries"), OUTPUT_SHOW_SCORE),
 		OPT_BIT('f', "show-name", &output_option, N_("Show original filename (Default: auto)"), OUTPUT_SHOW_NAME),
 		OPT_BIT('n', "show-number", &output_option, N_("Show original linenumber (Default: off)"), OUTPUT_SHOW_NUMBER),
@@ -2555,6 +2581,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 
 	save_commit_buffer = 0;
 	dashdash_pos = 0;
+	show_progress = -1;
 
 	parse_options_start(&ctx, argc, argv, prefix, options,
 			    PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
@@ -2579,6 +2606,11 @@ parse_done:
 	DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
 	argc = parse_options_end(&ctx);
 
+	if (incremental || (output_option & OUTPUT_PORCELAIN))
+		show_progress = 0;
+	else if (show_progress < 0)
+		show_progress = isatty(2);
+
 	if (0 < abbrev)
 		/* one more abbrev length is needed for the boundary commit */
 		abbrev++;
@@ -2830,11 +2862,11 @@ parse_done:
 
 	read_mailmap(&mailmap, NULL);
 
+	assign_blame(&sb, opt);
+
 	if (!incremental)
 		setup_pager();
 
-	assign_blame(&sb, opt);
-
 	free(final_commit_name);
 
 	if (incremental)
-- 
2.6.2

Re: [PATCH v5] blame: add support for --[no-]progress option

[Eric Sunshine]

On Tue, Nov 24, 2015 at 11:36 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> * created struct progress_info in builtin/blame.c
>   this struct holds the information used to display progress so
>   that only one additional parameter is passed to
>   found_guilty_entry().

Commit messages typically are composed of proper sentences and
paragraphs rather than bullets points. Also, write in imperative mood:
"Create progress_info..."

In this case, though, the information in this bullet point isn't all
that interesting for the commit message since it's a detail of
implementation easily gleaned by reading the patch itself. There's
nothing particularly tricky about it, thus it doesn't really deserve
to be called out as special in the commit message.

What might be more interesting and deserve mention in the commit
message is how this new option interacts with porcelain and
incremental modes and why the choice was made.

More below...

> * --[no-]progress option is also inherited by git-annotate.
>
> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
> ---
> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
> @@ -69,6 +69,13 @@ include::line-range-format.txt[]
>         iso format is used. For supported values, see the discussion
>         of the --date option at linkgit:git-log[1].
>
> +--[no-]progress::
> +       Progress status is reported on the standard error stream
> +       by default when it is attached to a terminal. This flag
> +       enables progress reporting even if not attached to a
> +       terminal. Progress information won't be displayed if using
> +       `--porcelain` or `--incremental`.

Is silently ignoring --progress a good idea when combined with
--porcelain or --incremental, or would it be better to error out with
a "mutually exclusive options" diagnostic? (Genuine question.)

> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
> @@ -10,7 +10,8 @@ SYNOPSIS
>  [verse]
>  'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]
>             [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]
> -           [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>] [--] <file>
> +           [--[no-]progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>]

Hmm, is [--[no-]progress] common in Git documentation? (Genuine
question.) For the synopsis, I'd have expected to see only
[--progress] and leave it to the more detailed description of the
option to mention the possibility of negation (but I haven't
double-checked other documentation, so my expectation may be skewed).

>  DESCRIPTION
>  -----------
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -127,6 +129,11 @@ struct origin {
> +struct progress_info {
> +       struct progress *progress;
> +       int blamed_lines;
> +};
> @@ -1746,7 +1753,8 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)
> -static void found_guilty_entry(struct blame_entry *ent)
> +static void found_guilty_entry(struct blame_entry *ent,
> +                          struct progress_info *pi)
> @@ -1758,6 +1766,8 @@ static void found_guilty_entry(struct blame_entry *ent)
> +       if (pi)
> +               display_progress(pi->progress, pi->blamed_lines += ent->num_lines);

This is updating of 'blamed_lines' is rather subtle and easily
overlooked. Splitting it out as a separate statement could improve
readability:

    pi->blamed_lines += ent->num_lines;
    display_progress(pi->progress, pi->blamed_lines);

>  }
> @@ -1768,6 +1778,16 @@ static void assign_blame(struct scoreboard *sb, int opt)
>  {
>         struct rev_info *revs = sb->revs;
>         struct commit *commit = prio_queue_get(&sb->commits);
> +       struct progress_info *pi = NULL;
> +
> +       if (show_progress) {
> +               pi = malloc(sizeof(*pi));
> +               if (pi) {

xmalloc(), unlike malloc(), will die() upon allocation failure which
would allow you to avoid the "if (pi)" conditional.

> +                       pi->progress = start_progress_delay(_("Blaming lines"),
> +                                                           sb->num_lines, 50, 1);
> +                       pi->blamed_lines = 0;
> +               }
> +       }
>
>         while (commit) {
>                 struct blame_entry *ent;
> @@ -1809,7 +1829,7 @@ static void assign_blame(struct scoreboard *sb, int opt)
>                         suspect->guilty = 1;
>                         for (;;) {
>                                 struct blame_entry *next = ent->next;
> -                               found_guilty_entry(ent);
> +                               found_guilty_entry(ent, pi);
>                                 if (next) {
>                                         ent = next;
>                                         continue;
> @@ -1825,6 +1845,11 @@ static void assign_blame(struct scoreboard *sb, int opt)
>                 if (DEBUG) /* sanity */
>                         sanity_check_refcnt(sb);
>         }
> +
> +       if (pi) {
> +               stop_progress(&pi->progress);
> +               free(pi);
> +       };

Style: drop semi-colon following closing brace

Overall, use of malloc/free for the progress_info struct seems to
makes the code more complex rather than less. It's not clear why you
don't just use a 'struct progress_info' directly, which would lift the
burden of freeing the allocated structure, and allow you to drop the
conditional around stop_progress() since NULL progress is accepted. In
other words, something like this (completely untested):

    struct progress_info pi = { NULL, 0 };
    if (show_progress) {
        pi.progress = start_progress_delay(...);
    ...
    found_guilty_entry(ent, &pi);
    ...
    stop_progress(&pi.progress)

which is more concise and less likely to leak resources. The code
within found_guilty_entry() is also simplified since you can drop the
"if (pi)" conditional entirely. This works because it's safe to call
display progress with NULL):

    pi->blamed_lines += ent->num_lines;
    display_progress(pi->progress, pi->blamed_lines);

>  }
>
>  static const char *format_time(unsigned long time, const char *tz_str,
> @@ -2579,6 +2606,11 @@ parse_done:
>         DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
>         argc = parse_options_end(&ctx);
>
> +       if (incremental || (output_option & OUTPUT_PORCELAIN))
> +               show_progress = 0;
> +       else if (show_progress < 0)
> +               show_progress = isatty(2);
> +

Re: [PATCH v5] blame: add support for --[no-]progress option

[Edmundo Carmona Antoranz]

Hey, Eric!

On Tue, Dec 8, 2015 at 2:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Nov 24, 2015 at 11:36 PM, Edmundo Carmona Antoranz
> <eantoranz@gmail.com> wrote:
>> * created struct progress_info in builtin/blame.c
>>   this struct holds the information used to display progress so
>>   that only one additional parameter is passed to
>>   found_guilty_entry().
>
> Commit messages typically are composed of proper sentences and
> paragraphs rather than bullets points. Also, write in imperative mood:
> "Create progress_info..."
>
> In this case, though, the information in this bullet point isn't all
> that interesting for the commit message since it's a detail of
> implementation easily gleaned by reading the patch itself. There's
> nothing particularly tricky about it, thus it doesn't really deserve
> to be called out as special in the commit message.
>
> What might be more interesting and deserve mention in the commit
> message is how this new option interacts with porcelain and
> incremental modes and why the choice was made.
>
> More below...

Ok

>
>> * --[no-]progress option is also inherited by git-annotate.
>>
>> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
>> ---
>> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
>> @@ -69,6 +69,13 @@ include::line-range-format.txt[]
>>         iso format is used. For supported values, see the discussion
>>         of the --date option at linkgit:git-log[1].
>>
>> +--[no-]progress::
>> +       Progress status is reported on the standard error stream
>> +       by default when it is attached to a terminal. This flag
>> +       enables progress reporting even if not attached to a
>> +       terminal. Progress information won't be displayed if using
>> +       `--porcelain` or `--incremental`.
>
> Is silently ignoring --progress a good idea when combined with
> --porcelain or --incremental, or would it be better to error out with
> a "mutually exclusive options" diagnostic? (Genuine question.)
>

I think it makes sense (and so it's documented so people can know what
could be going on but detecting mutually exclusive options and
erroring out could also make sense. Who could give some insight?

>> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
>> @@ -10,7 +10,8 @@ SYNOPSIS
>>  [verse]
>>  'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]
>>             [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]
>> -           [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>] [--] <file>
>> +           [--[no-]progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>]
>
> Hmm, is [--[no-]progress] common in Git documentation? (Genuine
> question.) For the synopsis, I'd have expected to see only
> [--progress] and leave it to the more detailed description of the
> option to mention the possibility of negation (but I haven't
> double-checked other documentation, so my expectation may be skewed).
>

Hmmm.... hadn't noticed that. Will remove it from this part.

>>  DESCRIPTION
>>  -----------
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> @@ -127,6 +129,11 @@ struct origin {
>> +struct progress_info {
>> +       struct progress *progress;
>> +       int blamed_lines;
>> +};
>> @@ -1746,7 +1753,8 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)
>> -static void found_guilty_entry(struct blame_entry *ent)
>> +static void found_guilty_entry(struct blame_entry *ent,
>> +                          struct progress_info *pi)
>> @@ -1758,6 +1766,8 @@ static void found_guilty_entry(struct blame_entry *ent)
>> +       if (pi)
>> +               display_progress(pi->progress, pi->blamed_lines += ent->num_lines);
>
> This is updating of 'blamed_lines' is rather subtle and easily
> overlooked. Splitting it out as a separate statement could improve
> readability:
>
>     pi->blamed_lines += ent->num_lines;
>     display_progress(pi->progress, pi->blamed_lines);
>

Ok.

>>  }
>> @@ -1768,6 +1778,16 @@ static void assign_blame(struct scoreboard *sb, int opt)
>>  {
>>         struct rev_info *revs = sb->revs;
>>         struct commit *commit = prio_queue_get(&sb->commits);
>> +       struct progress_info *pi = NULL;
>> +
>> +       if (show_progress) {
>> +               pi = malloc(sizeof(*pi));
>> +               if (pi) {
>
> xmalloc(), unlike malloc(), will die() upon allocation failure which
> would allow you to avoid the "if (pi)" conditional.
>

Ok.

>> +                       pi->progress = start_progress_delay(_("Blaming lines"),
>> +                                                           sb->num_lines, 50, 1);
>> +                       pi->blamed_lines = 0;
>> +               }
>> +       }
>>
>>         while (commit) {
>>                 struct blame_entry *ent;
>> @@ -1809,7 +1829,7 @@ static void assign_blame(struct scoreboard *sb, int opt)
>>                         suspect->guilty = 1;
>>                         for (;;) {
>>                                 struct blame_entry *next = ent->next;
>> -                               found_guilty_entry(ent);
>> +                               found_guilty_entry(ent, pi);
>>                                 if (next) {
>>                                         ent = next;
>>                                         continue;
>> @@ -1825,6 +1845,11 @@ static void assign_blame(struct scoreboard *sb, int opt)
>>                 if (DEBUG) /* sanity */
>>                         sanity_check_refcnt(sb);
>>         }
>> +
>> +       if (pi) {
>> +               stop_progress(&pi->progress);
>> +               free(pi);
>> +       };
>
> Style: drop semi-colon following closing brace
>

Ok.

> Overall, use of malloc/free for the progress_info struct seems to
> makes the code more complex rather than less. It's not clear why you
> don't just use a 'struct progress_info' directly, which would lift the
> burden of freeing the allocated structure, and allow you to drop the
> conditional around stop_progress() since NULL progress is accepted. In
> other words, something like this (completely untested):
>
>     struct progress_info pi = { NULL, 0 };

I like it! I'll adjust to not use the pointer.

>     if (show_progress) {
>         pi.progress = start_progress_delay(...);
>     ...
>     found_guilty_entry(ent, &pi);
>     ...
>     stop_progress(&pi.progress)
>
> which is more concise and less likely to leak resources. The code
> within found_guilty_entry() is also simplified since you can drop the
> "if (pi)" conditional entirely. This works because it's safe to call
> display progress with NULL):
>
>     pi->blamed_lines += ent->num_lines;
>     display_progress(pi->progress, pi->blamed_lines);
>
>>  }
>>
>>  static const char *format_time(unsigned long time, const char *tz_str,
>> @@ -2579,6 +2606,11 @@ parse_done:
>>         DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
>>         argc = parse_options_end(&ctx);
>>
>> +       if (incremental || (output_option & OUTPUT_PORCELAIN))
>> +               show_progress = 0;
>> +       else if (show_progress < 0)
>> +               show_progress = isatty(2);
>> +


Officially, I think not a single line of the patch survived. :-D

Thanks, man... I'll go through the stuff and let's wait for feedback
about the detection/error-out in case of using "mutually exclusive"
options.

Cheers!

Re: [PATCH v5] blame: add support for --[no-]progress option

[Eric Sunshine]

On Wed, Dec 9, 2015 at 11:20 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> On Tue, Dec 8, 2015 at 2:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Nov 24, 2015 at 11:36 PM, Edmundo Carmona Antoranz
>> <eantoranz@gmail.com> wrote:
>>> +--[no-]progress::
>>> +       Progress status is reported on the standard error stream
>>> +       by default when it is attached to a terminal. This flag
>>> +       enables progress reporting even if not attached to a
>>> +       terminal. Progress information won't be displayed if using
>>> +       `--porcelain` or `--incremental`.
>>
>> Is silently ignoring --progress a good idea when combined with
>> --porcelain or --incremental, or would it be better to error out with
>> a "mutually exclusive options" diagnostic? (Genuine question.)
>
> I think it makes sense (and so it's documented so people can know what
> could be going on but detecting mutually exclusive options and
> erroring out could also make sense. Who could give some insight?

It's a bit difficult to imagine a case when --progress would make
sense with --porcelain or --incremental, so I'd probably opt for
emitting a "mutually exclusive" diagnostic to inform the user
explicitly that the combination is nonsensical.

>> Overall, use of malloc/free for the progress_info struct seems to
>> makes the code more complex rather than less. It's not clear why you
>> don't just use a 'struct progress_info' directly, which would lift the
>> burden of freeing the allocated structure, and allow you to drop the
>> conditional around stop_progress() since NULL progress is accepted. In
>> other words, something like this (completely untested):
>>
>>     struct progress_info pi = { NULL, 0 };
>
> I like it! I'll adjust to not use the pointer.
>
>>     if (show_progress) {
>>         pi.progress = start_progress_delay(...);
>>     ...
>>     found_guilty_entry(ent, &pi);
>>     ...
>>     stop_progress(&pi.progress)
>>
>> which is more concise and less likely to leak resources. The code
>> within found_guilty_entry() is also simplified since you can drop the
>> "if (pi)" conditional entirely. This works because it's safe to call
>> display progress with NULL):
>>
>>     pi->blamed_lines += ent->num_lines;
>>     display_progress(pi->progress, pi->blamed_lines);
>
> Officially, I think not a single line of the patch survived. :-D

Adding Helped-by: footers before your Signed-off-by: is a way to
acknowledge those who've helped shape the patch if you feel that such
assistance had significant value.