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

Hi,

Emily Shaffer wrote:

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

[...]

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.

[...]

[...]

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.

[...]

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.

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

[...]

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