Re: [PATCH 1/2] fetch: add top-level trace2 regions

[Junio C Hamano <gitster@pobox.com>]

Josh Steadmon <steadmon@google.com> writes:

> -	if (!git_config_get_string_tmp("fetch.bundleuri", &bundle_uri) &&
> -	    fetch_bundle_uri(the_repository, bundle_uri, NULL))
> -		warning(_("failed to fetch bundles from '%s'"), bundle_uri);
> +	if (!git_config_get_string_tmp("fetch.bundleuri", &bundle_uri)) {
> +		int result = 0;

This needs no initialization.

> +		trace2_region_enter("fetch", "fetch-bundle-uri", the_repository);
> +		result = fetch_bundle_uri(the_repository, bundle_uri, NULL);
> +		trace2_region_leave("fetch", "fetch-bundle-uri", the_repository);
> +		if (result)
> +			warning(_("failed to fetch bundles from '%s'"), bundle_uri);
> +	}

It is a bit sad that the concise original with straight-forward
control flow had to be butchered like this to sprinkle tracing code
in it, but I guess that cannot be helped?  I wonder if it becomes
much less invasive and more future proof to define the trace region
in the fetch_bundle_uri() function itself.  Has it been considered?

> @@ -2407,6 +2412,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		struct oidset_iter iter;
>  		const struct object_id *oid;
>  
> +		trace2_region_enter("fetch", "negotiate-only", the_repository);
>  		if (!remote)
>  			die(_("must supply remote when using --negotiate-only"));
>  		gtransport = prepare_transport(remote, 1);
> @@ -2415,6 +2421,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		} else {
>  			warning(_("protocol does not support --negotiate-only, exiting"));
>  			result = 1;
> +			trace2_region_leave("fetch", "negotiate-only", the_repository);
>  			goto cleanup;
>  		}
>  		if (server_options.nr)
> @@ -2425,11 +2432,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		while ((oid = oidset_iter_next(&iter)))
>  			printf("%s\n", oid_to_hex(oid));
>  		oidset_clear(&acked_commits);
> +		trace2_region_leave("fetch", "negotiate-only", the_repository);

OK.  Both error path and normal path we leave the region we entered.

A complete tangent, but do we have an automated test or code
analysis that catches us if we forget to leave an entered region
(i.e., imagine we didn't leave in the else clause after issuing the
warning---we remain in the region in such an error case, even though
normally we leave the region correctly)?

>  	} else if (remote) {
> -		if (filter_options.choice || repo_has_promisor_remote(the_repository))
> +		if (filter_options.choice || repo_has_promisor_remote(the_repository)) {
> +			trace2_region_enter("fetch", "setup-partial", the_repository);
>  			fetch_one_setup_partial(remote);
> +			trace2_region_leave("fetch", "setup-partial", the_repository);
> +		}

OK.  That's nice and straight-forward.

> +		trace2_region_enter("fetch", "fetch-one", the_repository);
>  		result = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs,
>  				   &config);
> +		trace2_region_leave("fetch", "fetch-one", the_repository);

This one, too.
> @@ -2449,7 +2462,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			max_children = config.parallel;
>  
>  		/* TODO should this also die if we have a previous partial-clone? */
> +		trace2_region_enter("fetch", "fetch-multiple", the_repository);
>  		result = fetch_multiple(&list, max_children, &config);
> +		trace2_region_leave("fetch", "fetch-multiple", the_repository);

So is this.

> @@ -2471,6 +2486,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			max_children = config.parallel;
>  
>  		add_options_to_argv(&options, &config);
> +		trace2_region_enter_printf("fetch", "recurse-submodule", the_repository, "%s", submodule_prefix);
>  		result = fetch_submodules(the_repository,
>  					  &options,
>  					  submodule_prefix,
> @@ -2478,6 +2494,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  					  recurse_submodules_default,
>  					  verbosity < 0,
>  					  max_children);
> +		trace2_region_leave_printf("fetch", "recurse-submodule", the_repository, "%s", submodule_prefix);
>  		strvec_clear(&options);
>  	}

Ditto.

> @@ -2501,9 +2518,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		if (progress)
>  			commit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS;
>  
> +		trace2_region_enter("fetch", "write-commit-graph", the_repository);
>  		write_commit_graph_reachable(the_repository->objects->odb,
>  					     commit_graph_flags,
>  					     NULL);
> +		trace2_region_leave("fetch", "write-commit-graph", the_repository);

OK.