Hi Jeff, On Thu, 24 Feb 2022, Jeff Hostetler wrote: > On 2/24/22 11:22 AM, Johannes Schindelin wrote: > > > 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. > > [...] > > > > 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! > > > > [... describing how FSMonitor protocol v1 is affected...] > > > > 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? > > [...] > > 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. Yes! > 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. That's a good point: I guess if you added a comment to the documentation of `core.fsmonitor = true`, that should be good enough. > [...] > > 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.) That's a really good point. > 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. How about making it an error instead? That should really be helpful: if `core.fsmonitor = true` and `core.fsmonitorHookVersion = 1`, just error out. That way, users will more likely fall into the pit of success. > 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. Oooh! That's _also_ a good point. Maybe we can keep this deprecation out of this here patch series, though. It would be good to get this finished and into `next`, I think. I spent some time (in two separate thrusts) to review the entire patch series, and hope that my feedback was useful to you. In particular with your idea to document the incompatibilities of `core.fsmonitor = true` with Git v2.16.0..v2.26.0, I am really eager to see (the next iteration of) this patch series advance to the `next` branch, and then into an official Git version so that more users can benefit from it. Thank you for all your work on this, Dscho
