Re: [PATCH v2] trace2: don't include "fsync" events in all trace2 logs

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2022, 20:45 UTC
Message-ID: <xmqqczepev8e.fsf@gitster.g>
In-Reply-To: <patch-v2-1.1-a1fc37de947-20220630T084607Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> As we're needing to indent the trace2_data_intmax() lines let's
> introduce helper variables to ensure that our resulting lines (which
> were already too) don't exceed the recommendations of the

too -> too something

Show 5 quoted lines

> +    elsif ($event eq 'data_json') {
> +	# NEEDSWORK: Ignore due to
> +	# compat/win32/trace2_win32_process_info.c, which should log a
> +	# "cmd_ancestry" event instead.
> +    }

This does read better.

Show 12 quoted lines

>  void trace_git_fsync_stats(void)
>  {
> -	trace2_data_intmax("fsync", the_repository, "fsync/writeout-only", count_fsync_writeout_only);
> -	trace2_data_intmax("fsync", the_repository, "fsync/hardware-flush", count_fsync_hardware_flush);
> +	const struct repository *r = the_repository;
> +	const intmax_t cfwo = count_fsync_writeout_only;
> +	const intmax_t cfhf = count_fsync_hardware_flush;
> +
> +	if (cfwo)
> +		trace2_data_intmax("fsync", r, "fsync/writeout-only", cfwo);
> +	if (cfhf)
> +		trace2_data_intmax("fsync", r, "fsync/hardware-flush", cfhf);

I would have wrapped the lines instead of introducing these extra variables; the key observation is that it would make a much easier pattern to follow for a future developer who needs to add the third kind of "fsync" on top of the existing wo and hf.

But we can address that when somebody actually adds the third one, so let's take the patch as-is.

Thanks.