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

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

[Edmundo Carmona Antoranz]

Will also affect annotate

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

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 760eab7..43f4f08 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.
+
+
 -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..2e63397 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
 -----------
@@ -88,6 +89,12 @@ include::blame-options.txt[]
 	abbreviated object name, use <n>+1 digits. Note that 1 column
 	is used for a caret to mark the boundary commit.
 
+--[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.
+
 
 THE PORCELAIN FORMAT
 --------------------
diff --git a/builtin/blame.c b/builtin/blame.c
index 83612f5..31477d8 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;
@@ -1760,14 +1762,28 @@ static void found_guilty_entry(struct blame_entry *ent)
 	}
 }
 
+static int count_blamed_lines(struct scoreboard *sb)
+{
+	int counter = 0;
+	for (struct blame_entry * entry = sb->ent; entry; entry = entry->next)
+		counter += entry->num_lines;
+	return counter;
+}
+
 /*
  * The main loop -- while we have blobs with lines whose true origin
  * is still unknown, pick one blob, and allow its lines to pass blames
  * to its parents. */
-static void assign_blame(struct scoreboard *sb, int opt)
+static void assign_blame(struct scoreboard *sb, int opt, int show_progress)
 {
 	struct rev_info *revs = sb->revs;
 	struct commit *commit = prio_queue_get(&sb->commits);
+	struct progress * progress = NULL;
+	int blamed_lines = -1;
+
+	if (show_progress) {
+		progress = start_progress(_("Blaming lines"), sb->num_lines);
+	}
 
 	while (commit) {
 		struct blame_entry *ent;
@@ -1822,9 +1838,21 @@ static void assign_blame(struct scoreboard *sb, int opt)
 		}
 		origin_decref(suspect);
 
+		if (progress) {
+			int current_blamed_lines = count_blamed_lines(sb);
+			if (current_blamed_lines > blamed_lines) {
+				blamed_lines = current_blamed_lines;
+				display_progress(progress, blamed_lines);
+			}
+		}
+
 		if (DEBUG) /* sanity */
 			sanity_check_refcnt(sb);
 	}
+
+	if (progress) {
+		stop_progress(&progress);
+	}
 }
 
 static const char *format_time(unsigned long time, const char *tz_str,
@@ -2520,6 +2548,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 +2584,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);
@@ -2578,6 +2608,9 @@ parse_done:
 	no_whole_file_rename = !DIFF_OPT_TST(&revs.diffopt, FOLLOW_RENAMES);
 	DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
 	argc = parse_options_end(&ctx);
+	if (show_progress < 0) {
+		show_progress = isatty(2);
+	}
 
 	if (0 < abbrev)
 		/* one more abbrev length is needed for the boundary commit */
@@ -2830,11 +2863,11 @@ parse_done:
 
 	read_mailmap(&mailmap, NULL);
 
+	assign_blame(&sb, opt, show_progress);
+
 	if (!incremental)
 		setup_pager();
 
-	assign_blame(&sb, opt);
-
 	free(final_commit_name);
 
 	if (incremental)
-- 
2.6.2

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

[Edmundo Carmona Antoranz]

Hey, guys!

Time for some more patch destruction.

On Sat, Nov 21, 2015 at 11:11 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
-static void assign_blame(struct scoreboard *sb, int opt)
+static void assign_blame(struct scoreboard *sb, int opt, int show_progress)

Would it be better to include show_progress as a binary flag in opt
instead of a separate variable?

> +       struct progress * progress = NULL;
> +       int blamed_lines = -1;

I'm wondering if it would be better to save the 'last number used in
progress' inside struct progress so that we could skip blamed_lines.
That would also allow progress itself (as in the API) to avoid
printing duplicated progress values. In my case, I'm going through a
number of cycles where _I think_ I might have the same number of
blamed lines detected. To avoid asking to do a new progress print out
with the same value, I'm checking the value I had used last time. If
progress took care of that check up, this variable would go for good
and I would just ask progress to print with the current value (no
matter if it's duplicate or an increased value).

> +
> +       if (show_progress) {
> +               progress = start_progress(_("Blaming lines"), sb->num_lines);
> +       }

Already noticed I can get rid of the curly brackets.

>
>         while (commit) {
>                 struct blame_entry *ent;
> @@ -1822,9 +1838,21 @@ static void assign_blame(struct scoreboard *sb, int opt)
>                 }
>                 origin_decref(suspect);
>
> +               if (progress) {
> +                       int current_blamed_lines = count_blamed_lines(sb);
> +                       if (current_blamed_lines > blamed_lines) {
> +                               blamed_lines = current_blamed_lines;
> +                               display_progress(progress, blamed_lines);
> +                       }
> +               }
> +
>                 if (DEBUG) /* sanity */
>                         sanity_check_refcnt(sb);
>         }
> +
> +       if (progress) {
> +               stop_progress(&progress);
> +       }
>  }
>

It first I tried to detect how many revisions where going to be
checked so that I could report progress on them but I found extracting
information from scoreboard a little tricky (to be read: I could not
extract anything out of it). Then I though of counting lines and it
works so.... (same thing with the curly brackets).


> +       assign_blame(&sb, opt, show_progress);
> +
>         if (!incremental)
>                 setup_pager();
>
> -       assign_blame(&sb, opt);
> -

setup_pager() was breaking progress so I moved it _after_
assign_blame() and it seems to behave. Hope that's not a problem.

Looking forward to your feedback.

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

[Edmundo Carmona Antoranz]

I think I found where to calculate the number of blamed lines without
having to do it from scratch every cycle. It's where the list of
sb->ent is getting its nodes pointed.... around here:

for (;;) {
    struct blame_entry *next = ent->next;
    found_guilty_entry(ent);
    if (next) {
        ent = next;
        continue;
    }
    ent->next = sb->ent;
    sb->ent = suspect->suspects;
    suspect->suspects = NULL;
    break;
}

So the function I added to blame.c, it's gone.

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

[Edmundo Carmona Antoranz]

On Sat, Nov 21, 2015 at 11:27 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> -static void assign_blame(struct scoreboard *sb, int opt)
> +static void assign_blame(struct scoreboard *sb, int opt, int show_progress)
>
> Would it be better to include show_progress as a binary flag in opt
> instead of a separate variable?

What is the point of having 'global' variable and not using it as such?

I'm sending a second version of the patch where I'm taking care of
most of my previous comments. Let's hope that one holds water.

Thanks in advance.