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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Dec 9, 2015 at 11:20 PM, Edmundo Carmona Antoranz
<eantoranz@xxxxxxxxx> wrote:
> On Tue, Dec 8, 2015 at 2:22 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Tue, Nov 24, 2015 at 11:36 PM, Edmundo Carmona Antoranz
>> <eantoranz@xxxxxxxxx> 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?

It's a bit difficult to imagine a case when --progress would make
sense with --porcelain or --incremental, so I'd probably opt for
emitting a "mutually exclusive" diagnostic to inform the user
explicitly that the combination is nonsensical.

>> 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

Adding Helped-by: footers before your Signed-off-by: is a way to
acknowledge those who've helped shape the patch if you feel that such
assistance had significant value.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]