From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 13 Dec 2015 00:37:01 GMT
Subject: Re: [PATCH v6] blame: add support for --[no-]progress option
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:
> 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.