Re: [PATCH v2 2/4] fsmonitor: handle version 2 of the hooks that will use opaque token

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Kevin,

On Thu, 23 Jan 2020, Kevin Willford via GitGitGadget wrote:

> diff --git a/fsmonitor.c b/fsmonitor.c
> index 9860587225..932bd9012d 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -175,27 +195,60 @@ void refresh_fsmonitor(struct index_state *istate)
>  	 * should be inclusive to ensure we don't miss potential changes.
>  	 */
>  	last_update = getnanotime();
> -	strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
> +	if (hook_version == HOOK_INTERFACE_VERSION1)
> +		strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
>
>  	/*
> -	 * If we have a last update time, call query_fsmonitor for the set of
> -	 * changes since that time, else assume everything is possibly dirty
> +	 * If we have a last update token, call query_fsmonitor for the set of
> +	 * changes since that token, else assume everything is possibly dirty
>  	 * and check it all.
>  	 */
>  	if (istate->fsmonitor_last_update) {
> -		query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,
> -			istate->fsmonitor_last_update, &query_result);
> +		if (hook_version == -1 || hook_version == HOOK_INTERFACE_VERSION2) {
> +			query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION2,
> +				istate->fsmonitor_last_update, &query_result);
> +
> +			if (query_success) {
> +				if (hook_version < 0)
> +					hook_version = HOOK_INTERFACE_VERSION2;
> +
> +				/*
> +				 * First entry will be the last update token
> +				 * Need to use a char * variable because static
> +				 * analysis was suggesting to use strbuf_addbuf
> +				 * but we don't want to copy the entire strbuf
> +				 * only the the chars up to the first NUL
> +				 */
> +				buf = query_result.buf;
> +				strbuf_addstr(&last_update_token, buf);
> +				if (!last_update_token.len) {
> +					warning("Empty last update token.");
> +					query_success = 0;
> +				} else {
> +					bol = last_update_token.len + 1;
> +				}
> +			} else if (hook_version < 0) {
> +				hook_version = HOOK_INTERFACE_VERSION1;
> +				if (!last_update_token.len)
> +					strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
> +			}
> +		}
> +
> +		if (hook_version == HOOK_INTERFACE_VERSION1) {
> +			query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION1,
> +				istate->fsmonitor_last_update, &query_result);

I suspect that `istate->fsmonitor_last_update` might be incorrect here and
that you need `last_update_token.buf` instead. Besides...

> +		}

I could imagine that this would be easier to read if you initialized

	int interface_version = HOOK_INTERFACE_VERSION2;
	const char *token = istate->fsmonitor_last_update;

and then, at the beginning of this hunk, where you already added an `if`,
extend it to

	if (hook_version == HOOK_INTERFACE_VERSION1) {
		interface_version = HOOK_INTERFACE_VERSION1;
		strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
		token = last_update_token.buf;
	}

Now, you can call

		query_success = !query_fsmonitor(interface_version,
			token, &query_result);
		if (!query_success && hook_version < 0) {
			hook_version = HOOK_INTERFACE_VERSION1;
			strbuf_reset(&last_update_token);
			strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
			token = last_update_token.buf;
			query_success = !query_fsmonitor(hook_version,
				token, &query_result);
		}

		if (query_success && interface_version == HOOK_INTERFACE_VERSION2)
			bol = last_update_token.len + 1;


Technically, you could force `hook_version` to non-negative, via:

		if (hook_version < 0) {
			if (query_success)
				hook_version = HOOK_INTERFACE_VERSION2;
			else {
				hook_version = HOOK_INTERFACE_VERSION1;
				strbuf_reset(&last_update_token);
				strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
				token = last_update_token.buf;
				query_success = !query_fsmonitor(hook_version,
					token, &query_result);
			}
		}

but that would not make anything quicker, and would make the code more
convoluted.

