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

[Edmundo Carmona Antoranz <eantoranz@gmail.com>]

Hey, Eric!

On Tue, Dec 8, 2015 at 2:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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...

Ok

>
>> * --[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.)
>

I think it makes sense (and so it's documented so people can know what
could be going on but detecting mutually exclusive options and
erroring out could also make sense. Who could give some insight?

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

Hmmm.... hadn't noticed that. Will remove it from this part.

>>  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);
>

Ok.

>>  }
>> @@ -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.
>

Ok.

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

Ok.

> 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 };

I like it! I'll adjust to not use the pointer.

>     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);
>> +


Officially, I think not a single line of the patch survived. :-D

Thanks, man... I'll go through the stuff and let's wait for feedback
about the detection/error-out in case of using "mutually exclusive"
options.

Cheers!