From: David Turner <david.turner@twosigma.com>
Date: Mon, 18 Sep 2017 13:32:36 GMT
Subject: RE: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.
Message-ID: <1f095732aec2469f86a7018aa9f1f8ec@exmbdft7.ad.twosigma.com>
In-Reply-To: <3e0d003e-0643-0359-35fd-a5ecf9b751c3@gmail.com>

> -----Original Message-----
> From: Ben Peart [mailto:peartben@gmail.com]
> Sent: Monday, September 18, 2017 9:07 AM
> To: David Turner <David.Turner@twosigma.com>; 'Ben Peart'
> <benpeart@microsoft.com>
> Cc: avarab@gmail.com; christian.couder@gmail.com; git@vger.kernel.org;
> gitster@pobox.com; johannes.schindelin@gmx.de; pclouds@gmail.com;
> peff@peff.net
> Subject: Re: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file
> system monitor to speed up detecting new or changed files.
> 
> Thanks for taking the time to review/provide feedback!
> 
> On 9/15/2017 5:35 PM, David Turner wrote:
> >> -----Original Message-----
> >> From: Ben Peart [mailto:benpeart@microsoft.com]
> >> Sent: Friday, September 15, 2017 3:21 PM
> >> To: benpeart@microsoft.com
> >> Cc: David Turner <David.Turner@twosigma.com>; avarab@gmail.com;
> >> christian.couder@gmail.com; git@vger.kernel.org; gitster@pobox.com;
> >> johannes.schindelin@gmx.de; pclouds@gmail.com; peff@peff.net
> >> Subject: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize
> >> a file system monitor to speed up detecting new or changed files.
> >
> >> +int git_config_get_fsmonitor(void)
> >> +{
> >> +	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
> >> +		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
> >> +
> >> +	if (core_fsmonitor && !*core_fsmonitor)
> >> +		core_fsmonitor = NULL;
> >> +
> >> +	if (core_fsmonitor)
> >> +		return 1;
> >> +
> >> +	return 0;
> >> +}
> >
> > This functions return values are backwards relative to the rest of the
> git_config_* functions.
> 
> I'm confused.  If core.fsmonitor is configured, it returns 1. If it is not
> configured, it returns 0. I don't make use of the -1 /* default value */ option
> as I didn't see any use/value in this case. What is backwards?

The other git_config_* functions return 1 for error and 0 for success.

> > [snip]
> >
> > +>	/*
> > +>	 * With fsmonitor, we can trust the untracked cache's valid field.
> > +>	 */
> >
> 
> Did you intend to make a comment here?

Sorry.  I was going to make a comment that I didn't see how that could work 
since we weren't touching the untracked cache here, but then I saw the bit 
further down.   I'm still not sure it works (see comment on 10/12), but at
least it could in theory work.