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

> On Tue, Dec 8, 2015 at 2:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Nov 24, 2015 at 11:36 PM, Edmundo Carmona Antoranz
>> <eantoranz@gmail.com> wrote:
>>> +--[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`.
>>
>> Is silently ignoring --progress a good idea when combined with
>> --porcelain or --incremental, or would it be better to error out with
>> a "mutually exclusive options" diagnostic? (Genuine question.)
>
> I think it makes sense (and so it's documented so people can know what
> could be going on but detecting mutually exclusive options and
> erroring out could also make sense. Who could give some insight?

>> Overall, use of malloc/free for the progress_info struct seems to
>> makes the code more complex rather than less. It's not clear why you
>> don't just use a 'struct progress_info' directly, which would lift the
>> burden of freeing the allocated structure, and allow you to drop the
>> conditional around stop_progress() since NULL progress is accepted. In
>> other words, something like this (completely untested):
>>
>>     struct progress_info pi = { NULL, 0 };
>
> I like it! I'll adjust to not use the pointer.
>
>>     if (show_progress) {
>>         pi.progress = start_progress_delay(...);
>>     ...
>>     found_guilty_entry(ent, &pi);
>>     ...
>>     stop_progress(&pi.progress)
>>
>> which is more concise and less likely to leak resources. The code
>> within found_guilty_entry() is also simplified since you can drop the
>> "if (pi)" conditional entirely. This works because it's safe to call
>> display progress with NULL):
>>
>>     pi->blamed_lines += ent->num_lines;
>>     display_progress(pi->progress, pi->blamed_lines);
>
> Officially, I think not a single line of the patch survived. :-D