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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> +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.