Re: [PATCH 1/2] fetch: add top-level trace2 regions
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 15, 2024, 19:47 UTC
- Message-ID
- <xmqqh6blsh43.fsf@gitster.g>
- In-Reply-To
- <c0481f85f8166e520c387f9e9157b142b93d933c.1723747832.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
Show 5 quoted lines
> - 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.
Show 6 quoted lines
> + 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?
Show 21 quoted lines
> @@ -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)?
Show 7 quoted lines
> } 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.
Show 7 quoted lines
> @@ -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.
Show 15 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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.