Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2017, 02:06 UTC
Message-ID: <xmqq376dgsq4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <7c6bfaef-d05a-4c06-5a53-4f7811c81aec@gmail.com>

Ben Peart <peartben@gmail.com> writes:

Show 8 quoted lines

>>> +	} else {
>>> +		trace_printf_key(&trace_fsmonitor, "fsmonitor not enabled");
>>> +	}
>>> +
>
> I'd remove the trace statement above as it isn't always
> accurate. fsmonitor could be enabled but just hasn't written/read the
> extension yet.

I agree; when it is not enabled, we shouldn't be paying the penalty, either. I wonder if tweak_*() function can return early upfront if we know fsmonitor is not enabled to make it even more obvious.

Show 8 quoted lines

>>> +	if (ignore_fsmonitor)
>>> +		trace_printf_key(&trace_fsmonitor, "Ignoring fsmonitor for %s", ce->name);
>>
>> This is the code path I am fairly certain should not be penalized if
>> tracing is disabled.
>
> Definitely agree with the need to remove this tracing as it will get
> called a lot and we don't want to pay the perf penalty.

Yes.