[PATCH v2] fetch-pack: write effective filter to trace2
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jul 18, 2022, 17:00 UTC
- Message-ID
- <20220718170027.3993042-1-jonathantanmy@google.com>
- In-Reply-To
- <20220715172943.2681492-1-jonathantanmy@google.com>
Administrators of a managed Git environment (like the one at $DAYJOB) might want to quantify the performance change of fetches with and without filters from the client's point of view, and also detect if a server does not support it. Therefore, log the filter information being sent to the server whenever a fetch (or clone) occurs. Note that this is not necessarily the same as what's specified on the CLI, because during a fetch, the configured filter is used whenever a filter is not specified on the CLI.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Thanks everyone for your comments. I've renamed the keys and distinguished the case in which the server does not support filters following Jeff's suggestions.
A few small discussion points: - whether we need the print_verbose now that we have traces - should we log if no filter is specified - if yes, what key should it be logged under (I used "filter/none" for now) --- fetch-pack.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c index cb6647d657..68820f9a1a 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -292,6 +292,29 @@ static void mark_tips(struct fetch_negotiator *negotiator, return; } +static void write_and_trace_filter(struct fetch_pack_args *args, + struct strbuf *req_buf, + int server_supports_filter) +{ + if (args->filter_options.choice) { + const char *spec = + expand_list_objects_filter_spec(&args->filter_options); + if (server_supports_filter) { + print_verbose(args, _("Server supports filter")); + packet_buf_write(req_buf, "filter %s", spec); + trace2_data_string("fetch", the_repository, + "filter/effective", spec); + } else { + warning("filtering not recognized by server, ignoring"); + trace2_data_string("fetch", the_repository, + "filter/unsupported", spec); + } + } else { + trace2_data_string("fetch", the_repository, + "filter/none", ""); + } +} + static int find_common(struct fetch_negotiator *negotiator, struct fetch_pack_args *args, int fd[2], struct object_id *result_oid, @@ -389,11 +412,7 @@ static int find_common(struct fetch_negotiator *negotiator, packet_buf_write(&req_buf, "deepen-not %s", s->string); } } - if (server_supports_filtering && args->filter_options.choice) { - const char *spec = - expand_list_objects_filter_spec(&args->filter_options); - packet_buf_write(&req_buf, "filter %s", spec); - } + write_and_trace_filter(args, &req_buf, server_supports_filtering); packet_buf_flush(&req_buf); state_len = req_buf.len; @@ -1323,15 +1342,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, die(_("Server does not support shallow requests")); /* Add filter */ - if (server_supports_feature("fetch", "filter", 0) && - args->filter_options.choice) { - const char *spec = - expand_list_objects_filter_spec(&args->filter_options); - print_verbose(args, _("Server supports filter")); - packet_buf_write(&req_buf, "filter %s", spec); - } else if (args->filter_options.choice) { - warning("filtering not recognized by server, ignoring"); - } + write_and_trace_filter(args, &req_buf, + server_supports_feature("fetch", "filter", 0)); if (server_supports_feature("fetch", "packfile-uris", 0)) { int i;
-- 2.37.0.170.g444d1eabd0-goog