Re: [PATCH] index-helper: use watchman to avoid refreshing index with lstat()

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 13/05/16 19:27, David Turner wrote:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
[snip]

> +
> +static void reply_to_poke(int client_fd, const char *pid_buf)
> +{
> +	char *capabilities;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +#ifdef USE_WATCHMAN
> +	pid_t client_pid = strtoull(pid_buf, NULL, 10);
> +
> +	prepare_index(client_pid);
> +#endif
> +	capabilities = strchr(pid_buf, ' ');

So, if the pid is *not* followed by a space, the capabilities
will be NULL here, and ...

> +
> +	if (!strcmp(capabilities, " watchman"))

... we segfault here.

> +#ifdef USE_WATCHMAN
> +		packet_buf_write(&sb, "OK watchman");
> +#else
> +		packet_buf_write(&sb, "NAK watchman");
> +#endif
> +	else
> +		packet_buf_write(&sb, "OK");
> +	if (write_in_full(client_fd, sb.buf, sb.len) != sb.len)
> +		warning(_("client write failed"));
> +}
> +
>  static void loop(int fd, int idle_in_seconds)
>  {
>  	assert(idle_in_seconds < INT_MAX / 1000);
> @@ -252,11 +341,15 @@ static void loop(int fd, int idle_in_seconds)
>  			buf[bytes_read] = 0;
>  			if (!strcmp(buf, "refresh")) {
>  				refresh();
> -			} else if (!strcmp(buf, "poke")) {
> -				/*
> -				 * Just a poke to keep us
> -				 * alive, nothing to do.
> -				 */
> +			} else if (starts_with(buf, "poke")) {
> +				if (buf[4] == ' ') {
> +					reply_to_poke(client_fd, buf + 5);
> +				} else {
> +					/*
> +					 * Just a poke to keep us
> +					 * alive, nothing to do.
> +					 */
> +				}
>  			} else {
>  				warning("BUG: Bogus command %s", buf);
>  			}
> diff --git a/read-cache.c b/read-cache.c
> index 1719f5a..8ec4be3 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1235,7 +1235,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
>  		if (!new) {
>  			const char *fmt;
>  
> -			if (really && cache_errno == EINVAL) {
> +			if (really || cache_errno == EINVAL) {
>  				/* If we are doing --really-refresh that
>  				 * means the index is not valid anymore.
>  				 */
> @@ -1375,11 +1375,75 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)
>  	return 0;
>  }
>  
> +static struct untracked_cache_dir *find_untracked_cache_dir(
> +	struct untracked_cache *uc, struct untracked_cache_dir *ucd,
> +	const char *name)
> +{
> +	int component_len;
> +	const char *end;
> +	struct untracked_cache_dir *dir;
> +
> +	if (!*name)
> +		return ucd;
> +
> +	end = strchr(name, '/');
> +	if (end)
> +		component_len = end - name;
> +	else
> +		component_len = strlen(name);
> +
> +	dir = lookup_untracked(uc, ucd, name, component_len);
> +	if (dir)
> +		return find_untracked_cache_dir(uc, dir, name + component_len + 1);
> +
> +	return NULL;
> +}
> +
>  static void mark_no_watchman(size_t pos, void *data)
>  {
>  	struct index_state *istate = data;
> +	struct cache_entry *ce = istate->cache[pos];
> +	struct strbuf sb = STRBUF_INIT;
> +	char *c;
> +	struct untracked_cache_dir *dir;
> +
>  	assert(pos < istate->cache_nr);
> -	istate->cache[pos]->ce_flags |= CE_WATCHMAN_DIRTY;
> +	ce->ce_flags |= CE_WATCHMAN_DIRTY;
> +
> +	if (!istate->untracked || !istate->untracked->root)
> +		return;
> +
> +	strbuf_add(&sb, ce->name, ce_namelen(ce));
> +
> +	for (c = sb.buf + sb.len - 1; c > sb.buf; c--) {
> +		if (*c == '/') {
> +			strbuf_setlen(&sb, c - sb.buf);
> +			break;
> +		}
> +	}
> +
> +	if (c == sb.buf)
> +		strbuf_setlen(&sb, 0);
> +
> +	dir = find_untracked_cache_dir(istate->untracked,
> +				       istate->untracked->root, sb.buf);
> +	if (dir)
> +		dir->valid = 0;
> +
> +	strbuf_release(&sb);
> +}
> +
> +static int mark_untracked_invalid(struct string_list_item *item, void *uc)
> +{
> +	struct untracked_cache *untracked = uc;
> +	struct untracked_cache_dir *dir;
> +
> +	dir = find_untracked_cache_dir(untracked, untracked->root,
> +				       item->string);
> +	if (dir)
> +		dir->valid = 0;
> +
> +	return 0;
>  }
>  
>  static int read_watchman_ext(struct index_state *istate, const void *data,
> @@ -1409,10 +1473,24 @@ static int read_watchman_ext(struct index_state *istate, const void *data,
>  	ewah_each_bit(bitmap, mark_no_watchman, istate);
>  	ewah_free(bitmap);
>  
> -	/*
> -	 * TODO: update the untracked cache from the untracked data in this
> -	 * extension.
> -	 */
> +	if (istate->untracked && istate->untracked->root) {
> +		int i;
> +		const char *untracked;
> +
> +		untracked = (const char *)data + len + 8 + bitmap_size;
> +		for (i = 0; i < untracked_nr; ++i) {
> +			int len = strlen(untracked);
> +			string_list_append(&istate->untracked->invalid_untracked,
> +					   untracked);
> +			untracked += len + 1;
> +		}
> +
> +		for_each_string_list(&istate->untracked->invalid_untracked,
> +			 mark_untracked_invalid, istate->untracked);
> +
> +		if (untracked_nr)
> +			istate->cache_changed |= WATCHMAN_CHANGED;
> +	}
>  	return 0;
>  }
>  
> @@ -1645,29 +1723,88 @@ static void post_read_index_from(struct index_state *istate)
>  	tweak_untracked_cache(istate);
>  }
>  
> +/* in ms */
> +#define WATCHMAN_TIMEOUT 1000
> +
> +static int poke_and_wait_for_reply(int fd)
> +{
> +	int ret = -1;
> +	struct pollfd pollfd;
> +	int bytes_read;
> +	char reply_buf[4096];
> +	const char *requested_capabilities = "";
> +
> +#ifdef USE_WATCHMAN
> +	requested_capabilities = "watchman";
> +#endif
> +
> +	if (fd < 0)
> +		return -1;
> +
> +	if (packet_write_gently(fd, "poke %d %s", getpid(), requested_capabilities))

So, adding the empty capabilities (and more importantly the
separating space) is not so much 'doesn't hurt', rather than 
'prevents a core-dump!' ;-)

> +		return -1;
> +	if (packet_flush_gently(fd))
> +		return -1;

And yes, I'd forgotten about the 'maybe sometime in the future, we
could buffer the packets' ...

ATB,
Ramsay Jones