| Message ID | c0481f85f8166e520c387f9e9157b142b93d933c.1723747832.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add additional trace2 regions for fetch and push | expand |
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.
On 2024.08.15 12:47, Junio C Hamano wrote: > 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? 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/
diff --git a/builtin/fetch.c b/builtin/fetch.c index 693f02b958..950cd79baa 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2353,9 +2353,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) if (!max_jobs) max_jobs = online_cpus(); - 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; + 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); + } if (all < 0) { /* @@ -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); } 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); + } + 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); } else { int max_children = max_jobs; @@ -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); } /* @@ -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); } @@ -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); } if (enable_auto_gc) {
At $DAYJOB we experienced some slow fetch operations and needed some additional data to help diagnose the issue. Add top-level trace2 regions for the various modes of operation of `git-fetch`. None of these regions are in recursive code, so any enclosed trace messages should only see their nesting level increase by one. Signed-off-by: Josh Steadmon <steadmon@google.com> --- builtin/fetch.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)