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

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