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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Nov 24, 2015 at 11:36 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> * created struct progress_info in builtin/blame.c
>   this struct holds the information used to display progress so
>   that only one additional parameter is passed to
>   found_guilty_entry().

Commit messages typically are composed of proper sentences and
paragraphs rather than bullets points. Also, write in imperative mood:
"Create progress_info..."

In this case, though, the information in this bullet point isn't all
that interesting for the commit message since it's a detail of
implementation easily gleaned by reading the patch itself. There's
nothing particularly tricky about it, thus it doesn't really deserve
to be called out as special in the commit message.

What might be more interesting and deserve mention in the commit
message is how this new option interacts with porcelain and
incremental modes and why the choice was made.

More below...

> * --[no-]progress option is also inherited by git-annotate.
>
> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
> ---
> diff --git 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`.

Is silently ignoring --progress a good idea when combined with
--porcelain or --incremental, or would it be better to error out with
a "mutually exclusive options" diagnostic? (Genuine question.)

> diff --git 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>]

Hmm, is [--[no-]progress] common in Git documentation? (Genuine
question.) For the synopsis, I'd have expected to see only
[--progress] and leave it to the more detailed description of the
option to mention the possibility of negation (but I haven't
double-checked other documentation, so my expectation may be skewed).

>  DESCRIPTION
>  -----------
> 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;
> +};
> @@ -1746,7 +1753,8 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)
> -static void found_guilty_entry(struct blame_entry *ent)
> +static void found_guilty_entry(struct blame_entry *ent,
> +                          struct progress_info *pi)
> @@ -1758,6 +1766,8 @@ static void found_guilty_entry(struct blame_entry *ent)
> +       if (pi)
> +               display_progress(pi->progress, pi->blamed_lines += ent->num_lines);

This is updating of 'blamed_lines' is rather subtle and easily
overlooked. Splitting it out as a separate statement could improve
readability:

    pi->blamed_lines += ent->num_lines;
    display_progress(pi->progress, pi->blamed_lines);

>  }
> @@ -1768,6 +1778,16 @@ 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;
> +
> +       if (show_progress) {
> +               pi = malloc(sizeof(*pi));
> +               if (pi) {

xmalloc(), unlike malloc(), will die() upon allocation failure which
would allow you to avoid the "if (pi)" conditional.

> +                       pi->progress = start_progress_delay(_("Blaming lines"),
> +                                                           sb->num_lines, 50, 1);
> +                       pi->blamed_lines = 0;
> +               }
> +       }
>
>         while (commit) {
>                 struct blame_entry *ent;
> @@ -1809,7 +1829,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 +1845,11 @@ static void assign_blame(struct scoreboard *sb, int opt)
>                 if (DEBUG) /* sanity */
>                         sanity_check_refcnt(sb);
>         }
> +
> +       if (pi) {
> +               stop_progress(&pi->progress);
> +               free(pi);
> +       };

Style: drop semi-colon following closing brace

Overall, use of malloc/free for the progress_info struct seems to
makes the code more complex rather than less. It's not clear why you
don't just use a 'struct progress_info' directly, which would lift the
burden of freeing the allocated structure, and allow you to drop the
conditional around stop_progress() since NULL progress is accepted. In
other words, something like this (completely untested):

    struct progress_info pi = { NULL, 0 };
    if (show_progress) {
        pi.progress = start_progress_delay(...);
    ...
    found_guilty_entry(ent, &pi);
    ...
    stop_progress(&pi.progress)

which is more concise and less likely to leak resources. The code
within found_guilty_entry() is also simplified since you can drop the
"if (pi)" conditional entirely. This works because it's safe to call
display progress with NULL):

    pi->blamed_lines += ent->num_lines;
    display_progress(pi->progress, pi->blamed_lines);

>  }
>
>  static const char *format_time(unsigned long time, const char *tz_str,
> @@ -2579,6 +2606,11 @@ parse_done:
>         DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
>         argc = parse_options_end(&ctx);
>
> +       if (incremental || (output_option & OUTPUT_PORCELAIN))
> +               show_progress = 0;
> +       else if (show_progress < 0)
> +               show_progress = isatty(2);
> +