threads / patch / 40980

v6blame: add support for --[no-]progress option

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

## tl;dr

8 messages between Dec 12, 2015 and Dec 13, 2015. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Edmundo Carmona Antoranz· Dec 12, 2015, 23:57 UTC · lore
--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(-)
Show changes to 3 files +40 −5

Documentation/blame-options.txt, Documentation/git-blame.txt, builtin/blame.c

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
Edmundo Carmona Antoranz· Dec 13, 2015, 00:01 UTC · re: Edmundo Carmona Antoranz · lore

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

On Sat, Dec 12, 2015 at 5:57 PM, Edmundo Carmona Antoranz <eantoranz@gmail.com> wrote:

Show 7 quoted lines
> +       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!
Eric Sunshine· Dec 13, 2015, 00:17 UTC · re: Edmundo Carmona Antoranz · lore

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

On Sat, Dec 12, 2015 at 6:57 PM, Edmundo Carmona Antoranz <eantoranz@gmail.com> wrote:

Show 8 quoted lines
> --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
Show 13 quoted lines
>  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...
Show 44 quoted lines
> 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
Show 21 quoted lines
>  }
>
>  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++;
Edmundo Carmona Antoranz· Dec 13, 2015, 00:30 UTC · re: Eric Sunshine · lore

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

On Sat, Dec 12, 2015 at 6:17 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 6 quoted lines
> 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.
Show 15 quoted lines
>> @@ -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.

Show 10 quoted lines
>> +
>> +       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!
Show 13 quoted lines
>
> 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!
Edmundo Carmona Antoranz· Dec 13, 2015, 00:36 UTC · re: Edmundo Carmona Antoranz · lore

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

On Sat, Dec 12, 2015 at 6:30 PM, Edmundo Carmona Antoranz <eantoranz@gmail.com> wrote:

Show 19 quoted lines
>>
>> 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?
Eric Sunshine· Dec 13, 2015, 00:40 UTC · re: Edmundo Carmona Antoranz · lore

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

On Sat, Dec 12, 2015 at 7:36 PM, Edmundo Carmona Antoranz <eantoranz@gmail.com> wrote:

Show 13 quoted lines
> 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.

Eric Sunshine· Dec 13, 2015, 00:37 UTC · re: Edmundo Carmona Antoranz · lore

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

On Sat, Dec 12, 2015 at 7:30 PM, Edmundo Carmona Antoranz <eantoranz@gmail.com> wrote:

Show 19 quoted lines
> 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.

Edmundo Carmona Antoranz· Dec 13, 2015, 00:40 UTC · re: Eric Sunshine · lore

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

On Sat, Dec 12, 2015 at 6:37 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 10 quoted lines
>>
>> 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.

← back to recent threads