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

From: Edmundo Carmona Antoranz <eantoranz@gmail.com>
Date: Dec 13, 2015, 00:30 UTC
Message-ID: <CAOc6etar5DU0w_RFRgUAzK39R43cP=eSwT_=0u2NbgKhdPi8Sg@mail.gmail.com>
In-Reply-To: <CAPig+cQq2Y0m0UJVCMb-9B8qrGNXV7ecf5hDETdgX5w4oUuAvw@mail.gmail.com>

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!