git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Derrick Stolee <stolee@gmail.com>
Date
Jul 10, 2020, 02:00 UTC
Message-ID
<ed6b2339-83cd-a7b9-c653-bc86d8686680@gmail.com>
In-Reply-To
<20200710014242.1088216-2-emilyshaffer@google.com>
On 7/9/2020 9:42 PM, Emily Shaffer wrote:
Show 19 quoted lines
> 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. However, in
> 98a136474082cdc7228d7e0e45672c5274fab701 (trace: log progress time and
> throughput), the progress library learned to log traces during expensive
> operations. In cases where progress should not be displayed to the user
> - such as when Git is called by a script - no traces will be logged,
> because the progress object is never created.
> 
> Instead, to allow us to collect traces from scripted Git commands, teach
> a progress->verbose flag, which is specified via a new argument to
> start_progress() and friends. display_progress() also learns to filter
> for that flag. With these changes, start_progress() can be called
> unconditionally but with a conditional as an argument to determine
> whether to report progress to the user.
> 
> Since this changes the API, also modify callers of start_progress() and
> friends to drop their conditional and pass a new argument in instead.

This is a worthwhile change. Thanks! I was hoping that we would get some of these regions for free, which extends what we can get out of trace2 events.

CC'ing Taylor because he had some thoughts on adding a possible trace2 category to make it easier to reason about the regions, when appropriate. Not sure if he's ready to apply that change on top of this series.

Show 17 quoted lines
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index f520111eda..f64cad8390 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -620,8 +620,13 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  	if (bisect_list)
>  		revs.limited = 1;
>  
> -	if (show_progress)
> -		progress = start_delayed_progress(show_progress, 0);
> +	/*
> +	 * When progress is not printed to the user, we still want to be able to
> +	 * classify the progress during tracing. So, use a placeholder name.
> +	 */
> +	progress = start_delayed_progress(
> +			show_progress ? show_progress : _("Quiet rev-list operation"),
> +			0, show_progress != NULL)

This is so strange, how we let the command-lines specify a progress indicator. I guess it is necessary when we use rev-list as a subcommand instead of in-process. One such case is check_connected() in connected.c.

It's stranger still that "show_progress" is actually a string here, as opposed to being an int in most other places.

Your transformation is correct, here, though. Thanks for calling it out in the commit message.

