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

[PATCH v2] 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                 | 25 +++++++++++++++++++++++--
 3 files changed, 38 insertions(+), 3 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..480bb2d 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;
@@ -1768,6 +1770,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 * progress = NULL;
+	int blamed_lines = 0;
+
+	if (show_progress)
+		progress = start_progress(_("Blaming lines"), sb->num_lines);
 
 	while (commit) {
 		struct blame_entry *ent;
@@ -1814,6 +1821,12 @@ static void assign_blame(struct scoreboard *sb, int opt)
 					ent = next;
 					continue;
 				}
+				if (progress) {
+					for (next = suspect->suspects; next != NULL;
+					     next = next->next)
+						blamed_lines += next->num_lines;
+					display_progress(progress, blamed_lines);
+				}
 				ent->next = sb->ent;
 				sb->ent = suspect->suspects;
 				suspect->suspects = NULL;
@@ -1825,6 +1838,9 @@ static void assign_blame(struct scoreboard *sb, int opt)
 		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 +2536,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 +2572,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 +2597,9 @@ parse_done:
 	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 */
 		abbrev++;
@@ -2830,11 +2851,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 v2] blame: add support for --[no-]progress option

[Johannes Sixt]

Am 22.11.2015 um 17:02 schrieb 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                 | 25 +++++++++++++++++++++++--
>   3 files changed, 38 insertions(+), 3 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>

You add the option to to the synopsis of git-blame.txt, but not to 
git-annotate.txt.

>
>   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.
> +

Any particular reason you add this text twice? As can be seen on the 
hunk header, git-blame.txt includes blame-options.txt.

-- Hannes

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

[Edmundo Carmona Antoranz]

Hey, Johannes!

An oversight! git-annotate.txt took me to blame-options.txt.... and I
didn't notice it's also pointed to from git-blame.txt. Let's wait for
some comments about the coding (or blessings) so that I can send
another patch.

Thanks!

On Sun, Nov 22, 2015 at 1:58 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 22.11.2015 um 17:02 schrieb 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                 | 25 +++++++++++++++++++++++--
>>   3 files changed, 38 insertions(+), 3 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>
>
>
> You add the option to to the synopsis of git-blame.txt, but not to
> git-annotate.txt.
>
>>
>>   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.
>> +
>
>
> Any particular reason you add this text twice? As can be seen on the hunk
> header, git-blame.txt includes blame-options.txt.
>
> -- Hannes
>

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

[Junio C Hamano]

Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:

> Will also affect annotate

Is that a good thing?  In any case, make it understandable without
the title line (i.e. make it a full sentence, ending with a full
stop).

> +				if (progress) {
> +					for (next = suspect->suspects; next != NULL;
> +					     next = next->next)
> +						blamed_lines += next->num_lines;
> +					display_progress(progress, blamed_lines);
> +				}

Is this math and the placement of the code correct?  It would
probably be more obvious if this hunk is in found_guilty_entry(),
which is already the dedicated function in which we report about a
group of lines whose ultimate origin has become clear.

> @@ -2830,11 +2851,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)

Two comments.

 * How does this interact with incremental or porcelain blame?
   Shouldn't progress be turned off when these modes are in use?

 * Shouldn't progress be turned off if the result comes very
   quickly, using start_progress_delay()?

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

[Edmundo Carmona Antoranz]

Nice to see you back, Junio!


On Sun, Nov 22, 2015 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:
>
>> Will also affect annotate
>
> Is that a good thing?  In any case, make it understandable without
> the title line (i.e. make it a full sentence, ending with a full
> stop).
>

Will make the explanation a little more verbose. About being a bad
thing, I don't see how, it's just the same functionality. You think I
should turn it off if using annotate?

>> +                             if (progress) {
>> +                                     for (next = suspect->suspects; next != NULL;
>> +                                          next = next->next)
>> +                                             blamed_lines += next->num_lines;
>> +                                     display_progress(progress, blamed_lines);
>> +                             }
>
> Is this math and the placement of the code correct?  It would
> probably be more obvious if this hunk is in found_guilty_entry(),
> which is already the dedicated function in which we report about a
> group of lines whose ultimate origin has become clear.
>

I'll see what I can do about it.

>
> Two comments.
>
>  * How does this interact with incremental or porcelain blame?
>    Shouldn't progress be turned off when these modes are in use?

Given that they are supposed to be for machine consumption, I'll turn
progress off is using one of them.

>
>  * Shouldn't progress be turned off if the result comes very
>    quickly, using start_progress_delay()?
>

Ok. Default values as in checkout? 50, 1?