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
