Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2020, 05:09 UTC
Message-ID: <xmqqeenarzf2.fsf@gitster.c.googlers.com>
In-Reply-To: <20200910003157.GB667601@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

> A few consequences:
>
> - it's a little briefer, which is nice

Also this makes start always pair with stop, which is cleaner, especially if there is no extra work.

The current "if you do not want the overhead when not collecting stats for and showing progress, just avoid calling start---stop without calling start will be a no-op and safe anyway" arrangement we have feels a bit kludgy.

Show 10 quoted lines

> - progress is always non-NULL, so we can't express
>
> 	if (progress) {
> 		for ( ... ) {
> 			... do one chunk of work ...
> 			display_progress(...);
> 		}
> 	} else {
> 		... do work slightly more efficiently, all in one chunk ...
> 	}

Yes, this is the other side of the coin. When there is significant difference in the work between with and without progress codepath, it is convenient to be able to switch on the "progress" pointer itself. The progress_is_enabled() helper you bring up later may be a way to solve it.

> - even if we don't want progress, we always spend the overhead of
>   allocating a progress struct (not a big deal)

True.

> - if 'n' is a more complex expression (e.g. a function call), it gets
>   computed even if we don't want progress.  For example, in "git fsck",
>   as Junio noticed, this means accumulating the object counts from all
>   idx files just to throw them away.

Yes, I think the conceptual muddiness caused by this is what disturbed me the most. The cost of counting would likely to be negligible; developers' time to understand why things are counted in the first place however is the true waste.

Show 9 quoted lines

> - the motivation: it means the progress API can be aware of whether
>   the caller wants to write progress to the terminal and has control
>   over what to do with that information.
>
>   In particular this makes the function name display_progress even
>   more of a misnomer --- before this patch, display_progress on a
>   non-NULL progress struct would display some progress information and
>   possibly also write something to traces, but after this patch it
>   sometimes only writes something to traces.

Yeah, this might show us a way to an acceptable solution to the problem of conceptual uncleanliness, as naming may have a lot to contribute to it.

Show 15 quoted lines

> That said, what would an API look like that avoids that?
>
> One possibility would be to make separate initialization and
> start-of-progress calls:
>
> 	struct progress *progress = progress_new(show_progress, the_repository);
>
> 	if (progress_is_enabled(progress)) {
> 		for (...) {
> 			...
> 			total += ...
> 		}
>
> 		start_progress(progress, _("Checking objects"), total);
> 	}

OK.