Show 33 quoted lines
>  
>  	if (use_bitmap_index) {
>  		if (!try_bitmap_count(&revs, &filter_options))
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index dd4a75e030..719d446916 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -498,8 +498,7 @@ static void unpack_all(void)
>  			ntohl(hdr->hdr_version));
>  	use(sizeof(struct pack_header));
>  
> -	if (!quiet)
> -		progress = start_progress(_("Unpacking objects"), nr_objects);
> +	progress = start_progress(_("Unpacking objects"), nr_objects, !quiet);
>  	obj_list = xcalloc(nr_objects, sizeof(*obj_list));
>  	for (i = 0; i < nr_objects; i++) {
>  		unpack_one(i);
> diff --git a/commit-graph.c b/commit-graph.c
> index 328ab06fd4..b9a784fece 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1152,10 +1152,10 @@ static void write_graph_chunk_bloom_indexes(struct hashfile *f,
>  	struct progress *progress = NULL;
>  	int i = 0;
>  
> -	if (ctx->report_progress)
> -		progress = start_delayed_progress(
> -			_("Writing changed paths Bloom filters index"),
> -			ctx->commits.nr);
> +	progress = start_delayed_progress(
> +		_("Writing changed paths Bloom filters index"),
> +		ctx->commits.nr,
> +		ctx->report_progress);

There are a lot of blocks like this, where the progress string is long enough to require the first param to be after the method name. Since we are changing the API and every caller, would the resulting code be cleaner if the string value was the last parameter? That would allow this code pattern in most cases:

	progress = start_delayed_progress(count, show_progress,
					  _("My special string!"));
Just a thought. Not super-important.
The rest of the changes look to be correct.
Show 66 quoted lines
> diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
> index 5d05cbe789..19b874f9cd 100644
> --- a/t/helper/test-progress.c
> +++ b/t/helper/test-progress.c
> @@ -23,16 +23,18 @@
>  int cmd__progress(int argc, const char **argv)
>  {
>  	int total = 0;
> +	int quiet = 0;
>  	const char *title;
>  	struct strbuf line = STRBUF_INIT;
>  	struct progress *progress;
>  
>  	const char *usage[] = {
> -		"test-tool progress [--total=<n>] <progress-title>",
> +		"test-tool progress [--total=<n>] [--quiet] <progress-title>",
>  		NULL
>  	};
>  	struct option options[] = {
>  		OPT_INTEGER(0, "total", &total, "total number of items"),
> +		OPT_BOOL(0, "quiet", &quiet, "suppress stderr"),
>  		OPT_END(),
>  	};
>  
> @@ -42,7 +44,7 @@ int cmd__progress(int argc, const char **argv)
>  	title = argv[0];
>  
>  	progress_testing = 1;
> -	progress = start_progress(title, total);
> +	progress = start_progress(title, total, !quiet);
>  	while (strbuf_getline(&line, stdin) != EOF) {
>  		char *end;
>  
> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
> index 1ed1df351c..9d6e6274ad 100755
> --- 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' '
> +	cat >in <<-\EOF &&
> +	throughput 102400 1000
> +	update
> +	progress 10
> +	throughput 204800 2000
> +	update
> +	progress 20
> +	throughput 307200 3000
> +	update
> +	progress 30
> +	throughput 409600 4000
> +	update
> +	progress 40
> +	EOF
> +
> +	GIT_TRACE2_EVENT="$(pwd)/trace.event" test-tool progress --total=40 \
> +		--quiet "Working hard" <in 2>stderr &&
> +
> +	# t0212/parse_events.perl intentionally omits regions and data.
> +	grep -e "region_enter" -e "\"category\":\"progress\"" trace.event &&
> +	grep -e "region_leave" -e "\"category\":\"progress\"" trace.event &&
> +	grep "\"key\":\"total_objects\",\"value\":\"40\"" trace.event &&
> +	grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event
> +'
Thanks for adding a test, including the resulting trace events!

The patch of Taylor's that I mentioned earlier changes the "category" to something a bit more specific than "progress", when appropriate.

> +
> +
>  test_done
nit: extra empty line

Thanks! -Stolee

Previous: Emily ShafferNext: Taylor Blau
Message 3 of 27 in “enable progress traces even when quiet”
  1. 0/2 enable progress traces even when quietEmily Shaffer, Jul 10, 2020
  2. 1/2 progress: create progress struct in 'verbose' modeEmily Shaffer, Jul 10, 2020
  3. Derrick StoleeJul 10, 2020
  4. Taylor BlauJul 10, 2020
  5. Emily ShafferJul 10, 2020
  6. brian m. carlsonJul 10, 2020
  7. Emily ShafferJul 10, 2020
  8. Junio C HamanoJul 10, 2020
  9. Emily ShafferJul 10, 2020
  10. Junio C HamanoJul 10, 2020
  11. Emily ShafferJul 14, 2020
  12. Emily ShafferAug 17, 2020
  13. Junio C HamanoAug 17, 2020
  14. Junio C HamanoAug 17, 2020
  15. Jonathan TanSep 9, 2020
  16. Jonathan TanSep 9, 2020
  17. Junio C HamanoSep 9, 2020
  18. Jonathan NiederSep 10, 2020
  19. Junio C HamanoSep 10, 2020
  20. 2/2 progress: remove redundant null-checkingEmily Shaffer, Jul 10, 2020
  21. Derrick StoleeJul 10, 2020
  22. Taylor BlauJul 10, 2020
  23. Junio C HamanoJul 10, 2020
  24. Emily ShafferJul 10, 2020
  25. Junio C HamanoJul 10, 2020
  26. Emily ShafferJul 10, 2020
  27. Emily ShafferJul 10, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.