RE: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.
- From
- David Turner <david.turner@twosigma.com>
- Date
- Sep 15, 2017, 21:35 UTC
- Message-ID
- <850c2ad20acc4c14be87a767af851b19@exmbdft7.ad.twosigma.com>
- In-Reply-To
- <20170915192043.4516-5-benpeart@microsoft.com>
Show 9 quoted lines
> -----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.
Show 13 quoted lines
> +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/".