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