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

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

[Edmundo Carmona Antoranz]

--progress can't be used with --incremental or
 porcelain formats.

git-annotate inherits the option as well

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
---
 Documentation/blame-options.txt |  7 +++++++
 Documentation/git-blame.txt     |  3 ++-
 builtin/blame.c                 | 35 +++++++++++++++++++++++++++++++----
 3 files changed, 40 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..ba54175 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>
+	    [--progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>]
+	    [--] <file>
 
 DESCRIPTION
 -----------
diff --git a/builtin/blame.c b/builtin/blame.c
index 2afe828..be5d73d 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");
 	}
+	pi->blamed_lines += ent->num_lines;
+	display_progress(pi->progress, pi->blamed_lines);
 }
 
 /*
@@ -1768,6 +1778,11 @@ 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, 0 };
+
+	if (show_progress)
+		pi.progress = start_progress_delay(_("Blaming lines"),
+		                                   sb->num_lines, 50, 1);
 
 	while (commit) {
 		struct blame_entry *ent;
@@ -1809,7 +1824,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 +1840,9 @@ static void assign_blame(struct scoreboard *sb, int opt)
 		if (DEBUG) /* sanity */
 			sanity_check_refcnt(sb);
 	}
+
+	if (pi.progress)
+		stop_progress(&pi.progress);
 }
 
 static const char *format_time(unsigned long time, const char *tz_str,
@@ -2520,6 +2538,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 +2574,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 +2599,13 @@ parse_done:
 	DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
 	argc = parse_options_end(&ctx);
 
+	if (incremental || (output_option & OUTPUT_PORCELAIN)) {
+		if (show_progress > 0)
+			die("--progress can't be used with --incremental or porcelain formats");
+		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++;
@@ -2828,11 +2855,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 v6] blame: add support for --[no-]progress option

[Edmundo Carmona Antoranz]

On Sat, Dec 12, 2015 at 5:57 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> +       if (incremental || (output_option & OUTPUT_PORCELAIN)) {
> +               if (show_progress > 0)
> +                       die("--progress can't be used with --incremental or porcelain formats");
> +               show_progress = 0;
> +       } else if (show_progress < 0)
> +               show_progress = isatty(2);
> +

I think I took care of all the comments from Eric gracefully. Hope
that block is ok for detection of mutually exclusivity related to
--progress.

Cheers!

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

[Eric Sunshine]

On Sat, Dec 12, 2015 at 6:57 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> --progress can't be used with --incremental or
>  porcelain formats.
>
> git-annotate inherits the option as well
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
> ---

Right here below the "---" line would be a good place to explain what
changed since the previous version. As an aid for reviewers, it's also
helpful to provide a link to the previous round, like this[1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/281677

>  Documentation/blame-options.txt |  7 +++++++
>  Documentation/git-blame.txt     |  3 ++-
>  builtin/blame.c                 | 35 +++++++++++++++++++++++++++++++----
>  3 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
> @@ -69,6 +69,13 @@ include::line-range-format.txt[]
> +--[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`.

The actual implementation (below) actively forbids --progress with
--porcelain or --incremental, so the final sentence is misleading.
Perhaps say instead that "--progress is incompatible with --porcelain
and --incremental".

More below...

> 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;
> +};
> +
> @@ -1758,6 +1766,8 @@ static void found_guilty_entry(struct blame_entry *ent)
>                 write_filename_info(suspect->path);
>                 maybe_flush_or_die(stdout, "stdout");
>         }
> +       pi->blamed_lines += ent->num_lines;
> +       display_progress(pi->progress, pi->blamed_lines);
>  }
>
>  /*
> @@ -1768,6 +1778,11 @@ 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, 0 };
> +
> +       if (show_progress)
> +               pi.progress = start_progress_delay(_("Blaming lines"),
> +                                                  sb->num_lines, 50, 1);
>
>         while (commit) {
>                 struct blame_entry *ent;
> @@ -1809,7 +1824,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 +1840,9 @@ static void assign_blame(struct scoreboard *sb, int opt)
>                 if (DEBUG) /* sanity */
>                         sanity_check_refcnt(sb);
>         }
> +
> +       if (pi.progress)
> +               stop_progress(&pi.progress);

As noted in the v5 review[2], stop_progress() itself handles NULL
'struct progress' gracefully, so the 'if (pi.progress)' conditional is
unnecessary, thus the code can be simplified further to merely:

    stop_progress(&pi.progress);

Other than this nit, the implementation is now much cleaner and easier
to follow.

[2]: http://article.gmane.org/gmane.comp.version-control.git/282150

>  }
>
>  static const char *format_time(unsigned long time, const char *tz_str,
> @@ -2555,6 +2574,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 +2599,13 @@ parse_done:
>         DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
>         argc = parse_options_end(&ctx);
>
> +       if (incremental || (output_option & OUTPUT_PORCELAIN)) {
> +               if (show_progress > 0)
> +                       die("--progress can't be used with --incremental or porcelain formats");
> +               show_progress = 0;
> +       } else if (show_progress < 0)
> +               show_progress = isatty(2);

The 'show_progress = 0' seems unnecessary. What if you did something
like this instead?

    if (show_progress > 0 && (incremental ||
            (output_option & OUTPUT_PORCELAIN)))
        die("--progress can't be used with...");
    else if (show_progress < 0)
        show_progress = isatty(2);

>         if (0 < abbrev)
>                 /* one more abbrev length is needed for the boundary commit */
>                 abbrev++;

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

[Edmundo Carmona Antoranz]

On Sat, Dec 12, 2015 at 6:17 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Right here below the "---" line would be a good place to explain what
> changed since the previous version. As an aid for reviewers, it's also
> helpful to provide a link to the previous round, like this[1].
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/281677
>

Ok... learning the tricks.

>> @@ -69,6 +69,13 @@ include::line-range-format.txt[]
>> +--[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`.
>
> The actual implementation (below) actively forbids --progress with
> --porcelain or --incremental, so the final sentence is misleading.
> Perhaps say instead that "--progress is incompatible with --porcelain
> and --incremental".
>
> More below...
>

Absolutely right.... didn't reflect the 'policy change' in the
documentation. Will update for next patch version.

>> +
>> +       if (pi.progress)
>> +               stop_progress(&pi.progress);
>
> As noted in the v5 review[2], stop_progress() itself handles NULL
> 'struct progress' gracefully, so the 'if (pi.progress)' conditional is
> unnecessary, thus the code can be simplified further to merely:
>
>     stop_progress(&pi.progress);
>

You are right!

>
> The 'show_progress = 0' seems unnecessary. What if you did something
> like this instead?
>
>     if (show_progress > 0 && (incremental ||
>             (output_option & OUTPUT_PORCELAIN)))
>         die("--progress can't be used with...");
>     else if (show_progress < 0)
>         show_progress = isatty(2);
>
>>         if (0 < abbrev)
>>                 /* one more abbrev length is needed for the boundary commit */
>>                 abbrev++;

Because, if the user didn't provide --[no-]progress option, then the
value in show_progress will move forward being -1 and then in
assign_blame, there will be progress output if you chose --incremental
or porcelain. So, if you chose --incremental or porcelain, we better
set the value to 0 to make sure there will be _no_ progress. Agree?

Cheers!

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

[Edmundo Carmona Antoranz]

On Sat, Dec 12, 2015 at 6:30 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
>>
>> The 'show_progress = 0' seems unnecessary. What if you did something
>> like this instead?
>>
>>     if (show_progress > 0 && (incremental ||
>>             (output_option & OUTPUT_PORCELAIN)))
>>         die("--progress can't be used with...");
>>     else if (show_progress < 0)
>>         show_progress = isatty(2);
>>
>>>         if (0 < abbrev)
>>>                 /* one more abbrev length is needed for the boundary commit */
>>>                 abbrev++;
>
> Because, if the user didn't provide --[no-]progress option, then the
> value in show_progress will move forward being -1 and then in
> assign_blame, there will be progress output if you chose --incremental
> or porcelain. So, if you chose --incremental or porcelain, we better
> set the value to 0 to make sure there will be _no_ progress. Agree?


Hmmmm.... if the code in assign_blame changed to this, it would be
possible to allow the -1 to go through:

if (show_progress > 0)
    pi.progress = start_progress_delay(_("Blaming lines"),
sb->num_lines, 50, 1);

But then I think it would be more 'concise' if we had the value set to
0/1 instead of expecting to see a possible value of -1 there (or
anywhere else) after progressing if progress will be shown or not in
the piece of code we are chatting about.

Comments?

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

[Eric Sunshine]

On Sat, Dec 12, 2015 at 7:36 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> On Sat, Dec 12, 2015 at 6:30 PM, Edmundo Carmona Antoranz
> <eantoranz@gmail.com> wrote:
> Hmmmm.... if the code in assign_blame changed to this, it would be
> possible to allow the -1 to go through:
>
> if (show_progress > 0)
>     pi.progress = start_progress_delay(_("Blaming lines"),
> sb->num_lines, 50, 1);
>
> But then I think it would be more 'concise' if we had the value set to
> 0/1 instead of expecting to see a possible value of -1 there (or
> anywhere else) after progressing if progress will be shown or not in
> the piece of code we are chatting about.

The name "show_progress" does read like a boolean rather than a
tristate, so making sure its value is 0 or 1 after option processing
(as your current patch does) is probably the best way to go. I don't
otherwise feel strongly about it.

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

[Eric Sunshine]

On Sat, Dec 12, 2015 at 7:30 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> On Sat, Dec 12, 2015 at 6:17 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> The 'show_progress = 0' seems unnecessary. What if you did something
>> like this instead?
>>
>>     if (show_progress > 0 && (incremental ||
>>             (output_option & OUTPUT_PORCELAIN)))
>>         die("--progress can't be used with...");
>>     else if (show_progress < 0)
>>         show_progress = isatty(2);
>>
>>>         if (0 < abbrev)
>>>                 /* one more abbrev length is needed for the boundary commit */
>>>                 abbrev++;
>
> Because, if the user didn't provide --[no-]progress option, then the
> value in show_progress will move forward being -1 and then in
> assign_blame, there will be progress output if you chose --incremental
> or porcelain. So, if you chose --incremental or porcelain, we better
> set the value to 0 to make sure there will be _no_ progress. Agree?

Yeah, I was thinking of that and had the correct interpretation in
mind when reading the code, but then blocked it out of my brain for
some reason when actually composing the response.

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

[Edmundo Carmona Antoranz]

On Sat, Dec 12, 2015 at 6:37 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>
>> Because, if the user didn't provide --[no-]progress option, then the
>> value in show_progress will move forward being -1 and then in
>> assign_blame, there will be progress output if you chose --incremental
>> or porcelain. So, if you chose --incremental or porcelain, we better
>> set the value to 0 to make sure there will be _no_ progress. Agree?
>
> Yeah, I was thinking of that and had the correct interpretation in
> mind when reading the code, but then blocked it out of my brain for
> some reason when actually composing the response.

Good! So, the only things to modify would be:
- documentation to reflect new policy
- no need to check for show_progress to ask to finish up struct
progress instance.

Let's give some time to allow for more comments before my next patch
version.... so, say, 5 minutes.