Re: [PATCH v2] fetch-pack: write effective filter to trace2

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 18, 2022, 19:47 UTC
Message-ID: <xmqqwncadwzh.fsf@gitster.g>
In-Reply-To: <20220718170027.3993042-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 22 quoted lines

> +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", "");
> +	}
> +}

The previous round already had the same issue, but this makes it even worse by introducing a function that makes it clear that it mixes quite unrelated two features (i.e. write the filter to the other end, which MUST be done for correct operation of the protocol, and write a log to trace2, which may not be even necessary when we are not logging at all).

Can we make the code a bit better structured, I have to wonder, by having two separate phases of processing, one for the operation proper, and the other for the logging/debugging?

In a sense, we can say that the only purpose this helper function is to tell the server end what the filter we use is by renaming it; it is OK to have debugging statements and logging code as part of the implementation of such a function.

I actually like that direction better. A helper function may exist *ONLY* to trace, in which case, having "trace" in its name would make perfect sense. A helper function may exist to perform the real work, but it may log what it did to perform the real work as well. I think the latter shouldn't have "trace" in its name at all, or our helpers will all be called do_FOO_and_trace(), do_BAR_and_debug(), etc., which is nonsense. Just calling do_FOO() and do_BAR(), and then having them log or trace as needed, is fine.

Will queue, anyway.

Thanks.