Re: [PATCH v2 1/4] progress: add sparse mode to force 100% complete message

[Eric Sunshine <sunshine@xxxxxxxxxxxxx>]

On Tue, Mar 19, 2019 at 10:37 AM Jeff Hostetler via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
> Add new start_sparse_progress() and start_delayed_sparse_progress()
> constructors and "sparse" flag to struct progress.
>
> Teach stop_progress() to force a 100% complete progress message before
> printing the final "done" message when "sparse" is set.
>
> Calling display_progress() for every item in a large set can
> be expensive.  If callers try to filter this for performance
> reasons, such as emitting every k-th item, progress would
> not reach 100% unless they made a final call to display_progress()
> with the item count before calling stop_progress().
>
> Now this is automatic when "sparse" is set.
>
> Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
> ---
> diff --git a/progress.c b/progress.c
> +/*
> + * Here "sparse" means that the caller might use some sampling criteria to
> + * decide when to call display_progress() rather than calling it for every
> + * integer value in[0 .. total).  In particular, the caller might not call
> + * display_progress() for the last value in the range.
> + *
> + * When "sparse" is set, stop_progress() will automatically force the done
> + * message to show 100%.
> + */
> +static void finish_if_sparse(struct progress **p_progress)
> +{
> +       struct progress *progress = *p_progress;
> +
> +       if (progress &&
> +           progress->sparse &&
> +           progress->last_value != progress->total)
> +               display_progress(progress, progress->total);
>  }

There's no reason for this function to take a 'struct progress **'
rather than the simpler 'struct progress *', and doing so just
confuses the reader.

>  void stop_progress(struct progress **p_progress)
>  {
> +       finish_if_sparse(p_progress);
> +
>         stop_progress_msg(p_progress, _("done"));
>  }

Rather than adding a new "sparse" mode, I wonder if this entire
concept can be boiled down to a single new function:

    void finish_progress(struct progress **p_progress, const char *msg)
    {
        struct progress *progress = *p_progress;
        if (progress && progress->last_value != progress->total)
            display_progress(progress, progress->total);
        if (msg)
            stop_progress_msg(p_progress, msg);
        else
            stop_progress(p_progress);
    }

without having to make any other changes to the implementation or add
a new field to the structure. It would mean, though, that the caller
would have to remember to invoke finish_progress() rather than
stop_progress(). Which leads one to wonder why this functionality is
needed at all since it's easy enough for a caller to make the one
extra call to simulate this:

    /* client code */
    if (progress)
        display_progress(progress, progress->total);
    stop_progress(&progress);