Re: [PATCH v6] blame: add support for --[no-]progress option
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Dec 13, 2015, 00:17 UTC
- Message-ID
- <CAPig+cQq2Y0m0UJVCMb-9B8qrGNXV7ecf5hDETdgX5w4oUuAvw@mail.gmail.com>
- In-Reply-To
- <1449964625-27737-1-git-send-email-eantoranz@gmail.com>
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++;