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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 19, 2019, 18:46 UTC
Message-ID: <CAPig+cQhs-YDv2LySbAJkXOWGEAQohkPipj1WLz-BZqrWFyo1Q@mail.gmail.com>
In-Reply-To: <fbb2ce21-18ce-3d1e-e6f6-907c8b9693d9@jeffhostetler.com>

On Tue, Mar 19, 2019 at 2:33 PM Jeff Hostetler <git@jeffhostetler.com> wrote:

Show 16 quoted lines

> On 3/19/2019 12:42 PM, Eric Sunshine wrote:
> > 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)
> >      {
> >          [...]
> >      }
> >
> > without having to make any other changes to the implementation or add
> > a new field to the structure.
>
> The existing model has start_progress() and start_delayed_progress().
> I was following that model and added the new option at the start.
> I'm not planning on adding any additional flags, but if we had some
> we'd want them available on the startup next to this one.

Perhaps it makes sense to just take a 'flags' argument instead of 'sparse' so we don't have to worry about this going forward. In other words:

    #define PROGRESS_DELAYED (1 << 0)
    #define PROGRESS_SPARSE (1 << 1)

    struct progress *start_progress_x(const char *title, uint64_t total,
        unsigned flags);

which covers "delayed" start and "sparse". ("_x" is a placeholder since I can't think of a good name).

Show 7 quoted lines

> >                                It would mean, though, that the caller
> > would have to remember to invoke finish_progress() rather than
> > stop_progress().
>
> Right, I was trying to isolate the change to the setup, so that loop
> bottoms and any in-loop return points don't all have to worry about
> whether to call stop_ or finish_.

Yes, I understand the benefit.

Anyhow, my comments are akin to bikeshedding, thus not necessarily actionable.