From: David Turner <david.turner@twosigma.com>
Date: Fri, 15 Sep 2017 21:35:24 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: <850c2ad20acc4c14be87a767af851b19@exmbdft7.ad.twosigma.com>
In-Reply-To: <20170915192043.4516-5-benpeart@microsoft.com>

> -----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.

[snip]

+>	/*
+>	 * With fsmonitor, we can trust the untracked cache's valid field.
+>	 */

[snip]

> +int read_fsmonitor_extension(struct index_state *istate, const void *data,
> +	unsigned long sz)
> +{

If git_config_get_fsmonitor returns 0, fsmonitor_dirty will leak.

[snip]

> +	/* a fsmonitor process can return '*' to indicate all entries are invalid */

That's not documented in your documentation.  Also, I'm not sure I like it: what 
if I have a file whose name starts with '*'?  Yeah, that would be silly, but this indicates the need 
for the protocol to have some sort of signaling mechanism that's out-of-band  Maybe 
have some key\0value\0 pairs and then \0\0 and then the list of files?  Or, if you want to keep
it really simple, allow an entry of '/' (which is an invalid filename) to mean 'all'.

> +void add_fsmonitor(struct index_state *istate) {
> +	int i;
> +
> +	if (!istate->fsmonitor_last_update) {
[snip]
> +		/* reset the untracked cache */

Is this really necessary?  Shouldn't the untracked cache be in a correct state already? 

> +/*
> + * Clear the given cache entries CE_FSMONITOR_VALID bit and invalidate

Nit: "s/entries/entry's/".