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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 13, 2015, 00:37 UTC
Message-ID: <CAPig+cQjWgTR9Sd-CutkbuBa1Czp5YApsYoFXc9HS1H08cno7A@mail.gmail.com>
In-Reply-To: <CAOc6etar5DU0w_RFRgUAzK39R43cP=eSwT_=0u2NbgKhdPi8Sg@mail.gmail.com>

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.