Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup

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

Taylor Blau <me@ttaylorr.com> writes:

> Sorry for the confusion. I mean the following:
>
>   - These functions have existing callers that Nipunn claims do not need
>     to be explicitly inlined.

I guess "claims" is the key phrase in your responsehere.  Do you
feel that the claim is not sufficiently substantiated?

Those without fsmonitor would pay the call/return cost for no good
reason if core_fsmonitor is not set, and checking that on the caller
side may make a big difference.  How big?  That needs measurement.

This is a tangent, but with or without inlining, I find it iffy to
see that untracked_cache_invalidate_path() is called only when
fsmonitor is in use.  Does untracked_cache depend on fsmonitor for
its correct operation?  Why is it OK not to invlidate when the
caller would tell fsmonitor that a path is invalid if fsmonitor were
in use?  The call is a statement of fact that the path is no longer
valid, and that bit of information would be useful to the parts of
the system outside fsmonitor, no?  Puzzled....

>   - These functions are being moved to be part of the fsmonitor public
>     interface (presumably so that new callers can be added).

They used to be implemented as static inline functions in the
fsmonitor.h header file, so they have been part of the public
interface anyway.  Anybody that includes fsmonitor.h can use it,
with or without the patch.  So I think this one would not be
a problem.

> ...And I was wondering whether you wanted to wait for new callers
> before applying these to your tree.

Thanks.

I still do not know about the "should the inline be kept" question.
The proposed log message for the commit does not explain (let alone
justify) why "optimization" is unneeded for the fuctions in the
first place, which does not help.