On 2024.08.15 12:47, Junio C Hamano wrote:
> Josh Steadmon <steadmon@xxxxxxxxxx> 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?
Moved to fetch_bundle_uri() in v2.
> > @@ -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)?
It's been discussed before [1], but the general feeling seems to be that
it's not worth the effort / test runtime.
[1] https://lore.kernel.org/git/xmqqbka27zu9.fsf@gitster.g/
