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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Emily Shaffer wrote:

> Before now, the progress API is used by conditionally calling
> start_progress() or a similar call, and then unconditionally calling
> display_progress() and stop_progress(), both of which are tolerant of
> NULL or uninitialized inputs.

Being super nitpicky for a moment:

- Git's commit messages describe the behavior without the patch in
  the present tense, as though the author were writing a bug report.
  so "Before now" can be "Today".

- with an uninitialized input, display_progress would produce undefined
  behavior and likely crash ;-)

[...]
> Since this changes the API, also modify callers of start_progress() and
> friends to drop their conditional and pass a new argument in instead.

If I understand correctly, the calling sequence before this patch is
something like

	struct progress *progress =
		want_progress ? start_progress(title, n) : NULL;

	for (int i = 0; i < n; i++) {
		... do some work ...
		display_progress(progress, i);
	}

	stop_progress(progress);

and this patch changes it to

	struct progress *progress = start_progress(title, n, want_progress);

	for (int i = 0; i < n; i++) {
		... do some work ...
		display_progress(progress, i);
	}

	stop_progress(progress);

A few consequences:

- it's a little briefer, which is nice

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

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

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

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

Overall I think it's an improvement in the API.  It opens the door to
future changes like making the progress API handle isatty checks in
some cases, perhaps.

[...]
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
[...]
> @@ -836,16 +836,15 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
>  			uint32_t total = 0, count = 0;
>  			struct progress *progress = NULL;
>  
> -			if (show_progress) {
> -				for (p = get_all_packs(the_repository); p;
> -				     p = p->next) {
> -					if (open_pack_index(p))
> -						continue;
> -					total += p->num_objects;
> -				}
> -
> -				progress = start_progress(_("Checking objects"), total);
> +			for (p = get_all_packs(the_repository); p;
> +			     p = p->next) {
> +				if (open_pack_index(p))
> +					continue;
> +				total += p->num_objects;
>  			}
> +
> +			progress = start_progress(_("Checking objects"), total,
> +						  show_progress);

As Jonathan mentions[1], this is nothing next to the work that fsck is
about to do on those objects, so although it's aesthetically
unappealing to have to compute this total just in order to throw it
away, it's not likely to make a big difference.

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);
	}

Using a callback to supply the title and total like Jonathan suggests
is another possibility.  It seems a bit more fussy, though.

[...]
> --- a/progress.h
> +++ b/progress.h
> @@ -13,11 +13,13 @@ void progress_test_force_update(void);
>  
>  void display_throughput(struct progress *progress, uint64_t total);
>  void display_progress(struct progress *progress, uint64_t n);
> -struct progress *start_progress(const char *title, uint64_t total);
> -struct progress *start_sparse_progress(const char *title, uint64_t total);
> -struct progress *start_delayed_progress(const char *title, uint64_t total);
> +struct progress *start_progress(const char *title, uint64_t total, int verbose);
> +struct progress *start_sparse_progress(const char *title, uint64_t total,
> +				       int verbose);
> +struct progress *start_delayed_progress(const char *title, uint64_t total,
> +					int verbose);
>  struct progress *start_delayed_sparse_progress(const char *title,
> -					       uint64_t total);
> +					       uint64_t total, int verbose);
>  void stop_progress(struct progress **progress);
>  void stop_progress_msg(struct progress **progress, const char *msg);

This header contains no comments and there's no API docs for it in
Documentation/technical/ waiting to be copied into comments, so we end
up having to reverse engineer it.  Not about this patch, but it would
be nice to add an overview of the progress API to this file (e.g., to
show a typical calling sequence).

Since display_progress and display_throughput are no longer about
display, would it make sense to rename them (in a separate patch)?
For example,

	update_progress(progress, ++i);
	update_throughput(progress, bytes);

"update" is a bit vague but it may convey more clearly that this
affects traces too and not just what is written to the terminal.

> --- a/prune-packed.c
> +++ b/prune-packed.c
> @@ -31,8 +31,8 @@ static int prune_object(const struct object_id *oid, const char *path,
>  
>  void prune_packed_objects(int opts)
>  {
> -	if (opts & PRUNE_PACKED_VERBOSE)
> -		progress = start_delayed_progress(_("Removing duplicate objects"), 256);
> +	progress = start_delayed_progress(_("Removing duplicate objects"), 256,
> +					  (opts & PRUNE_PACKED_VERBOSE));

style nit: no need for parens around the function parameter (likewise
for most of these)

[...]
> --- a/t/t0500-progress-display.sh
> +++ b/t/t0500-progress-display.sh
> @@ -309,4 +309,31 @@ test_expect_success 'progress generates traces' '
>  	grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event
>  '
>  
> +test_expect_success 'progress generates traces even quietly' '

Nice.

With whatever subset of the changes discussed makes sense,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

[1] https://lore.kernel.org/git/20200909224253.866718-1-jonathantanmy@google.com