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

On Wed, Dec 9, 2015 at 11:20 PM, Edmundo Carmona Antoranz
[off-list ref] wrote:

On Tue, Dec 8, 2015 at 2:22 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Tue, Nov 24, 2015 at 11:36 PM, Edmundo Carmona Antoranz
[off-list ref] wrote:

quoted

+--[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.

quoted

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.

quoted

    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.