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