It is good that you keep the `fsmonitor-all` and `fsmonitor-watchman`
versions at 1, to verify that this fall-back mechanism works. I wonder
whether we should add one explicit test for that, so that those two hooks
can be upgraded to the interface v2.

Thanks,
Dscho

> +
>  		trace_performance_since(last_update, "fsmonitor process '%s'", core_fsmonitor);
>  		trace_printf_key(&trace_fsmonitor, "fsmonitor process '%s' returned %s",
>  			core_fsmonitor, query_success ? "success" : "failure");
>  	}
>
>  	/* a fsmonitor process can return '/' to indicate all entries are invalid */
> -	if (query_success && query_result.buf[0] != '/') {
> +	if (query_success && query_result.buf[bol] != '/') {
>  		/* Mark all entries returned by the monitor as dirty */
>  		buf = query_result.buf;
> -		bol = 0;
> -		for (i = 0; i < query_result.len; i++) {
> +		for (i = bol; i < query_result.len; i++) {
>  			if (buf[i] != '\0')
>  				continue;
>  			fsmonitor_refresh_callback(istate, buf + bol);
> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> index cf0fda2d5a..fbfdcca000 100755
> --- a/t/t7519-status-fsmonitor.sh
> +++ b/t/t7519-status-fsmonitor.sh
> @@ -32,11 +32,12 @@ write_integration_script () {
>  		echo "$0: exactly 2 arguments expected"
>  		exit 2
>  	fi
> -	if test "$1" != 1
> +	if test "$1" != 2
>  	then
>  		echo "Unsupported core.fsmonitor hook version." >&2
>  		exit 1
>  	fi
> +	printf "last_update_token\0"
>  	printf "untracked\0"
>  	printf "dir1/untracked\0"
>  	printf "dir2/untracked\0"
> @@ -107,6 +108,7 @@ EOF
>  # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit
>  test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' '
>  	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +		printf "last_update_token\0"
>  	EOF
>  	git update-index --fsmonitor &&
>  	git update-index --fsmonitor-valid dir1/modified &&
> @@ -167,6 +169,7 @@ EOF
>  # test that newly added files are marked valid
>  test_expect_success 'newly added files are marked valid' '
>  	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +		printf "last_update_token\0"
>  	EOF
>  	git add new &&
>  	git add dir1/new &&
> @@ -207,6 +210,7 @@ EOF
>  # test that *only* files returned by the integration script get flagged as invalid
>  test_expect_success '*only* files returned by the integration script get flagged as invalid' '
>  	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +	printf "last_update_token\0"
>  	printf "dir1/modified\0"
>  	EOF
>  	clean_repo &&
> @@ -276,6 +280,7 @@ do
>  		# (if enabled) files unless it is told about them.
>  		test_expect_success "status doesn't detect unreported modifications" '
>  			write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +			printf "last_update_token\0"
>  			:>marker
>  			EOF
>  			clean_repo &&
> diff --git a/t/t7519/fsmonitor-all b/t/t7519/fsmonitor-all
> index 691bc94dc2..94ab66bd3d 100755
> --- a/t/t7519/fsmonitor-all
> +++ b/t/t7519/fsmonitor-all
> @@ -17,7 +17,6 @@ fi
>
>  if test "$1" != 1
>  then
> -	echo "Unsupported core.fsmonitor hook version." >&2
>  	exit 1
>  fi
>
> diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
> index d8e7a1e5ba..264b9daf83 100755
> --- a/t/t7519/fsmonitor-watchman
> +++ b/t/t7519/fsmonitor-watchman
> @@ -26,8 +26,7 @@ if ($version == 1) {
>  	# subtract one second to make sure watchman will return all changes
>  	$time = int ($time / 1000000000) - 1;
>  } else {
> -	die "Unsupported query-fsmonitor hook version '$version'.\n" .
> -	    "Falling back to scanning...\n";
> +	exit 1;
>  }
>
>  my $git_work_tree;
> --
> gitgitgadget
>
>