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

From: Edmundo Carmona Antoranz <eantoranz@gmail.com>
Date: Nov 23, 2015, 02:47 UTC
Message-ID: <CAOc6etY2+OkHhn1rt=y8ekjzcA8ngu0zvX_MZ5Pk6utYj09vPg@mail.gmail.com>
In-Reply-To: <xmqq610to4bl.fsf@gitster.mtv.corp.google.com>

Nice to see you back, Junio!

On Sun, Nov 22, 2015 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 8 quoted lines

> Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:
>
>> Will also affect annotate
>
> Is that a good thing?  In any case, make it understandable without
> the title line (i.e. make it a full sentence, ending with a full
> stop).
>

Will make the explanation a little more verbose. About being a bad thing, I don't see how, it's just the same functionality. You think I should turn it off if using annotate?

Show 12 quoted lines

>> +                             if (progress) {
>> +                                     for (next = suspect->suspects; next != NULL;
>> +                                          next = next->next)
>> +                                             blamed_lines += next->num_lines;
>> +                                     display_progress(progress, blamed_lines);
>> +                             }
>
> Is this math and the placement of the code correct?  It would
> probably be more obvious if this hunk is in found_guilty_entry(),
> which is already the dedicated function in which we report about a
> group of lines whose ultimate origin has become clear.
>

I'll see what I can do about it.

Show 5 quoted lines

>
> Two comments.
>
>  * How does this interact with incremental or porcelain blame?
>    Shouldn't progress be turned off when these modes are in use?

Given that they are supposed to be for machine consumption, I'll turn progress off is using one of them.

>
>  * Shouldn't progress be turned off if the result comes very
>    quickly, using start_progress_delay()?
>

Ok. Default values as in checkout? 50, 1?