[0/2] fsmonitor inline / testing cleanup

Message

Linus Arver via GitGitGadget Oct. 21, 2020, 6:04 p.m. UTC

Credit to alexmv again - I'm rebasing these changes from a couple years ago
for contribution.

Full comments are available here -
https://public-inbox.org/git/[e-mail]/
To summarize the relevant points

Re: Inlining mark_fsmonitor_[in]valid peartben said

I'm fine with these not being inline.  I was attempting to minimize the 
performance impact of the fsmonitor code when it was not even turned on. 
  Inlineing these functions allowed it to be kept to a simple test but I 
suspect (especially with modern optimizing compilers) that the overhead 
of calling a function to do that test is negligible.

Re test-dump-fsmonitor peartben suggested keeping the +- syntax as well as
the summary counts dscho suggested dumping the invalid entries

Alex Vandiver (2):
  fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid
  fsmonitor: make output of test-dump-fsmonitor more concise

 fsmonitor.c                    | 19 +++++++++++++++++++
 fsmonitor.h                    | 18 ++----------------
 t/helper/test-dump-fsmonitor.c | 14 ++++++++++++--
 3 files changed, 33 insertions(+), 18 deletions(-)


base-commit: 69986e19ffcfb9af674ae5180689ab7bbf92ed28
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-767%2Fnipunn1313%2Ffsmonitor-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-767/nipunn1313/fsmonitor-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/767

Comments

Taylor Blau Oct. 21, 2020, 8:52 p.m. UTC | #1

Hi Nipunn,

On Wed, Oct 21, 2020 at 06:04:32PM +0000, Nipunn Koorapati via GitGitGadget wrote:
> Credit to alexmv again - I'm rebasing these changes from a couple years ago
> for contribution.
>
> Full comments are available here -
> https://public-inbox.org/git/[e-mail]/
> To summarize the relevant points

I'm fine with both of these patches, but it may help to have a bit
more information about how they will be used. Presumably more patches
are coming that make use of the new public functions, but it'd be good
to know a little bit of why these changes are necessary.

Thanks,
Taylor

Nipunn Koorapati Oct. 21, 2020, 11:15 p.m. UTC | #2

> I'm fine with both of these patches, but it may help to have a bit
> more information about how they will be used. Presumably more patches
> are coming that make use of the new public functions, but it'd be good
> to know a little bit of why these changes are necessary.

I believe the externs are just there to avoid pulling in `dir.h` from
the include file - since
it's only needed in the implementation. I believe at the time
(12/2017) `dir.h` was not imported from
`fsmonitor.h`, but it does appear imported now. I've eliminated the
import of `dir.h` as it
no longer appears necessary - which I will include in the next
iteration of this diff.

The test helper merely makes it easier to debug fsmonitor tests - will
be useful to any
developer working on fsmonitor related changes. I have an upcoming one
related to
fsmonitor in git checkout, which I've also revived, but I'm not 100%
sure I'll get it through,
but regardless, I believe this test-debugging change will be good.

--Nipunn