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

From: Edmundo Carmona Antoranz <eantoranz@gmail.com>
Date: Dec 13, 2015, 00:36 UTC
Message-ID: <CAOc6etYAbT8Yc3NwB4Rh1dShjDQB_PR2K-HvXpx6dTcB=YaB8w@mail.gmail.com>
In-Reply-To: <CAOc6etar5DU0w_RFRgUAzK39R43cP=eSwT_=0u2NbgKhdPi8Sg@mail.gmail.com>

On Sat, Dec 12, 2015 at 6:30 PM, Edmundo Carmona Antoranz <eantoranz@gmail.com> wrote:

Show 19 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?

Hmmmm.... if the code in assign_blame changed to this, it would be possible to allow the -1 to go through:

if (show_progress > 0)
    pi.progress = start_progress_delay(_("Blaming lines"),
sb->num_lines, 50, 1);

But then I think it would be more 'concise' if we had the value set to 0/1 instead of expecting to see a possible value of -1 there (or anywhere else) after progressing if progress will be shown or not in the piece of code we are chatting about.

Comments?