On 2/24/22 11:22 AM, Johannes Schindelin wrote:
Hi Jeff,
On Tue, 22 Feb 2022, Jeff Hostetler wrote:
On 2/17/22 11:06 AM, Johannes Schindelin wrote:
On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:
In this version I removed the core.useBuiltinFSMonitor config
setting and instead extended the existing core.fsmonitor.
I am somewhat surprised that a reviewer suggested this, as it breaks
the common paradigm we use to allow using several Git versions on the
same worktree.
Imagine, for example, that you run a Git version that understands
`core.fsmonitor=true` to imply the built-in FSMonitor, while you
_also_ use an IDE that bundles a slightly older Git version that
mistakes the `true` for meaning the executable `true` (which is not a
FSMonitor at all, but its exit code suggests that everything's fine
and dandy). The result would be that the IDE does not see _any_
updates anymore, but nothing would suggest that anything is wrong.
We can probably warn users about this, and we can also work around the
fact that Git for Windows already uses `core.useBuiltinFSMonitor`, but
it makes me somewhat uneasy nevertheless.
This is a valid concern and I should have thought to mention it when
the suggestion came up on the list. Yes, extending `core.fsmonitor` to
take a boolean or a path could confuse older clients (like ones bundled
with an IDE, like VS).
My assumption was that since we shipped `core.useBuiltinFSMonitor`
in GFW with an experimental label, that normal users would not be
using it at all and especially not from their IDEs, so it wouldn't
matter. And experimental features are just that -- experimental
and subject to change.
But your point is valid -- if someone does have the odd hook called
"true" or "1", they'll get an unexpected result.
I wondered about that for a while, and put that to a test last night. I
set `core.fsmonitor = true` and then modified a file and ran `git status`.
Something I did not expect happened: it picked up on the modified file!
It also printed out a warning:
warning: Empty last update token.
This is the reason why it works: by default, current Git versions assume
that the FSMonitor hook understands the FSMonitor protocol v2, which
starts by the client sending out a token, receiving a new token and then
the paths of the files/directories/symlinks to inspect. Since the program
`true` does _not_ write that token, Git warns that it did not receive a
token and continues as if no FSMonitor had been configured.
So that's good news!
The less good news is that prior to v2.26.0, Git did not support v2 of the
FSMonitor protocol, but only v1. And v1 does not expect such a token. Git
versions between v2.16.0 and v2.26.0 will interpret a successful run of
the `true` executable with an empty output to mean that no files have been
modified.
And indeed, in my tests, after making sure that the Git index had been
refreshed explicitly and then modifying a file and then running `git
status` with v2.16.0, Git did not pick up on the modification.
That's the less good news.
At first I thought that we're pretty safe because nobody should use older
Git versions and enable FSMonitor because FSMonitor protocol v1 is known
to be subject to racy behavior. But then, Git users sometimes do not
completely control which Git versions they use. Take for example Visual
Studio users who also use the Git Bash to work on their worktree. While
their Git Bash might be reasonably recent, Visual Studio comes with its
own embedded Git version. Therefore, a user might want to play with the
built-in FSMonitor in Git Bash, find that it dramatically speeds up
everything (as it does for me, thank you so much!), and not realize that
the Git executable used by Visual Studio totally misinterprets
`core.fsmonitor` to refer to `/usr/bin/true.exe` and then miss any
modifications.
As long as the embedded Git version is at least v2.26.0, Visual Studio
will at least work correctly (because it ignores `true.exe`'s output and
continue as if no FSMonitor had been configured). But as soon as an older
version is used, Git would work incorrectly, without any indication what
is going wrong.
I tried to come up with alternatives (because I _really_ dislike being a
reviewer who only points out what's wrong without any constructive
suggestion how to do it better), and the best alternatives I came up were:
- stick with `core.useBuiltinFSMonitor` as before, or
- use a special value of `core.fsmonitor` that simply is not a valid
executable name. In 2019, when I worked on the original precursor of the
built-in FSMonitor (before I had to drop working on FSMonitor
because of all the security work that went into v2.24.1), I had picked
`:builtin:` because colons are illegal on Windows, but of _course_ they
are legal everywhere else. But one thing is not possible, even on Linux:
to have a trailing slash in an executable name. So something like
`/builtin-fsmonitor/` would work.
However, after seeing how nicely your latest iteration cleans up the code
by simply interpreting a Boolean value to refer to the built-in FSMonitor,
I _really_ would like to make it work.
Maybe we can declare that it is "safe enough" to rely on new enough Git
versions to be used by users who use multiple Git versions on the same
worktree? They should use _at least_ v2.26.1 anyway, because that one
fixed a rather important vulnerability (CVE-2020-5260)? At least for
Visual Studio, this is true: it ships with Git version 2.33.0.windows.2.
What do you think? Can we somehow make `core.fsmonitor = true` work?
Ciao,
Dscho
Thanks for the testing and background information here!
I agree. I would like to keep the current
"core.fsmonitor = <bool> | <path>"
usage that I have in V5.
It cleaned up things very nicely and it got rid of the somewhat awkward
usage of having "core.useBuiltinFSMonitor" override the existing
"core.fsmonitor" setting.
It is unfortunate that it might cause a breakage for users who are
*also* running a Git version between 2.16 ... 2.26. I have to wonder
if it wouldn't be better to spend our energy documenting that users
should upgrade, rather than trying to support interop with them.
The 2.16 ... 2.26 versions don't have the security fix referenced
above.
The 2.16 ... 2.26 versions are based upon the V1 FSMonitor protocol
and don't have fixes for several serious bugs or races in the original
implementation:
* 398a3b0899 (fsmonitor: force a refresh after the index was discarded,
2019-05-07)
* 679f2f9fdd (unpack-trees: skip stat on fsmonitor-valid files, 2019-11-20)
* dfaed02862 (fsmonitor: update documentation for hook version and
watchman hooks, 2020-01-23)
That last fix added the V2 FSMonitor protocol. This is used by both
the hook- and the IPC-based providers.
So anything still using the V1 FSMonitor protocol is going to unreliable
and racy and users should not use it, so I don't think it is worth the
effort to complicate our current solution to maintain compatibility.
(I hate to say that, but they just shouldn't be using V1 any more.)
On a slight tangent, the current code (before my patch series) does
support a "core.fsmonitorhookversion" to allow the client to talk to
a V1 or V2 provider explicitly (vs the default of trying V2 and then
trying V1). The IPC implementation does not use this config setting,
but I could see adding something to emit a warning if it was set to
1 when using the builtin FSMonitor. This might help users who are
*also* running a Git version between 2.26 and 2.35 to understand the
fallback after the true.exe warning that Johannes described.
On another slight tangent, I'm wondering if we want to officially
deprecate the V1 hook code and/or remove support for it from the code.
Jeff
