Hi Jeff, On Fri, 22 Apr 2022, Jeff Hostetler via GitGitGadget wrote: > Here is version 6 of part 3 of FSMonitor. > > This version addresses: > > 1. Junio's comments on V5 23/28 WRT shell variable references and quoting > pathnames in the create_super and create_sub helper functions. > > 2. Ævar's comments on V4 4/27 (sorry I didn't see them until after I sent > V5) WRT somewhat blurry logic around the fsmonitor-settings and > detecting incompatible worktrees. I simplified things, but not to the > level that Ævar was suggesting. For example, in builtin/update-index.c > the suggestion was to detect incompatible FS before taking the lock on > the index, but the lock is taken before the CL args are parsed (because > update-index uses a custom version of parse_options_start()), so we > don't know yet whether the user passed --fsmonitor until much later and > that is what triggers the error/warning. I did replace the return 1 with > a die() so hopefully, we'll release the lock on the index like all of > the other errors in that function. I did try to better document the code > in update-index.c and in builtin/fsmonitor--daemon.c to explain how > things are supposed to work. So hopefully it'll be easier to review. > > 3. Also, in update-index and fsmonitor--daemon, I redid how the error > messages are printed, so that I could use die() in the cmd_*() functions > rather than having calls to error() hidden inside fsmonitor-settings.c. > I think that helped with the above cleanup. Thank you _so_ much for keeping on the ball. I do see how much effort you had to put into FSMonitor, what with three large patch series, plenty of reviews that necessitated plenty of iterations, but I heard from a couple of sides now just how important this feature is for users who work with large repositories. Your work truly has a great impact on Git users! I offered a couple of suggestions in my replies to individual patches, nothing majorly critical (except maybe the `wcscpy()`/`wcsncpy()` calls that _might_ overrun their buffer in cornercases). Hopefully you find the suggestions useful rather than annoying at this late stage (you're already at iteration 6 of the third large patch series, after all). I just didn't want us to run into any big surprises like when the second FSMonitor patch series was integrated into `next` (finally!!!), only to be dropped and needing to be replaced by yet another iteration (not so yay :-( ). After studying your patches for a few hours and then writing up my review (only being interrupted _once_ today, briefly, yay!), I am fairly happy even with the current shape of the series, and if you want to address my suggestions and send out a seventh iteration of the patch series, I am certain that it is ready for `next`. Again, thank you _so much_ for keeping up the good work, Dscho
