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

[Junio C Hamano <gitster@pobox.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.

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

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

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