[PATCH 0/1] fsmonitor: fix watchman integration

["Kevin Willford via GitGitGadget" <gitgitgadget@gmail.com>]

When running Git commands quickly -- such as in a shell script or the test
suite -- the Git commands frequently complete and start again during the
same second. The example fsmonitor hooks to integrate with Watchman truncate
the nanosecond times to seconds. In principle, this is fine, as Watchman
claims to use inclusive comparisons [1]. The result should only be an
over-representation of the changed paths since the last Git command.

However, Watchman's own documentation claims "Using a timestamp is prone to
race conditions in understanding the complete state of the file tree" [2].
All of their documented examples use a "clockspec" that looks like
'c:123:234'. Git should eventually learn how to store this type of string to
provide a stronger integration, but that will be a more invasive change.

When using GIT_TEST_FSMONITOR="$(pwd)/t7519/fsmonitor-watchman", scripts
such as t7519-wtstatus.sh fail due to these race conditions. In fact,
running any test script with GIT_TEST_FSMONITOR pointing at
t/t7519/fsmonitor-wathcman will cause failures in the test_commit function.
The 'git add "$indir$file"' command fails due to not enough time between the
creation of '$file' and the 'git add' command.

For now, subtract one second from the timestamp we pass to Watchman. This
will make our window large enough to avoid these race conditions. Increasing
the window causes tests like t7519-wtstatus.sh to pass.

When the integration was introduced in def437671 (fsmonitor: add a sample
integration script for Watchman, 2018-09-22), the query included an
expression that would ignore files created and deleted in that window. The
performance reason for this change was to ignore temporary files created by
a build between Git commands. However, this causes failures in script
scenarios where Git is creating or deleting files quickly.

When using GIT_TEST_FSMONITOR as before, t2203-add-intent.sh fails due to
this add-and-delete race condition.

By removing the "expression" from the Watchman query, we remove this race
condition. It will lead to some performance degradation in the case of users
creating and deleting temporary files inside their working directory between
Git commands. However, that is a cost we need to pay to be correct.

[1] https://github.com/facebook/watchman/blob/master/query/since.cpp#L35-L39
[2] https://facebook.github.io/watchman/docs/clockspec.html

Kevin Willford (1):
  fsmonitor: fix watchman integration

 t/t7519/fsmonitor-watchman                 | 13 ++++---------
 templates/hooks--fsmonitor-watchman.sample | 13 ++++---------
 2 files changed, 8 insertions(+), 18 deletions(-)


base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-444%2Fkewillford%2Ffsmonitor-watchman-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-444/kewillford/fsmonitor-watchman-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/444
-- 
gitgitgadget