Builtin FSMonitor Feature
Subsystems (2)
Re: [PATCH v3 14/34] fsmonitor--daemon: implement 'start' command
In reply to Jeff HostetlerQuoted 95 lines · Jeff Hostetler
My response here is in addition to Dscho's remarks on this topic. He makes excellent points that I'll just #include here. I do want to add my own $0.02 here.
Quoted 80 lines · Ævar Arnfjörð Bjarmason
Quoted 68 lines · Jeff Hostetler via GitGitGadget
builtin/fsmonitor--daemon.c +68
-0+#ifdef GIT_WINDOWS_NATIVE +/* +* Create a background process to run the daemon. It should be completely +* disassociated from the terminal. +* +* Conceptually like `daemonize()` but different because Windows does not +* have `fork(2)`. Spawn a normal Windows child process but without the +* limitations of `start_command()` and `finish_command()`. +* +* The child process execs the "git fsmonitor--daemon run" command. +* +* The current process returns so that the caller can wait for the child +* to startup before exiting. +*/ +static int spawn_background_fsmonitor_daemon (pid_t *pid) +{ +char git_exe[MAX_PATH ]; +struct strvec args =STRVEC_INIT ; +int in, out; + +GetModuleFileNameA (NULL , git_exe,MAX_PATH ); + + in =open ("/dev/null" ,O_RDONLY ); + out =open ("/dev/null" ,O_WRONLY ); + +strvec_push (&args, git_exe); +strvec_push (&args,"fsmonitor--daemon" ); +strvec_push (&args,"run" ); +strvec_pushf (&args,"--ipc-threads=%d" , fsmonitor__ipc_threads); + + *pid =mingw_spawnvpe (args.v[0 ], args.v,NULL ,NULL , in, out, out); +close (in); +close (out); + +strvec_clear (&args); + +if (*pid <0 ) +return error (_ ("could not spawn fsmonitor--daemon in the background" )); + +return 0 ; +} +#else +/* +* Create a background process to run the daemon. It should be completely +* disassociated from the terminal. +* +* This is adapted from `daemonize()`. Use `fork()` to directly +* create and run the daemon in the child process. +* +* The fork-child can just call the run code; it does not need to exec +* it. +* +* The fork-parent returns the child PID so that we can wait for the +* child to startup before exiting. +*/ +static int spawn_background_fsmonitor_daemon (pid_t *pid) +{ + *pid =fork (); + +switch (*pid) { +case 0 : +if (setsid () ==-1 ) +error_errno (_ ("setsid failed" )); +close (0 ); +close (1 ); +close (2 ); +sanitize_stdfds (); +builtin/fsmonitor--daemon.c static int try_to_run_foreground_daemon(void) · line 349
Quoted 10 lines
builtin/fsmonitor--daemon.c +10
-0+return !!fsmonitor_run_daemon (); + +case -1 : +return error_errno (_ ("could not spawn fsmonitor--daemon in the background" )); + +default : + return 0; + } +} +#endif Quoted 9 lines
The
spawn_background_fsmonitor_daemon()function here is almost the same
asdaemonize(). I wonder if this & the Windows-specific one you have here can't be refactored into an API from what's now insetup.c. Then we could makebuiltin/gc.canddaemon.cuse that, so Windows could
have background GC, and we'd have a more battle-tested central codepath for this tricky bit.
Quoted 7 lines
I'd rather not refactor all of this and add unnecessary generality and complexity just to save duplicating some of the code in
daemonize().And I'd rather not destabilize existing commands like gc and daemon by changing the
daemonize()layer on them. If those commands need help, let's have a separate conversation _later_ about what help they need and if it makes sense to combine them.
Johannes suggested in
https://lore.kernel.org/git/[email protected]/
that (if I understand that correctly, and I just skimmed the linked isse some days ago), that even if such a refactoring was done these two functions are solving subtly different problems, or something. I.e. we couldn't use it for daemonize().
Which I'd say is interesting for the code comments/commit message at least, i.e. how they're solving subtly different problems (not being able to run this & not being able to test on Windows I haven't poked at it myself).
Quoted 5 lines
Quoted 4 lines
It seems to me like the only limitations on it are to have this return slightly more general things (e.g. not set its own errors, return structured data), and maybe some callback for what to do in the child/parent.
Quoted 8 lines
There are several issues here when trying to start a background process and we're already on the edge of the behavioral differences between Windows and Unix -- let's not make things more confusing with multiple callbacks, returning structures, custom errors, and etc.
Also, since Windows doesn't do
fork(), we don't have child/parent branches in the call, so this whole "just pretend it's all Unix" model doesn't work.
Fair enough, And I think replied-to above.
Quoted 6 lines
Even if we did pretend I'd still need ifdef'd callback routines to either call
fsmonitor_run_daemon()or build a command line (or have blocks of functions that "just happen to never be called on one platform or the other").
Quoted 8 lines
What I have here is an API that the primary (read: parent) calls and gets back a 0 or -1 (with error message). And that's it. The primary can then wait for the child (whether from fork or CreateProcess) to become responsive or fail to start. And then the primary can exit (with or without error).
So I think we're good. Yes, there is an ifdef here, but I think it is worth it.
FWIW what I was noting here & elsewhere is that yes, you need to ifdef some of it, but the code you're proposing to add here is using a different pattern than the one generally preferred in this codebase.
I.e. check out how we do it for threading, we intentionally compile the "if (thread) {code}" clauses on platforms we know don't have threading, ditto the code around PCRE in grep.c.
Similarly, here in e.g. spawn_background_fsmonitor_daemon just the GetModuleFileNameA() and mingw_spawnvpe() are Windows-specifics (and could be calls to some helper that *is* ifdef'd).
In this case it's not a big deal, but as a general pattern it helps to e.g. avoid subtle syntax errors in nested ifdefs and the like, and generally encourages keeping the ifdef'd code as small as possible.
Quoted 81 lines
Quoted 71 lines
Quoted 59 lines
builtin/fsmonitor--daemon.c +59
-0+/* +* This is adapted from `wait_or_whine()`. Watch the child process and +* let it get started and begin listening for requests on the socket +* before reporting our success. +*/ +static int wait_for_background_startup (pid_t pid_child) +{ +int status; +pid_t pid_seen; +enum ipc_active_state s; +time_t time_limit, now; + +time (&time_limit); + time_limit += fsmonitor__start_timeout_sec; + +for (;;) { + pid_seen =waitpid (pid_child, &status,WNOHANG ); + +if (pid_seen ==-1 ) +return error_errno (_ ("waitpid failed" )); +else if (pid_seen ==0 ) { +/* +* The child is still running (this should be +* the normal case). Try to connect to it on +* the socket and see if it is ready for +* business. +* +* If there is another daemon already running, +* our child will fail to start (possibly +* after a timeout on the lock), but we don't +* care (who responds) if the socket is live. +*/ + s =fsmonitor_ipc__get_state (); +if (s ==IPC_STATE__LISTENING ) +return 0 ; + +time (&now); +if (now > time_limit) +return error (_ ("fsmonitor--daemon not online yet" )); + }else if (pid_seen == pid_child) { +/* +* The new child daemon process shutdown while +* it was starting up, so it is not listening +* on the socket. +* +* Try to ping the socket in the odd chance +* that another daemon started (or was already +* running) while our child was starting. +* +* Again, we don't care who services the socket. +*/ + s =fsmonitor_ipc__get_state (); +if (s ==IPC_STATE__LISTENING ) +return 0 ; + +/* +* We don't care about the WEXITSTATUS() nor +* any of the WIF*(status) values because +* `cmd_fsmonitor__daemon()` does the `!!result` builtin/fsmonitor--daemon.c static int try_to_run_foreground_daemon(void) · line 419
Quoted 10 lines
builtin/fsmonitor--daemon.c +10
-0+* trick on all function return values. +* +* So it is sufficient to just report the +* early shutdown as an error. +*/ +return error (_ ("fsmonitor--daemon failed to start" )); + }else +return error (_ ("waitpid is confused" )); + } +}Ditto this. could we extend the
wait_or_whine()function (or some extended version thereof) to do what you need with callbacks?Quoted 9 lines
It seems the main difference is just being able to pass down a flag for
waitpid(), and the loop needing to check EINTR or not depending on whether WNOHANG is passed.
For e.g. the "We don't care about theWEXITSTATUS()" you'd get that behavior with an adjustedwait_or_whine(). Wouldn't it be better to report what exit status it exits with e.g. if the top-level process is signalled? We do so in trace2 for other things we spawn...
Quoted 8 lines
Again, I don't want to mix my usage here with the existing code and destabilize all existing callers. Here we are spinning to give the child a chance to *start* and confirm that it is in a listening state and ready for connections. We do not wait for the child to exit (unless it dies quickly without becoming ready).
We want to end our wait as soon as we confirm that the child is ready and return. All I really need from the system is
waitpid().
Will this code behave correctly if the daemon we start is signalled per the WIFSIGNALED() cases the code this is derived handles, but this does not?
But sure, I just meant to point out that the flip side to "destabilize all existing callers" is reviewing new code that may be subtly buggy, and those subtle bugs (if any) would be smoked out if we were forced to extend run-command.c, i.e. to use whatever feature(s) this needs for all existing callers.
Re: [PATCH v3 14/34] fsmonitor--daemon: implement 'start' command
Quoted 98 lines · Ævar Arnfjörð Bjarmason
Quoted 95 lines · Jeff Hostetler
My response here is in addition to Dscho's remarks on this topic. He makes excellent points that I'll just #include here. I do want to add my own $0.02 here.
Quoted 74 lines · Ævar Arnfjörð Bjarmason
Quoted 59 lines · Jeff Hostetler via GitGitGadget
builtin/fsmonitor--daemon.c +59
-0+/* +* This is adapted from `wait_or_whine()`. Watch the child process and +* let it get started and begin listening for requests on the socket +* before reporting our success. +*/ +static int wait_for_background_startup (pid_t pid_child) +{ +int status; +pid_t pid_seen; +enum ipc_active_state s; +time_t time_limit, now; + +time (&time_limit); + time_limit += fsmonitor__start_timeout_sec; + +for (;;) { + pid_seen =waitpid (pid_child, &status,WNOHANG ); + +if (pid_seen ==-1 ) +return error_errno (_ ("waitpid failed" )); +else if (pid_seen ==0 ) { +/* +* The child is still running (this should be +* the normal case). Try to connect to it on +* the socket and see if it is ready for +* business. +* +* If there is another daemon already running, +* our child will fail to start (possibly +* after a timeout on the lock), but we don't +* care (who responds) if the socket is live. +*/ + s =fsmonitor_ipc__get_state (); +if (s ==IPC_STATE__LISTENING ) +return 0 ; + +time (&now); +if (now > time_limit) +return error (_ ("fsmonitor--daemon not online yet" )); + }else if (pid_seen == pid_child) { +/* +* The new child daemon process shutdown while +* it was starting up, so it is not listening +* on the socket. +* +* Try to ping the socket in the odd chance +* that another daemon started (or was already +* running) while our child was starting. +* +* Again, we don't care who services the socket. +*/ + s =fsmonitor_ipc__get_state (); +if (s ==IPC_STATE__LISTENING ) +return 0 ; + +/* +* We don't care about the WEXITSTATUS() nor +* any of the WIF*(status) values because +* `cmd_fsmonitor__daemon()` does the `!!result` builtin/fsmonitor--daemon.c static int try_to_run_foreground_daemon(void) · line 419
Quoted 10 lines
builtin/fsmonitor--daemon.c +10
-0+* trick on all function return values. +* +* So it is sufficient to just report the +* early shutdown as an error. +*/ +return error (_ ("fsmonitor--daemon failed to start" )); + }else +return error (_ ("waitpid is confused" )); + } +}Ditto this. could we extend the
wait_or_whine()function (or some extended version thereof) to do what you need with callbacks?Quoted 9 lines
It seems the main difference is just being able to pass down a flag for
waitpid(), and the loop needing to check EINTR or not depending on whether WNOHANG is passed.
For e.g. the "We don't care about theWEXITSTATUS()" you'd get that behavior with an adjustedwait_or_whine(). Wouldn't it be better to report what exit status it exits with e.g. if the top-level process is signalled? We do so in trace2 for other things we spawn...Again, I don't want to mix my usage here with the existing code and destabilize all existing callers. Here we are spinning to give the child a chance to *start* and confirm that it is in a listening state and ready for connections. We do not wait for the child to exit (unless it dies quickly without becoming ready).
Quoted 6 lines
Quoted 2 lines
We want to end our wait as soon as we confirm that the child is ready and return. All I really need from the system is
waitpid().Will this code behave correctly if the daemon we start is signalled per the
WIFSIGNALED()cases the code this is derived handles, but this does not?
We're only waiting until the child gets started and is able to receive requests -- what happens to it after we have confirmed that it is ready is not our concern (after all, the parent is about to exit anyway and the child is going to continue on).
If waitpid() gives us a WIFSIGNALED (or any other WIF*() state) before we have spoken to it, we will return a "failed to start".
But again, that signal would have to arrive immediately after we spawned it and *before* we could talk to it. If the child is signaled after we confirmed it was ready, we don't care because the parent process will be gone.
(If the child is signaled or is killed (or crashes or whatever), the next Git command (like "status") that tries to talk to it will re-start it implicitly -- the git fsmonitor--daemon start command here is an explicit start.)
Quoted 7 lines
But sure, I just meant to point out that the flip side to "destabilize all existing callers" is reviewing new code that may be subtly buggy, and those subtle bugs (if any) would be smoked out if we were forced to extend
run-command.c, i.e. to use whatever feature(s) this needs for all existing callers.
That would/could have a massive footprint. And I've already established that my usage here is sufficiently different from existing uses that the result would be a mess. IMHO.
Jeff
Re: [PATCH v3 14/34] fsmonitor--daemon: implement 'start' command
Quoted 105 lines · Jeff Hostetler
Quoted 97 lines · Ævar Arnfjörð Bjarmason
Quoted 95 lines · Jeff Hostetler
My response here is in addition to Dscho's remarks on this topic. He makes excellent points that I'll just #include here. I do want to add my own $0.02 here.
Quoted 74 lines · Ævar Arnfjörð Bjarmason
Quoted 59 lines · Jeff Hostetler via GitGitGadget
(quoted diff) +59
-0+/* + * This is adapted from `wait_or_whine()`. Watch the child process and + * let it get started and begin listening for requests on the socket + * before reporting our success. + */ +static int wait_for_background_startup(pid_t pid_child) +{ + int status; + pid_t pid_seen; + enum ipc_active_state s; + time_t time_limit, now; + + time(&time_limit); + time_limit += fsmonitor__start_timeout_sec; + + for (;;) { + pid_seen = waitpid(pid_child, &status, WNOHANG); + + if (pid_seen == -1) + return error_errno(_("waitpid failed")); + else if (pid_seen == 0) { + /* + * The child is still running (this should be + * the normal case). Try to connect to it on + * the socket and see if it is ready for + * business. + * + * If there is another daemon already running, + * our child will fail to start (possibly + * after a timeout on the lock), but we don't + * care (who responds) if the socket is live. + */ + s = fsmonitor_ipc__get_state(); + if (s == IPC_STATE__LISTENING) + return 0; + + time(&now); + if (now > time_limit) + return error(_("fsmonitor--daemon not online yet")); + } else if (pid_seen == pid_child) { + /* + * The new child daemon process shutdown while + * it was starting up, so it is not listening + * on the socket. + * + * Try to ping the socket in the odd chance + * that another daemon started (or was already + * running) while our child was starting. + * + * Again, we don't care who services the socket. + */ + s = fsmonitor_ipc__get_state(); + if (s == IPC_STATE__LISTENING) + return 0; + + /* + * We don't care about the WEXITSTATUS() nor + * any of the WIF*(status) values because + * `cmd_fsmonitor__daemon()` does the `!!result`Review
Quoted 10 lines
(quoted diff) +10
-0+ * trick on all function return values. + * + * So it is sufficient to just report the + * early shutdown as an error. + */ + return error(_("fsmonitor--daemon failed to start")); + } else + return error(_("waitpid is confused")); + } +}Ditto this. could we extend the
wait_or_whine()function (or some extended version thereof) to do what you need with callbacks?Quoted 9 lines
It seems the main difference is just being able to pass down a flag for
waitpid(), and the loop needing to check EINTR or not depending on whether WNOHANG is passed.
For e.g. the "We don't care about theWEXITSTATUS()" you'd get that behavior with an adjustedwait_or_whine(). Wouldn't it be better to report what exit status it exits with e.g. if the top-level process is signalled? We do so in trace2 for other things we spawn...Again, I don't want to mix my usage here with the existing code and destabilize all existing callers. Here we are spinning to give the child a chance to *start* and confirm that it is in a listening state and ready for connections. We do not wait for the child to exit (unless it dies quickly without becoming ready).
Quoted 6 lines
Quoted 2 lines
We want to end our wait as soon as we confirm that the child is ready and return. All I really need from the system is
waitpid().Will this code behave correctly if the daemon we start is signalled per
theWIFSIGNALED()cases the code this is derived handles, but this does not?
Quoted 7 lines
We're only waiting until the child gets started and is able to receive requests -- what happens to it after we have confirmed that it is ready is not our concern (after all, the parent is about to exit anyway and the child is going to continue on).
If
waitpid()gives us a WIFSIGNALED (or any other WIF*() state) before we have spoken to it, we will return a "failed to start".
So in wait_or_whine() and finish_command() we capture all of that in trace2 logs, we explicitly don't want that in this case? We do concern ourselves with the exact exit status/signal status etc. of children we start in most other scenarios for trace2 logging purposes.
Quoted 19 lines
But again, that signal would have to arrive immediately after we spawned it and *before* we could talk to it. If the child is signaled after we confirmed it was ready, we don't care because the parent process will be gone.
(If the child is signaled or is killed (or crashes or whatever), the next Git command (like "status") that tries to talk to it will re-start it implicitly -- the
git fsmonitor--daemon startcommand here is an explicit start.)Quoted 7 lines
But sure, I just meant to point out that the flip side to "destabilize
all existing callers" is reviewing new code that may be subtly buggy, and those subtle bugs (if any) would be smoked out if we were forced to extendrun-command.c, i.e. to use whatever feature(s) this needs for all existing callers.
Quoted 3 lines
That would/could have a massive footprint. And I've already established that my usage here is sufficiently different from existing uses that the result would be a mess. IMHO.
I hadn't see this before but it seems pretty much exactly the same code was already added (by you) in 36a7eb68760 (t0052: add simple-ipc tests and t/helper/test-simple-ipc tool, 2021-03-22), perhaps splitting it into a utility function for the two to use with a callback mechanism would reduce the footprint?
What I was suggesting was some continuation of the below.
(I stopped once I noticed the changes I was making to builtin/fsmonitor--daemon.c didn't even compile (almost the entire file is hidden behind a macro), but I've commented on that aspect elsewhere. I.e. it's nice to be able to do tree-wide refactoring without tripping over code hidden by ifdefs)
It passes all current tests for whatever that's worth, obviously not a pretty API, and I'm not claiming it's correct.
But I think it's clear how the trace2/error handling part of it could be further extracted into some utility, so this would just be a mode of run-command.
Not saying you need to do it, but the comments about us explicitly not caring at all about the exit state make me wonder if there's some reason for why someone else would be tripping over some landmine if they did that refactoring.
Anyway, just a thought. I see from other feedback that you seem to be getting pretty exasperated with me.
I'm just trying to help this along, usually being able to piggy-back on existing in-tree code and proving the correctness by passing all in-tree tests with that piggy-backing helps more than hinders.
builtin/fsmonitor--daemon.c +6 -5
@@ -8,6 +8,7 @@#include "simple-ipc.h" #include "khash.h" #include "pkt-line.h" +#include "run-command.h" static const char *const builtin_fsmonitor__daemon_usage[] = {N_ ("git fsmonitor--daemon start [<options>]" ),
@@ -1403,11 +1404,12 @@ static int wait_for_background_startup(pid_t pid_child) time_limit += fsmonitor__start_timeout_sec;for (;;) { +int saved_errno =0 ; + code =wait_or_whine_extended (pid_child, &pid_seen,"TODO" , +0 ,WNOHANG , &saved_errno); pid_seen =waitpid (pid_child, &status,WNOHANG ); -if (pid_seen ==-1 ) -return error_errno (_ ("waitpid failed" )); -else if (pid_seen ==0 ) { +if (pid_seen ==0 ) {/* * The child is still running (this should be * the normal case). Try to connect to it on
@@ -1452,8 +1454,7 @@ static int wait_for_background_startup(pid_t pid_child)* early shutdown as an error. */ return error (_ ("fsmonitor--daemon failed to start" )); - }else -return error (_ ("waitpid is confused" )); + } } }
run-command.c +22 -9
@@ -543,24 +543,28 @@ static inline void set_cloexec(int fd)fcntl (fd,F_SETFD , flags |FD_CLOEXEC ); } -static int wait_or_whine (pid_t pid,const char *argv0,int in_signal) +int wait_or_whine_extended (pid_t pid,pid_t *waiting,const char *argv0, +int in_signal,int waitpid_options,int *failed_errno) {int status, code =-1 ; -pid_t waiting; -int failed_errno =0 ; -while ((waiting =waitpid (pid, &status,0 )) <0 && errno ==EINTR ) +if (waitpid_options &WNOHANG ) + *waiting =waitpid (pid, &status, waitpid_options); +else +while ((*waiting =waitpid (pid, &status, waitpid_options)) <0 && + errno ==EINTR ) ;/* nothing */ +if (in_signal) {if (WIFEXITED (status)) code =WEXITSTATUS (status);return code; } -if (waiting <0 ) { - failed_errno = errno; +if (*waiting <0 ) { + *failed_errno = errno;error_errno ("waitpid for %s failed" , argv0); - }else if (waiting != pid) { + }else if (*waiting != pid) {error ("waitpid is confused (%s)" , argv0); }else if (WIFSIGNALED (status)) { code =WTERMSIG (status);
@@ -574,14 +578,23 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal) code +=128 ; }else if (WIFEXITED (status)) { code =WEXITSTATUS (status); + }else if (waitpid_options &WNOHANG && *waiting ==0 ) { + code =0 ; }else {error ("waitpid is confused (%s)" , argv0); } -clear_child_for_cleanup (pid); +return code; +} +static int wait_or_whine (pid_t pid,const char *argv0,int in_signal) +{ +pid_t ignore; +int failed_errno =0 ; +int ret =wait_or_whine_extended (pid, &ignore, argv0, in_signal,0 , &failed_errno); +clear_child_for_cleanup (pid); errno = failed_errno; -return code; +return ret; }static void trace_add_env (struct strbuf *dst,const char *const *deltaenv)
run-command.h +3 -0
@@ -182,6 +182,9 @@ void child_process_clear(struct child_process *);int is_executable (const char *name); +int wait_or_whine_extended (pid_t pid,pid_t *waiting,const char *argv0, +int in_signal,int waitpid_options,int *failed_errno); +/** * Start a sub-process. Takes a pointer to a `struct child_process` * that specifies the details and returns pipe FDs (if requested).
t/helper/test-simple-ipc.c +9 -13
@@ -9,6 +9,7 @@#include "parse-options.h" #include "thread-utils.h" #include "strvec.h" +#include "run-command.h" #ifndef SUPPORTS_SIMPLE_IPC int cmd__simple_ipc (int argc,const char **argv)
@@ -349,7 +350,7 @@ static int spawn_server(pid_t *pid)*/ static int wait_for_server_startup (pid_t pid_child) { -int status; +int code;pid_t pid_seen;enum ipc_active_state s;time_t time_limit, now;
@@ -358,12 +359,10 @@ static int wait_for_server_startup(pid_t pid_child) time_limit += cl_args.max_wait_sec;for (;;) { - pid_seen =waitpid (pid_child, &status,WNOHANG ); - -if (pid_seen ==-1 ) -return error_errno (_ ("waitpid failed" )); - -else if (pid_seen ==0 ) { +int saved_errno =0 ; + code =wait_or_whine_extended (pid_child, &pid_seen,"TODO" , +0 ,WNOHANG , &saved_errno); +if (pid_seen ==0 ) {/* * The child is still running (this should be * the normal case). Try to connect to it on
@@ -384,9 +383,7 @@ static int wait_for_server_startup(pid_t pid_child)return error (_ ("daemon not online yet" ));continue ; - } - -else if (pid_seen == pid_child) { + }else if (pid_seen == pid_child) {/* * The new child daemon process shutdown while * it was starting up, so it is not listening
@@ -412,10 +409,9 @@ static int wait_for_server_startup(pid_t pid_child)* early shutdown as an error. */ return error (_ ("daemon failed to start" )); + }else if (code) { +BUG ("??" ); } - -else -return error (_ ("waitpid is confused" )); } }
PATCH v3 15/34 fsmonitor: do not try to operate on bare repos
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Bare repos do not have a working directory, so there is no directory for the daemon to register a watch upon. And therefore there are no files within the directory for it to actually watch.
--- builtin/fsmonitor--daemon.c | 8 ++++++++ t/t7519-status-fsmonitor.sh | 16 ++++++++++++++++ 2 files changed, 24 insertions(+)
builtin/fsmonitor--daemon.c +8 -0
@@ -490,6 +490,14 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)die (_ ("invalid 'ipc-threads' value (%d)" ), fsmonitor__ipc_threads); +prepare_repo_settings (the_repository); +if (!the_repository->worktree) +return error (_ ("fsmonitor-daemon does not support bare repos '%s'" ), +xgetcwd ()); +if (the_repository->settings.fsmonitor_mode ==FSMONITOR_MODE_INCOMPATIBLE ) +return error (_ ("fsmonitor-daemon is incompatible with this repo '%s'" ), + the_repository->worktree); +if (!strcmp (subcmd,"start" ))return !!try_to_start_background_daemon ();
t/t7519-status-fsmonitor.sh +16 -0
@@ -394,6 +394,22 @@ test_expect_success 'incompatible bare repo' ' test_cmp expect actual ' +test_expect_success FSMONITOR_DAEMON 'try running fsmonitor-daemon in bare repo' ' + test_when_finished "rm -rf ./bare-clone" && + git clone --bare . ./bare-clone && + test_must_fail git -C ./bare-clone fsmonitor--daemon run 2>actual && + grep "fsmonitor-daemon does not support bare repos" actual +' + +test_expect_success FSMONITOR_DAEMON 'try running fsmonitor-daemon in virtual repo' ' + test_when_finished "rm -rf ./fake-virtual-clone" && + git clone . ./fake-virtual-clone && + test_must_fail git -C ./fake-virtual-clone \ + -c core.virtualfilesystem=true \ + fsmonitor--daemon run 2>actual && + grep "fsmonitor-daemon is incompatible with this repo" actual +' + test_expect_success 'incompatible core.virtualfilesystem' ' test_when_finished "rm -rf ./fake-gvfs-clone" && git clone . ./fake-gvfs-clone &&
Signature · 1 line
gitgitgadget
Re: [PATCH v3 15/34] fsmonitor: do not try to operate on bare repos
Quoted 17 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Bare repos do not have a working directory, so there is no directory for the daemon to register a watch upon. And therefore there are no files within the directory for it to actually watch.
Signed-off-by Jeff Hostetler <[email protected]>--- builtin/fsmonitor--daemon.c | 8 ++++++++ t/t7519-status-fsmonitor.sh | 16 ++++++++++++++++ 2 files changed, 24 insertions(+)builtin/fsmonitor--daemon.c +0
-0
builtin/fsmonitor--daemon.c int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix) · line 499
Quoted 10 lines
builtin/fsmonitor--daemon.c +7
-0@@ -490,6 +490,14 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)die (_ ("invalid 'ipc-threads' value (%d)" ), fsmonitor__ipc_threads); +prepare_repo_settings (the_repository); +if (!the_repository->worktree) +return error (_ ("fsmonitor-daemon does not support bare repos '%s'" ), +xgetcwd ()); +if (the_repository->settings.fsmonitor_mode ==FSMONITOR_MODE_INCOMPATIBLE ) +return error (_ ("fsmonitor-daemon is incompatible with this repo '%s'" ), + the_repository->worktree);
I commented on another patch that that second condition seems like it's only hit under core.virtualfilesystem=true....
builtin/fsmonitor--daemon.c int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
Quoted 6 lines
builtin/fsmonitor--daemon.c +6
-0+test_expect_success FSMONITOR_DAEMON 'try running fsmonitor-daemon in bare repo' ' + test_when_finished"rm -rf ./bare-clone" && + git clone --bare . ./bare-clone && + test_must_fail git -C ./bare-clone fsmonitor--daemon run 2>actual && + grep"fsmonitor-daemon does not support bare repos" actual +'
Isn't just:
git init --bare bare.git
test_must_fail git -C bare.git [...]
Enough, or does the repository need content to get to that error.
builtin/fsmonitor--daemon.c
Quoted 9 lines
builtin/fsmonitor--daemon.c +9
-0+test_expect_success FSMONITOR_DAEMON 'try running fsmonitor-daemon in virtual repo' ' + test_when_finished"rm -rf ./fake-virtual-clone" && + git clone . ./fake-virtual-clone && + test_must_fail git -C ./fake-virtual-clone \ + -c core.virtualfilesystem=true \ + fsmonitor--daemon run 2>actual && + grep"fsmonitor-daemon is incompatible with this repo" actual +' +
Ditto.
PATCH v3 16/34 fsmonitor--daemon: add pathname classification
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Teach fsmonitor--daemon to classify relative and absolute pathnames and decide how they should be handled. This will be used by the platform-specific backend to respond to each filesystem event.
When we register for filesystem notifications on a directory, we get events for everything (recursively) in the directory. We want to report to clients changes to tracked and untracked paths within the working directory. We do not want to report changes within the .git directory, for example.
This classification will be used in a later commit by the different backends to classify paths as events are received.
--- builtin/fsmonitor--daemon.c | 81 +++++++++++++++++++++++++++++++++++++ fsmonitor--daemon.h | 61 ++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+)
builtin/fsmonitor--daemon.c +81 -0
@@ -121,6 +121,87 @@ static int handle_client(void *data,return result; } +#define FSMONITOR_COOKIE_PREFIX ".fsmonitor-daemon-" + +enum fsmonitor_path_type fsmonitor_classify_path_workdir_relative ( +const char *rel) +{ +if (fspathncmp (rel,".git" ,4 )) +return IS_WORKDIR_PATH ; + rel +=4 ; + +if (!*rel) +return IS_DOT_GIT ; +if (*rel !='/' ) +return IS_WORKDIR_PATH ;/* e.g. .gitignore */ + rel++; + +if (!fspathncmp (rel,FSMONITOR_COOKIE_PREFIX , +strlen (FSMONITOR_COOKIE_PREFIX ))) +return IS_INSIDE_DOT_GIT_WITH_COOKIE_PREFIX ; + +return IS_INSIDE_DOT_GIT ; +} + +enum fsmonitor_path_type fsmonitor_classify_path_gitdir_relative ( +const char *rel) +{ +if (!fspathncmp (rel,FSMONITOR_COOKIE_PREFIX , +strlen (FSMONITOR_COOKIE_PREFIX ))) +return IS_INSIDE_GITDIR_WITH_COOKIE_PREFIX ; + +return IS_INSIDE_GITDIR ; +} + +static enum fsmonitor_path_type try_classify_workdir_abs_path ( +struct fsmonitor_daemon_state *state, +const char *path) +{ +const char *rel; + +if (fspathncmp (path, state->path_worktree_watch.buf, + state->path_worktree_watch.len)) +return IS_OUTSIDE_CONE ; + + rel = path + state->path_worktree_watch.len; + +if (!*rel) +return IS_WORKDIR_PATH ;/* it is the root dir exactly */ +if (*rel !='/' ) +return IS_OUTSIDE_CONE ; + rel++; + +return fsmonitor_classify_path_workdir_relative (rel); +} + +enum fsmonitor_path_type fsmonitor_classify_path_absolute ( +struct fsmonitor_daemon_state *state, +const char *path) +{ +const char *rel; +enum fsmonitor_path_type t; + + t =try_classify_workdir_abs_path (state, path); +if (state->nr_paths_watching ==1 ) +return t; +if (t !=IS_OUTSIDE_CONE ) +return t; + +if (fspathncmp (path, state->path_gitdir_watch.buf, + state->path_gitdir_watch.len)) +return IS_OUTSIDE_CONE ; + + rel = path + state->path_gitdir_watch.len; + +if (!*rel) +return IS_GITDIR ;/* it is the <gitdir> exactly */ +if (*rel !='/' ) +return IS_OUTSIDE_CONE ; + rel++; + +return fsmonitor_classify_path_gitdir_relative (rel); +} +static void *fsmonitor_fs_listen__thread_proc (void *_state) {struct fsmonitor_daemon_state *state = _state;
fsmonitor--daemon.h +61 -0
66 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH v3 17/34 fsmonitor--daemon: define token-ids
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Teach fsmonitor--daemon to create token-ids and define the overall token naming scheme.
--- builtin/fsmonitor--daemon.c | 116 +++++++++++++++++++++++++++++++++++- 1 file changed, 115 insertions(+), 1 deletion(-)
builtin/fsmonitor--daemon.c +115 -1
@@ -93,6 +93,120 @@ static int do_as_client__status(void) } } +/* +* Requests to and from a FSMonitor Protocol V2 provider use an opaque +* "token" as a virtual timestamp. Clients can request a summary of all +* created/deleted/modified files relative to a token. In the response, +* clients receive a new token for the next (relative) request. +* +* +* Token Format +* ============ +* +* The contents of the token are private and provider-specific. +* +* For the built-in fsmonitor--daemon, we define a token as follows: +* +* "builtin" ":" <token_id> ":" <sequence_nr> +* +* The "builtin" prefix is used as a namespace to avoid conflicts +* with other providers (such as Watchman). +* +* The <token_id> is an arbitrary OPAQUE string, such as a GUID, +* UUID, or {timestamp,pid}. It is used to group all filesystem +* events that happened while the daemon was monitoring (and in-sync +* with the filesystem). +* +* Unlike FSMonitor Protocol V1, it is not defined as a timestamp +* and does not define less-than/greater-than relationships. +* (There are too many race conditions to rely on file system +* event timestamps.) +* +* The <sequence_nr> is a simple integer incremented whenever the +* daemon needs to make its state public. For example, if 1000 file +* system events come in, but no clients have requested the data, +* the daemon can continue to accumulate file changes in the same +* bin and does not need to advance the sequence number. However, +* as soon as a client does arrive, the daemon needs to start a new +* bin and increment the sequence number. +* +* The sequence number serves as the boundary between 2 sets +* of bins -- the older ones that the client has already seen +* and the newer ones that it hasn't. +* +* When a new <token_id> is created, the <sequence_nr> is reset to +* zero. +* +* +* About Token Ids +* =============== +* +* A new token_id is created: +* +* [1] each time the daemon is started. +* +* [2] any time that the daemon must re-sync with the filesystem +* (such as when the kernel drops or we miss events on a very +* active volume). +* +* [3] in response to a client "flush" command (for dropped event +* testing). +* +* When a new token_id is created, the daemon is free to discard all +* cached filesystem events associated with any previous token_ids. +* Events associated with a non-current token_id will never be sent +* to a client. A token_id change implicitly means that the daemon +* has gap in its event history. +* +* Therefore, clients that present a token with a stale (non-current) +* token_id will always be given a trivial response. +*/ +struct fsmonitor_token_data { +struct strbuf token_id; +struct fsmonitor_batch *batch_head; +struct fsmonitor_batch *batch_tail; +uint64_t client_ref_count; +}; + +static struct fsmonitor_token_data *fsmonitor_new_token_data (void ) +{ +static int test_env_value =-1 ; +static uint64_t flush_count =0 ; +struct fsmonitor_token_data *token; + +CALLOC_ARRAY (token,1 ); + +strbuf_init (&token->token_id,0 ); + token->batch_head =NULL ; + token->batch_tail =NULL ; + token->client_ref_count =0 ; + +if (test_env_value <0 ) + test_env_value =git_env_bool ("GIT_TEST_FSMONITOR_TOKEN" ,0 ); + +if (!test_env_value) { +struct timeval tv; +struct tm tm; +time_t secs; + +gettimeofday (&tv,NULL ); + secs = tv.tv_sec; +gmtime_r (&secs, &tm); + +strbuf_addf (&token->token_id, +"%" PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ" , + flush_count++, +getpid (), + tm.tm_year +1900 , tm.tm_mon +1 , tm.tm_mday, + tm.tm_hour, tm.tm_min, tm.tm_sec, + (long )tv.tv_usec); + }else { +strbuf_addf (&token->token_id,"test_%08x" , test_env_value++); + } + +return token; +} +static ipc_server_application_cb handle_client; static int handle_client(void *data,
@@ -283,7 +397,7 @@ static int fsmonitor_run_daemon(void)pthread_mutex_init (&state.main_lock,NULL ); state.error_code =0 ; - state.current_token_data =NULL ; + state.current_token_data =fsmonitor_new_token_data ();/* Prepare to (recursively) watch the <worktree-root> directory. */ strbuf_init (&state.path_worktree_watch,0 );
Signature · 1 line
gitgitgadget
Re: [PATCH v3 17/34] fsmonitor--daemon: define token-ids
Quoted 6 lines · Jeff Hostetler via GitGitGadget
builtin/fsmonitor--daemon.c +6
-0+if (!test_env_value) { +struct timeval tv; +struct tm tm; +time_t secs; + +gettimeofday (&tv,NULL );
builtin/fsmonitor--daemon.c static int do_as_client__status(void) · line 202
Quoted 10 lines
builtin/fsmonitor--daemon.c +10
-0+ secs = tv.tv_sec; +gmtime_r (&secs, &tm); + +strbuf_addf (&token->token_id, +"%" PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ" , + flush_count++, +getpid (), + tm.tm_year +1900 , tm.tm_mon +1 , tm.tm_mday, + tm.tm_hour, tm.tm_min, tm.tm_sec, + (long )tv.tv_usec);
Just bikeshedding, but can we have tokens that mostly sort numeric-wise by time order? So time at the start, not the flush_count/getpid.
Maybe I'm missing something, but couldn't we just re-use the trace2 SID
+ a more trivial trailer? It would have the nice property that you could find the trace2 SID whenever you looked at such a token (could e.g. split them by "/" too), and add the tv_usec, flush_count+whatever else is needed to make it unique after the "/", no?
Re: [PATCH v3 17/34] fsmonitor--daemon: define token-ids
Quoted 14 lines · Ævar Arnfjörð Bjarmason
Quoted 11 lines · Jeff Hostetler via GitGitGadget
builtin/fsmonitor--daemon.c +11
-0+if (!test_env_value) { +struct timeval tv; +struct tm tm; +time_t secs; + +gettimeofday (&tv,NULL ); + secs = tv.tv_sec; +gmtime_r (&secs, &tm); + + strbuf_addf(&token->token_id, + "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ",
Quoted 8 lines
builtin/fsmonitor--daemon.c static int do_as_client__status(void) · line 202
Quoted 5 lines
builtin/fsmonitor--daemon.c +5
-0+ flush_count++, +getpid (), + tm.tm_year +1900 , tm.tm_mon +1 , tm.tm_mday, + tm.tm_hour, tm.tm_min, tm.tm_sec, + (long)tv.tv_usec);Just bikeshedding, but can we have tokens that mostly sort numeric-wise by time order? So time at the start, not the flush_count/getpid.
As I described in a rather large comment in the code, tokens are opaque strings -- without a less-than / greater-than relationship -- just a random string that the daemon can use (along with a sequence number) to ensure that a later request is well-defined.
Here I'm using a counter, pid, and date-stamp. I'd prefer using a GUID or UUID just to drive that home, but I didn't want to add a new .lib or .a to the build if not necessary.
Perhaps I should compute this portion as hex(hash(time())) to remove the temptation to look inside my opaque token ??
Quoted 7 lines
Maybe I'm missing something, but couldn't we just re-use the trace2 SID
+ a more trivial trailer? It would have the nice property that you could find the trace2 SID whenever you looked at such a token (could e.g. split them by "/" too), and add thetv_usec, flush_count+whatever else is needed to make it unique after the "/", no?
I would rather keep Trace2 out of this. The SID is another opaque string and I don't want to reach inside it.
Jeff
Re: [PATCH v3 17/34] fsmonitor--daemon: define token-ids
Quoted 28 lines · Jeff Hostetler
Quoted 13 lines · Ævar Arnfjörð Bjarmason
Quoted 11 lines · Jeff Hostetler via GitGitGadget
builtin/fsmonitor--daemon.c +11
-0+if (!test_env_value) { +struct timeval tv; +struct tm tm; +time_t secs; + +gettimeofday (&tv,NULL ); + secs = tv.tv_sec; +gmtime_r (&secs, &tm); + + strbuf_addf(&token->token_id, + "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ",Quoted 8 lines
builtin/fsmonitor--daemon.c static int do_as_client__status(void) · line 202
Quoted 5 lines
builtin/fsmonitor--daemon.c +5
-0+ flush_count++, +getpid (), + tm.tm_year +1900 , tm.tm_mon +1 , tm.tm_mday, + tm.tm_hour, tm.tm_min, tm.tm_sec, + (long)tv.tv_usec);Just bikeshedding, but can we have tokens that mostly sort numeric-wise
by time order? So time at the start, not the flush_count/getpid.As I described in a rather large comment in the code, tokens are opaque strings -- without a less-than / greater-than relationship -- just a random string that the daemon can use (along with a sequence number) to ensure that a later request is well-defined.
Quoted 6 lines
Here I'm using a counter, pid, and date-stamp. I'd prefer using a GUID or UUID just to drive that home, but I didn't want to add a new .lib or .a to the build if not necessary.
Perhaps I should compute this portion as hex(hash(time())) to remove the temptation to look inside my opaque token ??
Why does it matter if someone looks inside your opaque token if the code is treating it as opaque by just doing a strcmp(old,new) on it?
I just suggested this as a debugging aid, i.e. when you the human (as opposed to the program) are looking at this behavior it's handy to look at the token and see that your cookies don't match, and that they look to be N seconds apart.
And furthermore, if git crashes or whatever you can now easily look up what process crashed if you've got the leftover cookie, if you've also got trace2 logs.
Quoted 10 lines
Quoted 7 lines
Maybe I'm missing something, but couldn't we just re-use the trace2 SID
+ a more trivial trailer? It would have the nice property that you could find the trace2 SID whenever you looked at such a token (could e.g. split them by "/" too), and add thetv_usec, flush_count+whatever else is needed to make it unique after the "/", no?I would rather keep Trace2 out of this. The SID is another opaque string and I don't want to reach inside it.
For the purposes of the git.git codebase it's fine to reach inside of it, especially for a "I'd like a near-enough-UUID, and I know the trace2 SID already does that per-program", so you just need e.g. a sequence counter within the program to ensure global uniqueness with other git processes for such a cookie.
PATCH v3 18/34 fsmonitor--daemon: create token-based changed path cache
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Teach fsmonitor--daemon to build a list of changed paths and associate them with a token-id. This will be used by the platform-specific backends to accumulate changed paths in response to filesystem events.
The platform-specific file system listener thread receives file system events containing one or more changed pathnames (with whatever bucketing or grouping that is convenient for the file system). These paths are accumulated (without locking) by the file system layer into a fsmonitor_batch.
When the file system layer has drained the kernel event queue, it will "publish" them to our token queue and make them visible to concurrent client worker threads. The token layer is free to combine and/or de-dup paths within these batches for efficient presentation to clients.
--- builtin/fsmonitor--daemon.c | 234 +++++++++++++++++++++++++++++++++++- fsmonitor--daemon.h | 40 ++++++ 2 files changed, 272 insertions(+), 2 deletions(-)
builtin/fsmonitor--daemon.c +232 -2
@@ -168,17 +168,27 @@ struct fsmonitor_token_data {uint64_t client_ref_count; }; +struct fsmonitor_batch { +struct fsmonitor_batch *next; +uint64_t batch_seq_nr; +const char **interned_paths; +size_t nr, alloc; +time_t pinned_time; +}; +static struct fsmonitor_token_data *fsmonitor_new_token_data (void ) {static int test_env_value =-1 ;static uint64_t flush_count =0 ;struct fsmonitor_token_data *token; +struct fsmonitor_batch *batch;CALLOC_ARRAY (token,1 ); + batch =fsmonitor_batch__new ();strbuf_init (&token->token_id,0 ); - token->batch_head =NULL ; - token->batch_tail =NULL ; + token->batch_head = batch; + token->batch_tail = batch; token->client_ref_count =0 ;if (test_env_value <0 )
@@ -204,9 +214,147 @@ static struct fsmonitor_token_data *fsmonitor_new_token_data(void)strbuf_addf (&token->token_id,"test_%08x" , test_env_value++); } +/* +* We created a new <token_id> and are starting a new series +* of tokens with a zero <seq_nr>. +* +* Since clients cannot guess our new (non test) <token_id> +* they will always receive a trivial response (because of the +* mismatch on the <token_id>). The trivial response will +* tell them our new <token_id> so that subsequent requests +* will be relative to our new series. (And when sending that +* response, we pin the current head of the batch list.) +* +* Even if the client correctly guesses the <token_id>, their +* request of "builtin:<token_id>:0" asks for all changes MORE +* RECENT than batch/bin 0. +* +* This implies that it is a waste to accumulate paths in the +* initial batch/bin (because they will never be transmitted). +* +* So the daemon could be running for days and watching the +* file system, but doesn't need to actually accumulate any +* paths UNTIL we need to set a reference point for a later +* relative request. +* +* However, it is very useful for testing to always have a +* reference point set. Pin batch 0 to force early file system +* events to accumulate. +*/ +if (test_env_value) + batch->pinned_time =time (NULL ); +return token; } +struct fsmonitor_batch *fsmonitor_batch__new (void ) +{ +struct fsmonitor_batch *batch; + +CALLOC_ARRAY (batch,1 ); + +return batch; +} + +struct fsmonitor_batch *fsmonitor_batch__pop (struct fsmonitor_batch *batch) +{ +struct fsmonitor_batch *next; + +if (!batch) +return NULL ; + + next = batch->next; + +/* +* The actual strings within the array are interned, so we don't +* own them. +*/ +free (batch->interned_paths); + +return next; +} + +void fsmonitor_batch__add_path (struct fsmonitor_batch *batch, +const char *path) +{ +const char *interned_path =strintern (path); + +trace_printf_key (&trace_fsmonitor,"event: %s" , interned_path); + +ALLOC_GROW (batch->interned_paths, batch->nr +1 , batch->alloc); + batch->interned_paths[batch->nr++] = interned_path; +} + +static void fsmonitor_batch__combine (struct fsmonitor_batch *batch_dest, +const struct fsmonitor_batch *batch_src) +{ +size_t k; + +ALLOC_GROW (batch_dest->interned_paths, + batch_dest->nr + batch_src->nr +1 , + batch_dest->alloc); + +for (k =0 ; k < batch_src->nr; k++) + batch_dest->interned_paths[batch_dest->nr++] = + batch_src->interned_paths[k]; +} + +static void fsmonitor_free_token_data (struct fsmonitor_token_data *token) +{ +struct fsmonitor_batch *p; + +if (!token) +return ; + +assert (token->client_ref_count ==0 ); + +strbuf_release (&token->token_id); + +for (p = token->batch_head; p; p =fsmonitor_batch__pop (p)) + ; + +free (token); +} + +/* +* Flush all of our cached data about the filesystem. Call this if we +* lose sync with the filesystem and miss some notification events. +* +* [1] If we are missing events, then we no longer have a complete +* history of the directory (relative to our current start token). +* We should create a new token and start fresh (as if we just +* booted up). +* +* If there are no concurrent threads readering the current token data +* series, we can free it now. Otherwise, let the last reader free +* it. +* +* Either way, the old token data series is no longer associated with +* our state data. +*/ +static void with_lock__do_force_resync (struct fsmonitor_daemon_state *state) +{ +/* assert current thread holding state->main_lock */ + +struct fsmonitor_token_data *free_me =NULL ; +struct fsmonitor_token_data *new_one =NULL ; + + new_one =fsmonitor_new_token_data (); + +if (state->current_token_data->client_ref_count ==0 ) + free_me = state->current_token_data; + state->current_token_data = new_one; + +fsmonitor_free_token_data (free_me); +} + +void fsmonitor_force_resync (struct fsmonitor_daemon_state *state) +{ +pthread_mutex_lock (&state->main_lock); +with_lock__do_force_resync (state); +pthread_mutex_unlock (&state->main_lock); +} +static ipc_server_application_cb handle_client; static int handle_client(void *data,
@@ -316,6 +464,81 @@ enum fsmonitor_path_type fsmonitor_classify_path_absolute(return fsmonitor_classify_path_gitdir_relative (rel); } +/* +* We try to combine small batches at the front of the batch-list to avoid +* having a long list. This hopefully makes it a little easier when we want +* to truncate and maintain the list. However, we don't want the paths array +* to just keep growing and growing with realloc, so we insert an arbitrary +* limit. +*/ +#define MY_COMBINE_LIMIT (1024) + +void fsmonitor_publish (struct fsmonitor_daemon_state *state, +struct fsmonitor_batch *batch, +const struct string_list *cookie_names) +{ +if (!batch && !cookie_names->nr) +return ; + +pthread_mutex_lock (&state->main_lock); + +if (batch) { +struct fsmonitor_batch *head; + + head = state->current_token_data->batch_head; +if (!head) { +BUG ("token does not have batch" ); + }else if (head->pinned_time) { +/* +* We cannot alter the current batch list +* because: +* +* [a] it is being transmitted to at least one +* client and the handle_client() thread has a +* ref-count, but not a lock on the batch list +* starting with this item. +* +* [b] it has been transmitted in the past to +* at least one client such that future +* requests are relative to this head batch. +* +* So, we can only prepend a new batch onto +* the front of the list. +*/ + batch->batch_seq_nr = head->batch_seq_nr +1 ; + batch->next = head; + state->current_token_data->batch_head = batch; + }else if (!head->batch_seq_nr) { +/* +* Batch 0 is unpinned. See the note in +* `fsmonitor_new_token_data()` about why we +* don't need to accumulate these paths. +*/ +fsmonitor_batch__pop (batch); + }else if (head->nr + batch->nr >MY_COMBINE_LIMIT ) { +/* +* The head batch in the list has never been +* transmitted to a client, but folding the +* contents of the new batch onto it would +* exceed our arbitrary limit, so just prepend +* the new batch onto the list. +*/ + batch->batch_seq_nr = head->batch_seq_nr +1 ; + batch->next = head; + state->current_token_data->batch_head = batch; + }else { +/* +* We are free to append the paths in the given +* batch onto the end of the current head batch. +*/ +fsmonitor_batch__combine (head, batch); +fsmonitor_batch__pop (batch); + } + } + +pthread_mutex_unlock (&state->main_lock); +} +static void *fsmonitor_fs_listen__thread_proc (void *_state) {struct fsmonitor_daemon_state *state = _state;
@@ -330,6 +553,13 @@ static void *fsmonitor_fs_listen__thread_proc(void *_state)fsmonitor_fs_listen__loop (state); +pthread_mutex_lock (&state->main_lock); +if (state->current_token_data && + state->current_token_data->client_ref_count ==0 ) +fsmonitor_free_token_data (state->current_token_data); + state->current_token_data =NULL ; +pthread_mutex_unlock (&state->main_lock); +trace2_thread_exit ();return NULL ; }
fsmonitor--daemon.h +40 -0
@@ -12,6 +12,27 @@struct fsmonitor_batch ;struct fsmonitor_token_data ; +/* +* Create a new batch of path(s). The returned batch is considered +* private and not linked into the fsmonitor daemon state. The caller +* should fill this batch with one or more paths and then publish it. +*/ +struct fsmonitor_batch *fsmonitor_batch__new (void ); + +/* +* Free this batch and return the value of the batch->next field. +*/ +struct fsmonitor_batch *fsmonitor_batch__pop (struct fsmonitor_batch *batch); + +/* +* Add this path to this batch of modified files. +* +* The batch should be private and NOT (yet) linked into the fsmonitor +* daemon state and therefore not yet visible to worker threads and so +* no locking is required. +*/ +void fsmonitor_batch__add_path (struct fsmonitor_batch *batch,const char *path); +struct fsmonitor_daemon_backend_data ;/* opaque platform-specific data */ struct fsmonitor_daemon_state {
@@ -91,5 +112,24 @@ enum fsmonitor_path_type fsmonitor_classify_path_absolute(struct fsmonitor_daemon_state *state, const char *path); +/* +* Prepend the this batch of path(s) onto the list of batches associated +* with the current token. This makes the batch visible to worker threads. +* +* The caller no longer owns the batch and must not free it. +* +* Wake up the client threads waiting on these cookies. +*/ +void fsmonitor_publish (struct fsmonitor_daemon_state *state, +struct fsmonitor_batch *batch, +const struct string_list *cookie_names); + +/* +* If the platform-specific layer loses sync with the filesystem, +* it should call this to invalidate cached data and abort waiting +* threads. +*/ +void fsmonitor_force_resync (struct fsmonitor_daemon_state *state); +#endif /* HAVE_FSMONITOR_DAEMON_BACKEND */ #endif /* FSMONITOR_DAEMON_H */
Signature · 1 line
gitgitgadget
PATCH v3 19/34 fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.
--- compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++ 1 file changed, 530 insertions(+)
compat/fsmonitor/fsmonitor-fs-listen-win32.c +530 -0
@@ -2,20 +2,550 @@#include "config.h" #include "fsmonitor.h" #include "fsmonitor-fs-listen.h" +#include "fsmonitor--daemon.h" + +/* +* The documentation of ReadDirectoryChangesW() states that the maximum +* buffer size is 64K when the monitored directory is remote. +* +* Larger buffers may be used when the monitored directory is local and +* will help us receive events faster from the kernel and avoid dropped +* events. +* +* So we try to use a very large buffer and silently fallback to 64K if +* we get an error. +*/ +#define MAX_RDCW_BUF_FALLBACK (65536) +#define MAX_RDCW_BUF (65536 * 8) + +struct one_watch +{ +char buffer[MAX_RDCW_BUF ]; +DWORD buf_len; +DWORD count; + +struct strbuf path; +HANDLE hDir; +HANDLE hEvent; +OVERLAPPED overlapped; + +/* +* Is there an active ReadDirectoryChangesW() call pending. If so, we +* need to later call GetOverlappedResult() and possibly CancelIoEx(). +*/ +BOOL is_active; +}; + +struct fsmonitor_daemon_backend_data +{ +struct one_watch *watch_worktree; +struct one_watch *watch_gitdir; + +HANDLE hEventShutdown; + +HANDLE hListener[3 ];/* we don't own these handles */ +#define LISTENER_SHUTDOWN 0 +#define LISTENER_HAVE_DATA_WORKTREE 1 +#define LISTENER_HAVE_DATA_GITDIR 2 +int nr_listener_handles; +}; + +/* +* Convert the WCHAR path from the notification into UTF8 and +* then normalize it. +*/ +static int normalize_path_in_utf8 (FILE_NOTIFY_INFORMATION *info, +struct strbuf *normalized_path) +{ +int reserve; +int len =0 ; + +strbuf_reset (normalized_path); +if (!info->FileNameLength) + goto normalize; + +/* +* Pre-reserve enough space in the UTF8 buffer for +* each Unicode WCHAR character to be mapped into a +* sequence of 2 UTF8 characters. That should let us +* avoid ERROR_INSUFFICIENT_BUFFER 99.9+% of the time. +*/ + reserve = info->FileNameLength +1 ; +strbuf_grow (normalized_path, reserve); + +for (;;) { + len =WideCharToMultiByte (CP_UTF8 ,0 , info->FileName, + info->FileNameLength /sizeof (WCHAR ), + normalized_path->buf, +strbuf_avail (normalized_path) -1 , +NULL ,NULL ); +if (len >0 ) + goto normalize; +if (GetLastError () !=ERROR_INSUFFICIENT_BUFFER ) { +error ("[GLE %ld] could not convert path to UTF-8: '%.*ls'" , +GetLastError (), + (int )(info->FileNameLength /sizeof (WCHAR )), + info->FileName); +return -1 ; + } + +strbuf_grow (normalized_path, +strbuf_avail (normalized_path) + reserve); + } + +normalize: +strbuf_setlen (normalized_path, len); +return strbuf_normalize_path (normalized_path); +}void fsmonitor_fs_listen__stop_async (struct fsmonitor_daemon_state *state) { +SetEvent (state->backend_data->hListener[LISTENER_SHUTDOWN ]); +} + +static struct one_watch *create_watch (struct fsmonitor_daemon_state *state, +const char *path) +{ +struct one_watch *watch =NULL ; +DWORD desired_access =FILE_LIST_DIRECTORY ; +DWORD share_mode = +FILE_SHARE_WRITE |FILE_SHARE_READ |FILE_SHARE_DELETE ; +HANDLE hDir; + + hDir =CreateFileA (path, + desired_access, share_mode,NULL ,OPEN_EXISTING , +FILE_FLAG_BACKUP_SEMANTICS |FILE_FLAG_OVERLAPPED , +NULL ); +if (hDir ==INVALID_HANDLE_VALUE ) { +error (_ ("[GLE %ld] could not watch '%s'" ), +GetLastError (), path); +return NULL ; + } + +CALLOC_ARRAY (watch,1 ); + + watch->buf_len =sizeof (watch->buffer);/* assume full MAX_RDCW_BUF */ + +strbuf_init (&watch->path,0 ); +strbuf_addstr (&watch->path, path); + + watch->hDir = hDir; + watch->hEvent =CreateEvent (NULL , TRUE, FALSE,NULL ); + +return watch; +} + +static void destroy_watch (struct one_watch *watch) +{ +if (!watch) +return ; + +strbuf_release (&watch->path); +if (watch->hDir !=INVALID_HANDLE_VALUE ) +CloseHandle (watch->hDir); +if (watch->hEvent !=INVALID_HANDLE_VALUE ) +CloseHandle (watch->hEvent); + +free (watch); +} + +static int start_rdcw_watch (struct fsmonitor_daemon_backend_data *data, +struct one_watch *watch) +{ +DWORD dwNotifyFilter = +FILE_NOTIFY_CHANGE_FILE_NAME | +FILE_NOTIFY_CHANGE_DIR_NAME | +FILE_NOTIFY_CHANGE_ATTRIBUTES | +FILE_NOTIFY_CHANGE_SIZE | +FILE_NOTIFY_CHANGE_LAST_WRITE | +FILE_NOTIFY_CHANGE_CREATION ; + +ResetEvent (watch->hEvent); + +memset (&watch->overlapped,0 ,sizeof (watch->overlapped)); + watch->overlapped.hEvent = watch->hEvent; + +start_watch: +/* +* Queue an async call using Overlapped IO. This returns immediately. +* Our event handle will be signalled when the real result is available. +* +* The return value here just means that we successfully queued it. +* We won't know if the Read...() actually produces data until later. +*/ + watch->is_active =ReadDirectoryChangesW ( + watch->hDir, watch->buffer, watch->buf_len, TRUE, + dwNotifyFilter, &watch->count, &watch->overlapped,NULL ); + +/* +* The kernel throws an invalid parameter error when our buffer +* is too big and we are pointed at a remote directory (and possibly +* for other reasons). Quietly set it down and try again. +* +* See note about MAX_RDCW_BUF at the top. +*/ +if (!watch->is_active && +GetLastError () ==ERROR_INVALID_PARAMETER && + watch->buf_len >MAX_RDCW_BUF_FALLBACK ) { + watch->buf_len =MAX_RDCW_BUF_FALLBACK ; + goto start_watch; + } + +if (watch->is_active) +return 0 ; + +error ("ReadDirectoryChangedW failed on '%s' [GLE %ld]" , + watch->path.buf,GetLastError ()); +return -1 ; +} + +static int recv_rdcw_watch (struct one_watch *watch) +{ + watch->is_active = FALSE; + +/* +* The overlapped result is ready. If the Read...() was successful +* we finally receive the actual result into our buffer. +*/ +if (GetOverlappedResult (watch->hDir, &watch->overlapped, &watch->count, + TRUE)) +return 0 ; + +/* +* NEEDSWORK: If an external <gitdir> is deleted, the above +* returns an error. I'm not sure that there's anything that +* we can do here other than failing -- the <worktree>/.git +* link file would be broken anyway. We might try to check +* for that and return a better error message, but I'm not +* sure it is worth it. +*/ + +error ("GetOverlappedResult failed on '%s' [GLE %ld]" , + watch->path.buf,GetLastError ()); +return -1 ; +} + +static void cancel_rdcw_watch (struct one_watch *watch) +{ +DWORD count; + +if (!watch || !watch->is_active) +return ; + +/* +* The calls to ReadDirectoryChangesW() and GetOverlappedResult() +* form a "pair" (my term) where we queue an IO and promise to +* hang around and wait for the kernel to give us the result. +* +* If for some reason after we queue the IO, we have to quit +* or otherwise not stick around for the second half, we must +* tell the kernel to abort the IO. This prevents the kernel +* from writing to our buffer and/or signalling our event +* after we free them. +* +* (Ask me how much fun it was to track that one down). +*/ +CancelIoEx (watch->hDir, &watch->overlapped); +GetOverlappedResult (watch->hDir, &watch->overlapped, &count, TRUE); + watch->is_active = FALSE; +} + +/* +* Process filesystem events that happen anywhere (recursively) under the +* <worktree> root directory. For a normal working directory, this includes +* both version controlled files and the contents of the .git/ directory. +* +* If <worktree>/.git is a file, then we only see events for the file +* itself. +*/ +static int process_worktree_events (struct fsmonitor_daemon_state *state) +{ +struct fsmonitor_daemon_backend_data *data = state->backend_data; +struct one_watch *watch = data->watch_worktree; +struct strbuf path =STRBUF_INIT ; +struct string_list cookie_list =STRING_LIST_INIT_DUP ; +struct fsmonitor_batch *batch =NULL ; +const char *p = watch->buffer; + +/* +* If the kernel gets more events than will fit in the kernel +* buffer associated with our RDCW handle, it drops them and +* returns a count of zero. +* +* Yes, the call returns WITHOUT error and with length zero. +* +* (The "overflow" case is not ambiguous with the "no data" case +* because we did an INFINITE wait.) +* +* This means we have a gap in coverage. Tell the daemon layer +* to resync. +*/ +if (!watch->count) { +trace2_data_string ("fsmonitor" ,NULL ,"fsm-listen/kernel" , +"overflow" ); +fsmonitor_force_resync (state); +return LISTENER_HAVE_DATA_WORKTREE ; + } + +/* +* On Windows, `info` contains an "array" of paths that are +* relative to the root of whichever directory handle received +* the event. +*/ +for (;;) { +FILE_NOTIFY_INFORMATION *info = (void *)p; +const char *slash; +enum fsmonitor_path_type t; + +strbuf_reset (&path); +if (normalize_path_in_utf8 (info, &path) ==-1 ) + goto skip_this_path; + + t =fsmonitor_classify_path_workdir_relative (path.buf); + +switch (t) { +case IS_INSIDE_DOT_GIT_WITH_COOKIE_PREFIX : +/* special case cookie files within .git */ + +/* Use just the filename of the cookie file. */ + slash =find_last_dir_sep (path.buf); +string_list_append (&cookie_list, + slash ? slash +1 : path.buf); +break ; + +case IS_INSIDE_DOT_GIT : +/* ignore everything inside of "<worktree>/.git/" */ +break ; + +case IS_DOT_GIT : +/* "<worktree>/.git" was deleted (or renamed away) */ +if ((info->Action ==FILE_ACTION_REMOVED ) || + (info->Action ==FILE_ACTION_RENAMED_OLD_NAME )) { +trace2_data_string ("fsmonitor" ,NULL , +"fsm-listen/dotgit" , +"removed" ); + goto force_shutdown; + } +break ; + +case IS_WORKDIR_PATH : +/* queue normal pathname */ +if (!batch) + batch =fsmonitor_batch__new (); +fsmonitor_batch__add_path (batch, path.buf); +break ; + +case IS_GITDIR : +case IS_INSIDE_GITDIR : +case IS_INSIDE_GITDIR_WITH_COOKIE_PREFIX : +default : +BUG ("unexpected path classification '%d' for '%s'" , + t, path.buf); + } + +skip_this_path: +if (!info->NextEntryOffset) +break ; + p += info->NextEntryOffset; + } + +fsmonitor_publish (state, batch, &cookie_list); + batch =NULL ; +string_list_clear (&cookie_list,0 ); +strbuf_release (&path); +return LISTENER_HAVE_DATA_WORKTREE ; + +force_shutdown: +fsmonitor_batch__pop (batch); +string_list_clear (&cookie_list,0 ); +strbuf_release (&path); +return LISTENER_SHUTDOWN ; +} + +/* +* Process filesystem events that happened anywhere (recursively) under the +* external <gitdir> (such as non-primary worktrees or submodules). +* We only care about cookie files that our client threads created here. +* +* Note that we DO NOT get filesystem events on the external <gitdir> +* itself (it is not inside something that we are watching). In particular, +* we do not get an event if the external <gitdir> is deleted. +*/ +static int process_gitdir_events (struct fsmonitor_daemon_state *state) +{ +struct fsmonitor_daemon_backend_data *data = state->backend_data; +struct one_watch *watch = data->watch_gitdir; +struct strbuf path =STRBUF_INIT ; +struct string_list cookie_list =STRING_LIST_INIT_DUP ; +const char *p = watch->buffer; + +if (!watch->count) { +trace2_data_string ("fsmonitor" ,NULL ,"fsm-listen/kernel" , +"overflow" ); +fsmonitor_force_resync (state); +return LISTENER_HAVE_DATA_GITDIR ; + } + +for (;;) { +FILE_NOTIFY_INFORMATION *info = (void *)p; +const char *slash; +enum fsmonitor_path_type t; + +strbuf_reset (&path); +if (normalize_path_in_utf8 (info, &path) ==-1 ) + goto skip_this_path; + + t =fsmonitor_classify_path_gitdir_relative (path.buf); + +switch (t) { +case IS_INSIDE_GITDIR_WITH_COOKIE_PREFIX : +/* special case cookie files within gitdir */ + +/* Use just the filename of the cookie file. */ + slash =find_last_dir_sep (path.buf); +string_list_append (&cookie_list, + slash ? slash +1 : path.buf); +break ; + +case IS_INSIDE_GITDIR : + goto skip_this_path; + +default : +BUG ("unexpected path classification '%d' for '%s'" , + t, path.buf); + } + +skip_this_path: +if (!info->NextEntryOffset) +break ; + p += info->NextEntryOffset; + } + +fsmonitor_publish (state,NULL , &cookie_list); +string_list_clear (&cookie_list,0 ); +strbuf_release (&path); +return LISTENER_HAVE_DATA_GITDIR ; }void fsmonitor_fs_listen__loop (struct fsmonitor_daemon_state *state) { +struct fsmonitor_daemon_backend_data *data = state->backend_data; +DWORD dwWait; + + state->error_code =0 ; + +if (start_rdcw_watch (data, data->watch_worktree) ==-1 ) + goto force_error_stop; + +if (data->watch_gitdir && +start_rdcw_watch (data, data->watch_gitdir) ==-1 ) + goto force_error_stop; + +for (;;) { + dwWait =WaitForMultipleObjects (data->nr_listener_handles, + data->hListener, + FALSE,INFINITE ); + +if (dwWait ==WAIT_OBJECT_0 +LISTENER_HAVE_DATA_WORKTREE ) { +if (recv_rdcw_watch (data->watch_worktree) ==-1 ) + goto force_error_stop; +if (process_worktree_events (state) ==LISTENER_SHUTDOWN ) + goto force_shutdown; +if (start_rdcw_watch (data, data->watch_worktree) ==-1 ) + goto force_error_stop; +continue ; + } + +if (dwWait ==WAIT_OBJECT_0 +LISTENER_HAVE_DATA_GITDIR ) { +if (recv_rdcw_watch (data->watch_gitdir) ==-1 ) + goto force_error_stop; +if (process_gitdir_events (state) ==LISTENER_SHUTDOWN ) + goto force_shutdown; +if (start_rdcw_watch (data, data->watch_gitdir) ==-1 ) + goto force_error_stop; +continue ; + } + +if (dwWait ==WAIT_OBJECT_0 +LISTENER_SHUTDOWN ) + goto clean_shutdown; + +error (_ ("could not read directory changes [GLE %ld]" ), +GetLastError ()); + goto force_error_stop; + } + +force_error_stop: + state->error_code =-1 ; + +force_shutdown: +/* +* Tell the IPC thead pool to stop (which completes the await +* in the main thread (which will also signal this thread (if +* we are still alive))). +*/ +ipc_server_stop_async (state->ipc_server_data); + +clean_shutdown: +cancel_rdcw_watch (data->watch_worktree); +cancel_rdcw_watch (data->watch_gitdir); }int fsmonitor_fs_listen__ctor (struct fsmonitor_daemon_state *state) { +struct fsmonitor_daemon_backend_data *data; + +CALLOC_ARRAY (data,1 ); + + data->hEventShutdown =CreateEvent (NULL , TRUE, FALSE,NULL ); + + data->watch_worktree =create_watch (state, + state->path_worktree_watch.buf); +if (!data->watch_worktree) + goto failed; + +if (state->nr_paths_watching >1 ) { + data->watch_gitdir =create_watch (state, + state->path_gitdir_watch.buf); +if (!data->watch_gitdir) + goto failed; + } + + data->hListener[LISTENER_SHUTDOWN ] = data->hEventShutdown; + data->nr_listener_handles++; + + data->hListener[LISTENER_HAVE_DATA_WORKTREE ] = + data->watch_worktree->hEvent; + data->nr_listener_handles++; + +if (data->watch_gitdir) { + data->hListener[LISTENER_HAVE_DATA_GITDIR ] = + data->watch_gitdir->hEvent; + data->nr_listener_handles++; + } + + state->backend_data = data; +return 0 ; + +failed: +CloseHandle (data->hEventShutdown); +destroy_watch (data->watch_worktree); +destroy_watch (data->watch_gitdir); +return -1 ; }void fsmonitor_fs_listen__dtor (struct fsmonitor_daemon_state *state) { +struct fsmonitor_daemon_backend_data *data; + +if (!state || !state->backend_data) +return ; + + data = state->backend_data; + +CloseHandle (data->hEventShutdown); +destroy_watch (data->watch_worktree); +destroy_watch (data->watch_gitdir); + +FREE_AND_NULL (state->backend_data); }
Signature · 1 line
gitgitgadget
Re: [PATCH v3 19/34] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
Quoted 10 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.
Signed-off-by Jeff Hostetler <[email protected]>--- compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++
<bikeshed mode> Spying on the early history of this (looking for the Linux backend) I saw that at some point we had just compat/fsmonitor/linux.c, and presumably some of compat/fsmonitor/{windows,win32,macos,darwin}.c.
At some point those filenames became much much longer.
I've noticed you tend to prefer really long file and function names, e.g. your borrowed daemonize() became spawn_background_fsmonitor_daemon(), I think aiming for shorter filenames & function names helps, e.g. these long names widen diffstats, and many people who hack on the code stick religiously to 80 character width terminals.
Re: [PATCH v3 19/34] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
Quoted 14 lines · Ævar Arnfjörð Bjarmason
Quoted 10 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.
Signed-off-by Jeff Hostetler <[email protected]>--- compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++
Quoted 7 lines
<bikeshed mode> Spying on the early history of this (looking for the Linux backend) I saw that at some point we had just
compat/fsmonitor/linux.c, and presumably some ofcompat/fsmonitor/{windows,win32,macos,darwin}.c.At some point those filenames became much much longer.
Once upon a time having "foo/bar/win32.c" and "abc/def/win32.c" would cause confusion in the debugger (I've long since forgotten which). Breaking at win32.c:30 was no longer unique.
Also, if the Makefile sends all .o's to the root directory or a unified OBJS directory rather than to the subdir containing the .c, then we have another issue during linking...
So, having been burned too many times, I prefer to make source filenames unique when possible.
Quoted 7 lines
I've noticed you tend to prefer really long file and function names, e.g. your borrowed
daemonize()becamespawn_background_fsmonitor_daemon(), I think aiming for shorter filenames & function names helps, e.g. these long names widen diffstats, and many people who hack on the code stick religiously to 80 character width terminals.
I prefer self-documenting code.
Jeff
Re: [PATCH v3 19/34] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
Quoted 24 lines · Jeff Hostetler
Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.
Quoted 9 lines
Quoted 3 lines
Signed-off-by Jeff Hostetler <[email protected]>--- compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++<bikeshed mode> Spying on the early history of this (looking for the Linux backend) I saw that at some point we had just
compat/fsmonitor/linux.c, and presumably some ofcompat/fsmonitor/{windows,win32,macos,darwin}.c.
At some point those filenames became much much longer.Once upon a time having "
foo/bar/win32.c" and "abc/def/win32.c" would cause confusion in the debugger (I've long since forgotten which). Breaking at win32.c:30 was no longer unique.
Quoted 6 lines
Also, if the
Makefilesends all .o's to the root directory or a unified OBJS directory rather than to the subdir containing the .c, then we have another issue during linking...So, having been burned too many times, I prefer to make source filenames unique when possible.
A much shorter name like compat/fsmonitor/fsmon-win32.c would achieve that goal.
Quoted 9 lines
Quoted 7 lines
I've noticed you tend to prefer really long file and function names, e.g. your borrowed
daemonize()becamespawn_background_fsmonitor_daemon(), I think aiming for shorter filenames & function names helps, e.g. these long names widen diffstats, and many people who hack on the code stick religiously to 80 character width terminals.I prefer self-documenting code.
Sure, I'm not saying daemonize() is an ideal name, just suggesting that you can both get uniqueness & self-documentation and not need to split to multiple lines in some common cases to stay within the "We try to keep to at most 80 characters per line" in CodingGuidelines in this series.
Re: [PATCH v3 19/34] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
Hi Ævar,
Quoted 45 lines · Ævar Arnfjörð Bjarmason
Quoted 24 lines · Jeff Hostetler
Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.
Quoted 9 lines
Quoted 3 lines
Signed-off-by Jeff Hostetler <[email protected]>--- compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++<bikeshed mode> Spying on the early history of this (looking for the Linux backend) I saw that at some point we had just
compat/fsmonitor/linux.c, and presumably some ofcompat/fsmonitor/{windows,win32,macos,darwin}.c.
At some point those filenames became much much longer.Once upon a time having "
foo/bar/win32.c" and "abc/def/win32.c" would cause confusion in the debugger (I've long since forgotten which). Breaking at win32.c:30 was no longer unique.Quoted 6 lines
Also, if the
Makefilesends all .o's to the root directory or a unified OBJS directory rather than to the subdir containing the .c, then we have another issue during linking...So, having been burned too many times, I prefer to make source filenames unique when possible.
A much shorter name like
compat/fsmonitor/fsmon-win32.cwould achieve that goal.Quoted 8 lines
Quoted 7 lines
I've noticed you tend to prefer really long file and function names, e.g. your borrowed
daemonize()becamespawn_background_fsmonitor_daemon(), I think aiming for shorter filenames & function names helps, e.g. these long names widen diffstats, and many people who hack on the code stick religiously to 80 character width terminals.
Quoted 7 lines
Quoted 1 line
I prefer self-documenting code.
Sure, I'm not saying
daemonize()is an ideal name, just suggesting that you can both get uniqueness & self-documentation and not need to split to multiple lines in some common cases to stay within the "We try to keep to at most 80 characters per line" in CodingGuidelines in this series.
While you are entitled to have your taste, I have to point out that Jeff is just as entitled to their taste, and I don't think that you can claim that yours is better.
So I wonder what the intended outcome of this review is? To make the patch better? Or to pit taste against taste?
Ciao,
Johannes
Re: [PATCH v3 19/34] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
Quoted 57 lines · Johannes Schindelin
Hi Ævar,
Quoted 45 lines · Ævar Arnfjörð Bjarmason
Quoted 24 lines · Jeff Hostetler
Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.
Quoted 9 lines
Quoted 3 lines
Signed-off-by Jeff Hostetler <[email protected]>--- compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++<bikeshed mode> Spying on the early history of this (looking for the Linux backend) I saw that at some point we had just
compat/fsmonitor/linux.c, and presumably some ofcompat/fsmonitor/{windows,win32,macos,darwin}.c.
At some point those filenames became much much longer.Once upon a time having "
foo/bar/win32.c" and "abc/def/win32.c" would cause confusion in the debugger (I've long since forgotten which). Breaking at win32.c:30 was no longer unique.Quoted 6 lines
Also, if the
Makefilesends all .o's to the root directory or a unified OBJS directory rather than to the subdir containing the .c, then we have another issue during linking...So, having been burned too many times, I prefer to make source filenames unique when possible.
A much shorter name like
compat/fsmonitor/fsmon-win32.cwould achieve that goal.Quoted 8 lines
Quoted 7 lines
I've noticed you tend to prefer really long file and function names, e.g. your borrowed
daemonize()becamespawn_background_fsmonitor_daemon(), I think aiming for shorter filenames & function names helps, e.g. these long names widen diffstats, and many people who hack on the code stick religiously to 80 character width terminals.Quoted 7 lines
Quoted 1 line
I prefer self-documenting code.
Sure, I'm not saying
daemonize()is an ideal name, just suggesting that you can both get uniqueness & self-documentation and not need to split to multiple lines in some common cases to stay within the "We try to keep to at most 80 characters per line" in CodingGuidelines in this series.
Quoted 6 lines
While you are entitled to have your taste, I have to point out that Jeff is just as entitled to their taste, and I don't think that you can claim that yours is better.
So I wonder what the intended outcome of this review is? To make the patch better? Or to pit taste against taste?
Neither, to address a misunderstanding.
Sure, if a reviewer points out "maybe change X to Y" and the reply is "I like X better than Y", fair enough.
My reading of Jeff H.'s upthread was that he'd misunderstood my suggesting of that Y for a Z.
I.e. that shortening a name like fsmonitor-fs-listen-win32.c (X) necessarily had to mean that we'd have a win32.c (Z), negatively impacting some debugging workflows, as opposed to just a shorter-but-unique name like fsmon-win32.c (Y).
Ditto for daemonize() (X/Z) and spawn_background_fsmonitor_daemon() (X).
I'm certain that with this reply we're thoroughly into the "respectfully disagree" territory as opposed to having a misunderstanding.
I also take and agree your implied point that there's no point in having a yes/no/yes/no/yes argument on-list, and I did not mean to engage in such a thing, only to clear up the misunderstanding, if any.
I'll only say that I don't think that something like long variable/file etc. names is *just* a matter of taste, seeing as we have a fairly strict "keep to at most 80 characters per line" as the 2nd item in the C coding style (after "use tabs, not spaces").
That matter of taste for one developer objectively makes it harder to stay within the bounds of the coding style for furute maintenance.
We do have active contributors that I understand actually use terminals of that size to work on this project (CC'd, but maybe I misrecall that for one/both). I'm not one of those people, but I do find that maintaining code with needlessly long identifiers in this codebase is painful.
E.g. in a patch I just submitted I've been working on similarly long identifiers in the refs code[1], and with say a long variable/type name and using a long-named function you get to the point of needing to place each individual argument of the function on its own line, or near enough to that.
Re: [PATCH v3 19/34] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
Quoted 42 lines · Ævar Arnfjörð Bjarmason
Quoted 41 lines · Johannes Schindelin
Hi Ævar,
Quoted 37 lines · Ævar Arnfjörð Bjarmason
Quoted 24 lines · Jeff Hostetler
Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.
Quoted 9 lines
Quoted 3 lines
Signed-off-by Jeff Hostetler <[email protected]>--- compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++<bikeshed mode> Spying on the early history of this (looking for the Linux backend) I saw that at some point we had just
compat/fsmonitor/linux.c, and presumably some ofcompat/fsmonitor/{windows,win32,macos,darwin}.c.
At some point those filenames became much much longer.Once upon a time having "
foo/bar/win32.c" and "abc/def/win32.c" would cause confusion in the debugger (I've long since forgotten which). Breaking at win32.c:30 was no longer unique.Quoted 6 lines
Also, if the
Makefilesends all .o's to the root directory or a unified OBJS directory rather than to the subdir containing the .c, then we have another issue during linking...So, having been burned too many times, I prefer to make source filenames unique when possible.
A much shorter name like
compat/fsmonitor/fsmon-win32.cwould achieve that goal.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
I've noticed you tend to prefer really long file and function names, e.g. your borrowed
daemonize()becamespawn_background_fsmonitor_daemon(), I think aiming for shorter filenames & function names helps, e.g. these long names widen diffstats, and many people who hack on the code stick religiously to 80 character width terminals.
At least "daemon"/"daemonize" already implies "background"; so even if we have the extra function, "spawn_fsmon_daemon()" would be enough info.
Quoted 22 lines
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Quoted 1 line
I prefer self-documenting code.
Sure, I'm not saying
daemonize()is an ideal name, just suggesting that you can both get uniqueness & self-documentation and not need to split to multiple lines in some common cases to stay within the "We try to keep to at most 80 characters per line" in CodingGuidelines in this series.Quoted 6 lines
While you are entitled to have your taste, I have to point out that Jeff is just as entitled to their taste, and I don't think that you can claim that yours is better.
So I wonder what the intended outcome of this review is? To make the patch better? Or to pit taste against taste?
Neither, to address a misunderstanding.
Sure, if a reviewer points out "maybe change X to Y" and the reply is "I like X better than Y", fair enough.
Quoted 7 lines
My reading of Jeff H.'s upthread was that he'd misunderstood my suggesting of that Y for a Z.
I.e. that shortening a name like
fsmonitor-fs-listen-win32.c(X) necessarily had to mean that we'd have awin32.c(Z), negatively impacting some debugging workflows, as opposed to just a shorter-but-unique name likefsmon-win32.c(Y).
Short-as-possible-while-being-meaningful is a pretty important usability thing git. There's a good reason git supports OID prefix abbreviations, after all.
Not my area of expertise, but AFAIK git's rename detection is affected by basename; and I've encountered debugger confusion with non-unique basenames while debugging other codebases.
My brain works like a naive "strcmp"/"memcmp": long common prefixes slows down my ability to differentiate filenames.
Having lots of common terms/prefixes on the screen works like camoflage to me and slows down my ability to process things. I suppose my eyes and cognitive abilities are below average; and even worse due to the pandemic numbing my brain.
Quoted 1 line
Ditto for
daemonize()(X/Z) andspawn_background_fsmonitor_daemon()(X).
(what I said above)
Quoted 12 lines
I'm certain that with this reply we're thoroughly into the "respectfully disagree" territory as opposed to having a misunderstanding.
I also take and agree your implied point that there's no point in having a yes/no/yes/no/yes argument on-list, and I did not mean to engage in such a thing, only to clear up the misunderstanding, if any.
I'll only say that I don't think that something like long variable/file etc. names is *just* a matter of taste, seeing as we have a fairly strict "keep to at most 80 characters per line" as the 2nd item in the C coding style (after "use tabs, not spaces").
Quoted 8 lines
That matter of taste for one developer objectively makes it harder to stay within the bounds of the coding style for furute maintenance.
We do have active contributors that I understand actually use terminals of that size to work on this project (CC'd, but maybe I misrecall that for one/both). I'm not one of those people, but I do find that maintaining code with needlessly long identifiers in this codebase is painful.
Thanks for Cc-ing me.
Yes, I'm one of those developers. Accessibility matters to me:
my eyesight certainly isn't getting better with age (nor do I expect anyone elses'). I need giant fonts to reduce eye and neck strain.
Fwiw, newspaper publishers figured out line width decades/centuries ago and wrap lines despite having large sheets to work on.
I mostly work over mosh or ssh to reduce noise and heat locally. There's no bandwidth for VNC or similar, and graphical stuff tends to be unstable UI-wise anyways so I stick to the terminal.
Taste does have much to do with it: I favor stable, reliable tools (e.g. POSIX, Perl5, git) that works well on both old and new hardware. I avoid mainstream "desktop" software since they tend to have unstable UIs which break users' workflows while requiring more powerful HW.
Complex graphics drivers tend to get unreliable, too, especially when one is stuck with old HW that gets limited support from vendors. It's also difficult to fix complex drivers as a hobbyist given the one-off HW/vendor-specific knowledge required.
So we shouldn't expect a developer with old HW can have more than a standard text terminal. This is an accessibility problem for developers lacking in finances.
This is also a problem for developers wishing to backdoors+bugs found in modern systems (IntelME, AMD-PSP, endless stream of CPU bugs).
Back to health-related accessibility; I've also had joint problems for many years so shorter identifiers helps reduce typing I need to do. I mostly had that under control pre-pandemic, but it's been a huge struggle to find adequate replacements for activities I used to rely on to manage the pain.
Re: [PATCH v3 19/34] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
Quoted 7 lines · Eric Wong
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Quoted 5 lines · Johannes Schindelin
Hi Ævar,
Quoted 1 line · Ævar Arnfjörð Bjarmason
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines · Jeff Hostetler
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Quoted 4 lines · Jeff Hostetler via GitGitGadget
Quoted 3 lines
From: Jeff Hostetler <[email protected]>
...
Eric, welcome to the conversation and thanks for sharing your concerns.
For my upcoming V4 I've shortened the filenames of the various backends, renamed the -macos one to -darwin, and shortened the names of the fsm-listener API, and the names of those static functions associated with starting the daemon in the background.
I think this covers all of the issues raised across several patches in the series.
Jeff
Re: [PATCH v3 19/34] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
In reply to Johannes SchindelinQuoted 11 lines · Johannes Schindelin
Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 1 line · Jeff Hostetler
I prefer self-documenting code.
Sure, I'm not saying
daemonize()is an ideal name, just suggesting that you can both get uniqueness & self-documentation and not need to split to multiple lines in some common cases to stay within the "We try to keep to at most 80 characters per line" in CodingGuidelines in this series.
Quoted 6 lines
While you are entitled to have your taste, I have to point out that Jeff is just as entitled to their taste, and I don't think that you can claim that yours is better.
So I wonder what the intended outcome of this review is? To make the patch better? Or to pit taste against taste?
Unless you read minds you can't possibly know what the taste of other people will be.
So you put forward what *you* think is better, and then find out if others agree with your taste or not. If it turns out you are the only one that thinks it's better, so be it.
For what it's worth, I agree with Ævar that daemonize() is better and I find your statement "I prefer self-documenting code" a) not an argument,
b) not a valid argument if we fill in the dots, and c) passively aggressive.
Each one of us can only do one thing: express our opinion. What else can we do?
Reviewers should not be chastised for expressing their opinion.
Signature · 1 line
Felipe Contreras
Re: [PATCH v3 19/34] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
In reply to Jeff Hostetler via GitGitGadgetHi Jeff,
On Thu, 1 Jul 2021, Jeff Hostetler via GitGitGadget wrote:
Jeff Hostetler <[email protected]>
the win32 backend to register a watch on the working tree irectory (recursively). Also watch the <gitdir> if it is side the working tree. And to collect path change notifications atches and publish.
-off-by: Jeff Hostetler <[email protected]>
t/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++ e changed, 530 insertions(+)
Quoted 10 lines
compat/fsmonitor/fsmonitor-fs-listen-win32.c +5
-0@@ -2,20 +2,550 @@ + [...] + +static struct one_watch *create_watch (struct fsmonitor_daemon_state *state, +const char *path) +{
compat/fsmonitor/fsmonitor-fs-listen-win32.c line 16
Quoted 10 lines
compat/fsmonitor/fsmonitor-fs-listen-win32.c +10
-0+struct one_watch *watch =NULL ; +DWORD desired_access =FILE_LIST_DIRECTORY ; +DWORD share_mode = +FILE_SHARE_WRITE |FILE_SHARE_READ |FILE_SHARE_DELETE ; +HANDLE hDir; + + hDir =CreateFileA (path, + desired_access, share_mode,NULL ,OPEN_EXISTING , +FILE_FLAG_BACKUP_SEMANTICS |FILE_FLAG_OVERLAPPED , +NULL );
The *A() family of Win32 API functions disagree with Git in one very interesting aspect: Git always assumes UTF-8, while e.g. CreateFileA() will use the current Win32 locale to internally transform to wide characters and then call CreateFileW().
This poses no problem when your locale is US American and your paths contain no non-ASCII characters.
In the Git for Windows bug tracker, it was reported that it _does_ cause problems when venturing outside such a cozy scenario (for full details, see https://github.com/git-for-windows/git/issues/3262)
I need this (and merged it before starting the process to release Git for Windows v2.32.0(2)) to fix that (could I ask you to integrate this in case a re-roll will become necessary?):
-- snipsnap --
From: Johannes Schindelin <[email protected]>
Date: Mon, 5 Jul 2021 13:51:05 +0200
Subject: [PATCH] fixup! fsmonitor-fs-listen-win32: implement FSMonitor backend
on Windows
Let's keep avoiding the *A() family of Win32 API functions because they are susceptible to incoherent encoding problems. In Git for Windows, we always assume paths to be UTF-8 encoded. Let's use the dedicated helper to convert such a path to the wide character version, and then use the *W() function instead.
--- compat/fsmonitor/fsmonitor-fs-listen-win32.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
compat/fsmonitor/fsmonitor-fs-listen-win32.c +7 -1
@@ -111,8 +111,14 @@ static struct one_watch *create_watch(struct fsmonitor_daemon_state *state,DWORD share_mode =FILE_SHARE_WRITE |FILE_SHARE_READ |FILE_SHARE_DELETE ;HANDLE hDir; +wchar_t wpath[MAX_PATH ]; - hDir =CreateFileA (path, +if (xutftowcs_path (wpath, path) <0 ) { +error (_ ("could not convert to wide characters: '%s'" ), path); +return NULL ; + } + + hDir =CreateFileW (wpath, desired_access, share_mode,NULL ,OPEN_EXISTING ,FILE_FLAG_BACKUP_SEMANTICS |FILE_FLAG_OVERLAPPED ,NULL );
--
2.32.0.windows.1.15.gf1590a75e2d
Re: [PATCH v3 19/34] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows
Quoted 92 lines · Johannes Schindelin
Hi Jeff,
On Thu, 1 Jul 2021, Jeff Hostetler via GitGitGadget wrote:
Jeff Hostetler <[email protected]>
the win32 backend to register a watch on the working tree irectory (recursively). Also watch the <gitdir> if it is side the working tree. And to collect path change notifications atches and publish.
-off-by: Jeff Hostetler <[email protected]>
t/fsmonitor/fsmonitor-fs-listen-win32.c| 530 +++++++++++++++++++ e changed, 530 insertions(+)Quoted 10 lines
compat/fsmonitor/fsmonitor-fs-listen-win32.c +5
-0@@ -2,20 +2,550 @@ + [...] + +static struct one_watch *create_watch (struct fsmonitor_daemon_state *state, +const char *path) +{compat/fsmonitor/fsmonitor-fs-listen-win32.c line 16
Quoted 10 lines
compat/fsmonitor/fsmonitor-fs-listen-win32.c +10
-0+struct one_watch *watch =NULL ; +DWORD desired_access =FILE_LIST_DIRECTORY ; +DWORD share_mode = +FILE_SHARE_WRITE |FILE_SHARE_READ |FILE_SHARE_DELETE ; +HANDLE hDir; + + hDir =CreateFileA (path, + desired_access, share_mode,NULL ,OPEN_EXISTING , +FILE_FLAG_BACKUP_SEMANTICS |FILE_FLAG_OVERLAPPED , +NULL );The
*A()family of Win32 API functions disagree with Git in one very interesting aspect: Git always assumes UTF-8, while e.g.CreateFileA()will use the current Win32 locale to internally transform to wide characters and then callCreateFileW().This poses no problem when your locale is US American and your paths contain no non-ASCII characters.
In the Git for Windows bug tracker, it was reported that it _does_ cause problems when venturing outside such a cozy scenario (for full details, see https://github.com/git-for-windows/git/issues/3262)
I need this (and merged it before starting the process to release Git for Windows v2.32.0(2)) to fix that (could I ask you to integrate this in case a re-roll will become necessary?):
-- snipsnap --
From: Johannes Schindelin <[email protected]>
Date: Mon, 5 Jul 2021 13:51:05 +0200
Subject: [PATCH] fixup! fsmonitor-fs-listen-win32: implement FSMonitor backend
on WindowsLet's keep avoiding the
*A()family of Win32 API functions because they are susceptible to incoherent encoding problems. In Git for Windows, we always assume paths to be UTF-8 encoded. Let's use the dedicated helper to convert such a path to the wide character version, and then use the*W()function instead.Signed-off-by Johannes Schindelin <[email protected]>--- compat/fsmonitor/fsmonitor-fs-listen-win32.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)compat/fsmonitor/fsmonitor-fs-listen-win32.c +7
-2@@ -111,8 +111,14 @@ static struct one_watch *create_watch(struct fsmonitor_daemon_state *state,DWORD share_mode =FILE_SHARE_WRITE |FILE_SHARE_READ |FILE_SHARE_DELETE ;HANDLE hDir; +wchar_t wpath[MAX_PATH ]; - hDir =CreateFileA (path, +if (xutftowcs_path (wpath, path) <0 ) { +error (_ ("could not convert to wide characters: '%s'" ), path); +return NULL ; + } + + hDir =CreateFileW (wpath, desired_access, share_mode,NULL ,OPEN_EXISTING ,FILE_FLAG_BACKUP_SEMANTICS |FILE_FLAG_OVERLAPPED ,NULL ); --
Quoted 2 lines
2.32.0.windows.1.15.gf1590a75e2d
Thanks for the heads up. I'll pull this into my next release. Jeff
PATCH v3 20/34 fsmonitor-fs-listen-macos: add macos header files for FSEvent
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Include MacOS system declarations to allow us to use FSEvent and CoreFoundation APIs. We need GCC and clang versions because of compiler and header file conflicts.
While it is quite possible to #include Apple's CoreServices.h when compiling C source code with clang, trying to build it with GCC currently fails with this error:
In file included
from /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/Security.framework/Headers/AuthSession.h:32,
from /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/Security.framework/Headers/Security.h:42,
from /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/OSServices.framework/Headers/CSIdentity.h:43, from /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/OSServices.framework/Headers/OSServices.h:29, from /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Headers/IconsCore.h:23, from /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Headers/LaunchServices.h:23, from /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/CoreServices.framework/Headers/CoreServices.h:45,
/Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/Security.framework/Headers/Authorization.h:193:7: error: variably modified 'bytes' at file scope
193 | char bytes[kAuthorizationExternalFormLength];
| ^~~~~The underlying reason is that GCC (rightfully) objects that an enum value such as kAuthorizationExternalFormLength is not a constant (because it is not, the preprocessor has no knowledge of it, only the actual C compiler does) and can therefore not be used to define the size of a C array.
This is a known problem and tracked in GCC's bug tracker:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93082
In the meantime, let's not block things and go the slightly ugly route of declaring/defining the FSEvents constants, data structures and functions that we need, so that we can avoid above-mentioned issue.
Let's do this _only_ for GCC, though, so that the CI/PR builds (which build both with clang and with GCC) can guarantee that we _are_ using the correct data types.
--- compat/fsmonitor/fsmonitor-fs-listen-macos.c | 96 ++++++++++++++++++++ 1 file changed, 96 insertions(+)
compat/fsmonitor/fsmonitor-fs-listen-macos.c +96 -0
@@ -1,3 +1,99 @@ +#if defined(__GNUC__) +/* +* It is possible to #include CoreFoundation/CoreFoundation.h when compiling +* with clang, but not with GCC as of time of writing. +* +* See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93082 for details. +*/ +typedef unsigned int FSEventStreamCreateFlags ; +#define kFSEventStreamEventFlagNone 0x00000000 +#define kFSEventStreamEventFlagMustScanSubDirs 0x00000001 +#define kFSEventStreamEventFlagUserDropped 0x00000002 +#define kFSEventStreamEventFlagKernelDropped 0x00000004 +#define kFSEventStreamEventFlagEventIdsWrapped 0x00000008 +#define kFSEventStreamEventFlagHistoryDone 0x00000010 +#define kFSEventStreamEventFlagRootChanged 0x00000020 +#define kFSEventStreamEventFlagMount 0x00000040 +#define kFSEventStreamEventFlagUnmount 0x00000080 +#define kFSEventStreamEventFlagItemCreated 0x00000100 +#define kFSEventStreamEventFlagItemRemoved 0x00000200 +#define kFSEventStreamEventFlagItemInodeMetaMod 0x00000400 +#define kFSEventStreamEventFlagItemRenamed 0x00000800 +#define kFSEventStreamEventFlagItemModified 0x00001000 +#define kFSEventStreamEventFlagItemFinderInfoMod 0x00002000 +#define kFSEventStreamEventFlagItemChangeOwner 0x00004000 +#define kFSEventStreamEventFlagItemXattrMod 0x00008000 +#define kFSEventStreamEventFlagItemIsFile 0x00010000 +#define kFSEventStreamEventFlagItemIsDir 0x00020000 +#define kFSEventStreamEventFlagItemIsSymlink 0x00040000 +#define kFSEventStreamEventFlagOwnEvent 0x00080000 +#define kFSEventStreamEventFlagItemIsHardlink 0x00100000 +#define kFSEventStreamEventFlagItemIsLastHardlink 0x00200000 +#define kFSEventStreamEventFlagItemCloned 0x00400000 + +typedef struct __FSEventStream *FSEventStreamRef ; +typedef const FSEventStreamRef ConstFSEventStreamRef ; + +typedef unsigned int CFStringEncoding ; +#define kCFStringEncodingUTF8 0x08000100 + +typedef const struct __CFString *CFStringRef ; +typedef const struct __CFArray *CFArrayRef ; +typedef const struct __CFRunLoop *CFRunLoopRef ; + +struct FSEventStreamContext { +long long version; +void *cb_data, *retain, *release, *copy_description; +}; + +typedef struct FSEventStreamContext FSEventStreamContext ; +typedef unsigned int FSEventStreamEventFlags ; +#define kFSEventStreamCreateFlagNoDefer 0x02 +#define kFSEventStreamCreateFlagWatchRoot 0x04 +#define kFSEventStreamCreateFlagFileEvents 0x10 + +typedef unsigned long long FSEventStreamEventId ; +#define kFSEventStreamEventIdSinceNow 0xFFFFFFFFFFFFFFFFULL + +typedef void (*FSEventStreamCallback )(ConstFSEventStreamRef streamRef, +void *context, +__SIZE_TYPE__ num_of_events, +void *event_paths, +const FSEventStreamEventFlags event_flags[], +const FSEventStreamEventId event_ids[]); +typedef double CFTimeInterval ; +FSEventStreamRef FSEventStreamCreate (void *allocator, +FSEventStreamCallback callback, +FSEventStreamContext *context, +CFArrayRef paths_to_watch, +FSEventStreamEventId since_when, +CFTimeInterval latency, +FSEventStreamCreateFlags flags); +CFStringRef CFStringCreateWithCString (void *allocator,const char *string, +CFStringEncoding encoding); +CFArrayRef CFArrayCreate (void *allocator,const void **items,long long count, +void *callbacks); +void CFRunLoopRun (void ); +void CFRunLoopStop (CFRunLoopRef run_loop); +CFRunLoopRef CFRunLoopGetCurrent (void ); +extern CFStringRef kCFRunLoopDefaultMode; +void FSEventStreamScheduleWithRunLoop (FSEventStreamRef stream, +CFRunLoopRef run_loop, +CFStringRef run_loop_mode); +unsigned char FSEventStreamStart (FSEventStreamRef stream); +void FSEventStreamStop (FSEventStreamRef stream); +void FSEventStreamInvalidate (FSEventStreamRef stream); +void FSEventStreamRelease (FSEventStreamRef stream); +#else +/* +* Let Apple's headers declare `isalnum()` first, before +* Git's headers override it via a constant +*/ +#include <string.h> +#include <CoreFoundation/CoreFoundation.h> +#include <CoreServices/CoreServices.h> +#endif +#include "cache.h" #include "fsmonitor.h" #include "fsmonitor-fs-listen.h"
Signature · 1 line
gitgitgadget
PATCH v3 21/34 fsmonitor-fs-listen-macos: implement FSEvent listener on MacOS
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Implement file system event listener on MacOS using FSEvent, CoreFoundation, and CoreServices.
--- compat/fsmonitor/fsmonitor-fs-listen-macos.c | 381 +++++++++++++++++++ 1 file changed, 381 insertions(+)
compat/fsmonitor/fsmonitor-fs-listen-macos.c +381 -0
@@ -97,20 +97,401 @@ void FSEventStreamRelease(FSEventStreamRef stream);#include "cache.h" #include "fsmonitor.h" #include "fsmonitor-fs-listen.h" +#include "fsmonitor--daemon.h" + +struct fsmonitor_daemon_backend_data +{ +CFStringRef cfsr_worktree_path; +CFStringRef cfsr_gitdir_path; + +CFArrayRef cfar_paths_to_watch; +int nr_paths_watching; + +FSEventStreamRef stream; + +CFRunLoopRef rl; + +enum shutdown_style { +SHUTDOWN_EVENT =0 , +FORCE_SHUTDOWN , +FORCE_ERROR_STOP , + } shutdown_style; + +unsigned int stream_scheduled:1 ; +unsigned int stream_started:1 ; +}; + +static void log_flags_set (const char *path,const FSEventStreamEventFlags flag) +{ +struct strbuf msg =STRBUF_INIT ; + +if (flag & kFSEventStreamEventFlagMustScanSubDirs) +strbuf_addstr (&msg,"MustScanSubDirs|" ); +if (flag & kFSEventStreamEventFlagUserDropped) +strbuf_addstr (&msg,"UserDropped|" ); +if (flag & kFSEventStreamEventFlagKernelDropped) +strbuf_addstr (&msg,"KernelDropped|" ); +if (flag & kFSEventStreamEventFlagEventIdsWrapped) +strbuf_addstr (&msg,"EventIdsWrapped|" ); +if (flag & kFSEventStreamEventFlagHistoryDone) +strbuf_addstr (&msg,"HistoryDone|" ); +if (flag & kFSEventStreamEventFlagRootChanged) +strbuf_addstr (&msg,"RootChanged|" ); +if (flag & kFSEventStreamEventFlagMount) +strbuf_addstr (&msg,"Mount|" ); +if (flag & kFSEventStreamEventFlagUnmount) +strbuf_addstr (&msg,"Unmount|" ); +if (flag & kFSEventStreamEventFlagItemChangeOwner) +strbuf_addstr (&msg,"ItemChangeOwner|" ); +if (flag & kFSEventStreamEventFlagItemCreated) +strbuf_addstr (&msg,"ItemCreated|" ); +if (flag & kFSEventStreamEventFlagItemFinderInfoMod) +strbuf_addstr (&msg,"ItemFinderInfoMod|" ); +if (flag & kFSEventStreamEventFlagItemInodeMetaMod) +strbuf_addstr (&msg,"ItemInodeMetaMod|" ); +if (flag & kFSEventStreamEventFlagItemIsDir) +strbuf_addstr (&msg,"ItemIsDir|" ); +if (flag & kFSEventStreamEventFlagItemIsFile) +strbuf_addstr (&msg,"ItemIsFile|" ); +if (flag & kFSEventStreamEventFlagItemIsHardlink) +strbuf_addstr (&msg,"ItemIsHardlink|" ); +if (flag & kFSEventStreamEventFlagItemIsLastHardlink) +strbuf_addstr (&msg,"ItemIsLastHardlink|" ); +if (flag & kFSEventStreamEventFlagItemIsSymlink) +strbuf_addstr (&msg,"ItemIsSymlink|" ); +if (flag & kFSEventStreamEventFlagItemModified) +strbuf_addstr (&msg,"ItemModified|" ); +if (flag & kFSEventStreamEventFlagItemRemoved) +strbuf_addstr (&msg,"ItemRemoved|" ); +if (flag & kFSEventStreamEventFlagItemRenamed) +strbuf_addstr (&msg,"ItemRenamed|" ); +if (flag & kFSEventStreamEventFlagItemXattrMod) +strbuf_addstr (&msg,"ItemXattrMod|" ); +if (flag & kFSEventStreamEventFlagOwnEvent) +strbuf_addstr (&msg,"OwnEvent|" ); +if (flag & kFSEventStreamEventFlagItemCloned) +strbuf_addstr (&msg,"ItemCloned|" ); + +trace_printf_key (&trace_fsmonitor,"fsevent: '%s', flags=%u %s" , + path, flag, msg.buf); + +strbuf_release (&msg); +} + +static int ef_is_root_delete (const FSEventStreamEventFlags ef) +{ +return (ef & kFSEventStreamEventFlagItemIsDir && + ef & kFSEventStreamEventFlagItemRemoved); +} + +static int ef_is_root_renamed (const FSEventStreamEventFlags ef) +{ +return (ef & kFSEventStreamEventFlagItemIsDir && + ef & kFSEventStreamEventFlagItemRenamed); +} + +static int ef_is_dropped (const FSEventStreamEventFlags ef) +{ +return (ef & kFSEventStreamEventFlagKernelDropped || + ef & kFSEventStreamEventFlagUserDropped); +} + +static void fsevent_callback (ConstFSEventStreamRef streamRef, +void *ctx, +size_t num_of_events, +void *event_paths, +const FSEventStreamEventFlags event_flags[], +const FSEventStreamEventId event_ids[]) +{ +struct fsmonitor_daemon_state *state = ctx; +struct fsmonitor_daemon_backend_data *data = state->backend_data; +char **paths = (char **)event_paths; +struct fsmonitor_batch *batch =NULL ; +struct string_list cookie_list =STRING_LIST_INIT_DUP ; +const char *path_k; +const char *slash; +int k; +struct strbuf tmp =STRBUF_INIT ; + +/* +* Build a list of all filesystem changes into a private/local +* list and without holding any locks. +*/ +for (k =0 ; k < num_of_events; k++) { +/* +* On Mac, we receive an array of absolute paths. +*/ + path_k = paths[k]; + +/* +* If you want to debug FSEvents, log them to GIT_TRACE_FSMONITOR. +* Please don't log them to Trace2. +* +* trace_printf_key(&trace_fsmonitor, "Path: '%s'", path_k); +*/ + +/* +* If event[k] is marked as dropped, we assume that we have +* lost sync with the filesystem and should flush our cached +* data. We need to: +* +* [1] Abort/wake any client threads waiting for a cookie and +* flush the cached state data (the current token), and +* create a new token. +* +* [2] Discard the batch that we were locally building (since +* they are conceptually relative to the just flushed +* token). +*/ +if (ef_is_dropped (event_flags[k])) { +/* +* see also kFSEventStreamEventFlagMustScanSubDirs +*/ +trace_printf_key (&trace_fsmonitor,"event: dropped" ); + +fsmonitor_force_resync (state); +fsmonitor_batch__pop (batch); +string_list_clear (&cookie_list,0 ); + +/* +* We assume that any events that we received +* in this callback after this dropped event +* may still be valid, so we continue rather +* than break. (And just in case there is a +* delete of ".git" hiding in there.) +*/ +continue ; + } + +switch (fsmonitor_classify_path_absolute (state, path_k)) { + +case IS_INSIDE_DOT_GIT_WITH_COOKIE_PREFIX : +case IS_INSIDE_GITDIR_WITH_COOKIE_PREFIX : +/* special case cookie files within .git or gitdir */ + +/* Use just the filename of the cookie file. */ + slash =find_last_dir_sep (path_k); +string_list_append (&cookie_list, + slash ? slash +1 : path_k); +break ; + +case IS_INSIDE_DOT_GIT : +case IS_INSIDE_GITDIR : +/* ignore all other paths inside of .git or gitdir */ +break ; + +case IS_DOT_GIT : +case IS_GITDIR : +/* +* If .git directory is deleted or renamed away, +* we have to quit. +*/ +if (ef_is_root_delete (event_flags[k])) { +trace_printf_key (&trace_fsmonitor, +"event: gitdir removed" ); + goto force_shutdown; + } +if (ef_is_root_renamed (event_flags[k])) { +trace_printf_key (&trace_fsmonitor, +"event: gitdir renamed" ); + goto force_shutdown; + } +break ; + +case IS_WORKDIR_PATH : +/* try to queue normal pathnames */ + +if (trace_pass_fl (&trace_fsmonitor)) +log_flags_set (path_k, event_flags[k]); + +/* +* Because of the implicit "binning" (the +* kernel calls us at a given frequency) and +* de-duping (the kernel is free to combine +* multiple events for a given pathname), an +* individual fsevent could be marked as both +* a file and directory. Add it to the queue +* with both spellings so that the client will +* know how much to invalidate/refresh. +*/ + +if (event_flags[k] & kFSEventStreamEventFlagItemIsFile) { +const char *rel = path_k + + state->path_worktree_watch.len +1 ; + +if (!batch) + batch =fsmonitor_batch__new (); +fsmonitor_batch__add_path (batch, rel); + } + +if (event_flags[k] & kFSEventStreamEventFlagItemIsDir) { +const char *rel = path_k + + state->path_worktree_watch.len +1 ; + +strbuf_reset (&tmp); +strbuf_addstr (&tmp, rel); +strbuf_addch (&tmp,'/' ); + +if (!batch) + batch =fsmonitor_batch__new (); +fsmonitor_batch__add_path (batch, tmp.buf); + } + +break ; + +case IS_OUTSIDE_CONE : +default : +trace_printf_key (&trace_fsmonitor, +"ignoring '%s'" , path_k); +break ; + } + } + +fsmonitor_publish (state, batch, &cookie_list); +string_list_clear (&cookie_list,0 ); +strbuf_release (&tmp); +return ; + +force_shutdown: +fsmonitor_batch__pop (batch); +string_list_clear (&cookie_list,0 ); + + data->shutdown_style =FORCE_SHUTDOWN ; +CFRunLoopStop (data->rl); +strbuf_release (&tmp); +return ; +} + +/* +* NEEDSWORK: Investigate the proper value for the `latency` argument +* in the call to `FSEventStreamCreate()`. I'm not sure that this +* needs to be a config setting or just something that we tune after +* some testing. +* +* With a latency of 0.1, I was seeing lots of dropped events during +* the "touch 100000" files test within t/perf/p7519, but with a +* latency of 0.001 I did not see any dropped events. So the +* "correct" value may be somewhere in between. +* +* https://developer.apple.com/documentation/coreservices/1443980-fseventstreamcreate +*/ int fsmonitor_fs_listen__ctor (struct fsmonitor_daemon_state *state) { +FSEventStreamCreateFlags flags = kFSEventStreamCreateFlagNoDefer | + kFSEventStreamCreateFlagWatchRoot | + kFSEventStreamCreateFlagFileEvents; +FSEventStreamContext ctx = { +0 , + state, +NULL , +NULL , +NULL + }; +struct fsmonitor_daemon_backend_data *data; +const void *dir_array[2 ]; + +CALLOC_ARRAY (data,1 ); + state->backend_data = data; + + data->cfsr_worktree_path =CFStringCreateWithCString ( +NULL , state->path_worktree_watch.buf, kCFStringEncodingUTF8); + dir_array[data->nr_paths_watching++] = data->cfsr_worktree_path; + +if (state->nr_paths_watching >1 ) { + data->cfsr_gitdir_path =CFStringCreateWithCString ( +NULL , state->path_gitdir_watch.buf, + kCFStringEncodingUTF8); + dir_array[data->nr_paths_watching++] = data->cfsr_gitdir_path; + } + + data->cfar_paths_to_watch =CFArrayCreate (NULL , dir_array, + data->nr_paths_watching, +NULL ); + data->stream =FSEventStreamCreate (NULL , fsevent_callback, &ctx, + data->cfar_paths_to_watch, + kFSEventStreamEventIdSinceNow, +0.001 , flags); +if (data->stream ==NULL ) + goto failed; + +/* +* `data->rl` needs to be set inside the listener thread. +*/ + +return 0 ; + +failed: +error ("Unable to create FSEventStream." ); + +FREE_AND_NULL (state->backend_data);return -1 ; }void fsmonitor_fs_listen__dtor (struct fsmonitor_daemon_state *state) { +struct fsmonitor_daemon_backend_data *data; + +if (!state || !state->backend_data) +return ; + + data = state->backend_data; + +if (data->stream) { +if (data->stream_started) +FSEventStreamStop (data->stream); +if (data->stream_scheduled) +FSEventStreamInvalidate (data->stream); +FSEventStreamRelease (data->stream); + } + +FREE_AND_NULL (state->backend_data); }void fsmonitor_fs_listen__stop_async (struct fsmonitor_daemon_state *state) { +struct fsmonitor_daemon_backend_data *data; + + data = state->backend_data; + data->shutdown_style =SHUTDOWN_EVENT ; + +CFRunLoopStop (data->rl); }void fsmonitor_fs_listen__loop (struct fsmonitor_daemon_state *state) { +struct fsmonitor_daemon_backend_data *data; + + data = state->backend_data; + + data->rl =CFRunLoopGetCurrent (); + +FSEventStreamScheduleWithRunLoop (data->stream, data->rl, kCFRunLoopDefaultMode); + data->stream_scheduled =1 ; + +if (!FSEventStreamStart (data->stream)) { +error ("Failed to start the FSEventStream" ); + goto force_error_stop_without_loop; + } + data->stream_started =1 ; + +CFRunLoopRun (); + +switch (data->shutdown_style) { +case FORCE_ERROR_STOP : + state->error_code =-1 ; +/* fall thru */ +case FORCE_SHUTDOWN : +ipc_server_stop_async (state->ipc_server_data); +/* fall thru */ +case SHUTDOWN_EVENT : +default : +break ; + } +return ; + +force_error_stop_without_loop: + state->error_code =-1 ; +ipc_server_stop_async (state->ipc_server_data); +return ; }
Signature · 1 line
gitgitgadget
PATCH v3 22/34 fsmonitor--daemon: implement handle_client callback
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Teach fsmonitor--daemon to respond to IPC requests from client Git processes and respond with a list of modified pathnames relative to the provided token.
--- builtin/fsmonitor--daemon.c | 312 +++++++++++++++++++++++++++++++++++- 1 file changed, 310 insertions(+), 2 deletions(-)
builtin/fsmonitor--daemon.c +310 -2
@@ -7,6 +7,7 @@#include "fsmonitor--daemon.h" #include "simple-ipc.h" #include "khash.h" +#include "pkt-line.h" static const char *const builtin_fsmonitor__daemon_usage[] = {N_ ("git fsmonitor--daemon start [<options>]" ),
@@ -355,6 +356,311 @@ void fsmonitor_force_resync(struct fsmonitor_daemon_state *state)pthread_mutex_unlock (&state->main_lock); } +/* +* Format an opaque token string to send to the client. +*/ +static void with_lock__format_response_token ( +struct strbuf *response_token, +const struct strbuf *response_token_id, +const struct fsmonitor_batch *batch) +{ +/* assert current thread holding state->main_lock */ + +strbuf_reset (response_token); +strbuf_addf (response_token,"builtin:%s:%" PRIu64, + response_token_id->buf, batch->batch_seq_nr); +} + +/* +* Parse an opaque token from the client. +* Returns -1 on error. +*/ +static int fsmonitor_parse_client_token (const char *buf_token, +struct strbuf *requested_token_id, +uint64_t *seq_nr) +{ +const char *p; +char *p_end; + +strbuf_reset (requested_token_id); + *seq_nr =0 ; + +if (!skip_prefix (buf_token,"builtin:" , &p)) +return -1 ; + +while (*p && *p !=':' ) +strbuf_addch (requested_token_id, *p++); +if (!*p++) +return -1 ; + + *seq_nr = (uint64_t )strtoumax (p, &p_end,10 ); +if (*p_end) +return -1 ; + +return 0 ; +} + +KHASH_INIT(str, const char *, int, 0, kh_str_hash_func, kh_str_hash_equal); + +static int do_handle_client (struct fsmonitor_daemon_state *state, +const char *command, +ipc_server_reply_cb *reply, +struct ipc_server_reply_data *reply_data) +{ +struct fsmonitor_token_data *token_data =NULL ; +struct strbuf response_token =STRBUF_INIT ; +struct strbuf requested_token_id =STRBUF_INIT ; +struct strbuf payload =STRBUF_INIT ; +uint64_t requested_oldest_seq_nr =0 ; +uint64_t total_response_len =0 ; +const char *p; +const struct fsmonitor_batch *batch_head; +const struct fsmonitor_batch *batch; +intmax_t count =0 , duplicates =0 ; +kh_str_t *shown; +int hash_ret; +int do_trivial =0 ; +int do_flush =0 ; + +/* +* We expect `command` to be of the form: +* +* <command> := quit NUL +* | flush NUL +* | <V1-time-since-epoch-ns> NUL +* | <V2-opaque-fsmonitor-token> NUL +*/ + +if (!strcmp (command,"quit" )) { +/* +* A client has requested over the socket/pipe that the +* daemon shutdown. +* +* Tell the IPC thread pool to shutdown (which completes +* the await in the main thread (which can stop the +* fsmonitor listener thread)). +* +* There is no reply to the client. +*/ +return SIMPLE_IPC_QUIT ; + + }else if (!strcmp (command,"flush" )) { +/* +* Flush all of our cached data and generate a new token +* just like if we lost sync with the filesystem. +* +* Then send a trivial response using the new token. +*/ + do_flush =1 ; + do_trivial =1 ; + + }else if (!skip_prefix (command,"builtin:" , &p)) { +/* assume V1 timestamp or garbage */ + +char *p_end; + +strtoumax (command, &p_end,10 ); +trace_printf_key (&trace_fsmonitor, + ((*p_end) ? +"fsmonitor: invalid command line '%s'" : +"fsmonitor: unsupported V1 protocol '%s'" ), + command); + do_trivial =1 ; + + }else { +/* We have "builtin:*" */ +if (fsmonitor_parse_client_token (command, &requested_token_id, + &requested_oldest_seq_nr)) { +trace_printf_key (&trace_fsmonitor, +"fsmonitor: invalid V2 protocol token '%s'" , + command); + do_trivial =1 ; + + }else { +/* +* We have a V2 valid token: +* "builtin:<token_id>:<seq_nr>" +*/ + } + } + +pthread_mutex_lock (&state->main_lock); + +if (!state->current_token_data) +BUG ("fsmonitor state does not have a current token" ); + +if (do_flush) +with_lock__do_force_resync (state); + +/* +* We mark the current head of the batch list as "pinned" so +* that the listener thread will treat this item as read-only +* (and prevent any more paths from being added to it) from +* now on. +*/ + token_data = state->current_token_data; + batch_head = token_data->batch_head; + ((struct fsmonitor_batch *)batch_head)->pinned_time =time (NULL ); + +/* +* FSMonitor Protocol V2 requires that we send a response header +* with a "new current token" and then all of the paths that changed +* since the "requested token". We send the seq_nr of the just-pinned +* head batch so that future requests from a client will be relative +* to it. +*/ +with_lock__format_response_token (&response_token, + &token_data->token_id, batch_head); + +reply (reply_data, response_token.buf, response_token.len +1 ); + total_response_len += response_token.len +1 ; + +trace2_data_string ("fsmonitor" , the_repository,"response/token" , + response_token.buf); +trace_printf_key (&trace_fsmonitor,"response token: %s" , + response_token.buf); + +if (!do_trivial) { +if (strcmp (requested_token_id.buf, token_data->token_id.buf)) { +/* +* The client last spoke to a different daemon +* instance -OR- the daemon had to resync with +* the filesystem (and lost events), so reject. +*/ +trace2_data_string ("fsmonitor" , the_repository, +"response/token" ,"different" ); + do_trivial =1 ; + + }else if (requested_oldest_seq_nr < + token_data->batch_tail->batch_seq_nr) { +/* +* The client wants older events than we have for +* this token_id. This means that the end of our +* batch list was truncated and we cannot give the +* client a complete snapshot relative to their +* request. +*/ +trace_printf_key (&trace_fsmonitor, +"client requested truncated data" ); + do_trivial =1 ; + } + } + +if (do_trivial) { +pthread_mutex_unlock (&state->main_lock); + +reply (reply_data,"/" ,2 ); + +trace2_data_intmax ("fsmonitor" , the_repository, +"response/trivial" ,1 ); + +strbuf_release (&response_token); +strbuf_release (&requested_token_id); +return 0 ; + } + +/* +* We're going to hold onto a pointer to the current +* token-data while we walk the list of batches of files. +* During this time, we will NOT be under the lock. +* So we ref-count it. +* +* This allows the listener thread to continue prepending +* new batches of items to the token-data (which we'll ignore). +* +* AND it allows the listener thread to do a token-reset +* (and install a new `current_token_data`). +*/ + token_data->client_ref_count++; + +pthread_mutex_unlock (&state->main_lock); + +/* +* The client request is relative to the token that they sent, +* so walk the batch list backwards from the current head back +* to the batch (sequence number) they named. +* +* We use khash to de-dup the list of pathnames. +* +* NEEDSWORK: each batch contains a list of interned strings, +* so we only need to do pointer comparisons here to build the +* hash table. Currently, we're still comparing the string +* values. +*/ + shown =kh_init_str (); +for (batch = batch_head; + batch && batch->batch_seq_nr > requested_oldest_seq_nr; + batch = batch->next) { +size_t k; + +for (k =0 ; k < batch->nr; k++) { +const char *s = batch->interned_paths[k]; +size_t s_len; + +if (kh_get_str (shown, s) !=kh_end (shown)) + duplicates++; +else { +kh_put_str (shown, s, &hash_ret); + +trace_printf_key (&trace_fsmonitor, +"send[%" PRIuMAX"]: %s" , + count, s); + +/* Each path gets written with a trailing NUL */ + s_len =strlen (s) +1 ; + +if (payload.len + s_len >= +LARGE_PACKET_DATA_MAX ) { +reply (reply_data, payload.buf, + payload.len); + total_response_len += payload.len; +strbuf_reset (&payload); + } + +strbuf_add (&payload, s, s_len); + count++; + } + } + } + +if (payload.len) { +reply (reply_data, payload.buf, payload.len); + total_response_len += payload.len; + } + +kh_release_str (shown); + +pthread_mutex_lock (&state->main_lock); + +if (token_data->client_ref_count >0 ) + token_data->client_ref_count--; + +if (token_data->client_ref_count ==0 ) { +if (token_data != state->current_token_data) { +/* +* The listener thread did a token-reset while we were +* walking the batch list. Therefore, this token is +* stale and can be discarded completely. If we are +* the last reader thread using this token, we own +* that work. +*/ +fsmonitor_free_token_data (token_data); + } + } + +pthread_mutex_unlock (&state->main_lock); + +trace2_data_intmax ("fsmonitor" , the_repository,"response/length" , total_response_len); +trace2_data_intmax ("fsmonitor" , the_repository,"response/count/files" , count); +trace2_data_intmax ("fsmonitor" , the_repository,"response/count/duplicates" , duplicates); + +strbuf_release (&response_token); +strbuf_release (&requested_token_id); +strbuf_release (&payload); + +return 0 ; +} +static ipc_server_application_cb handle_client; static int handle_client(void *data,
@@ -362,7 +668,7 @@ static int handle_client(void *data,ipc_server_reply_cb *reply, struct ipc_server_reply_data *reply_data) { - /* struct fsmonitor_daemon_state *state = data; */ + struct fsmonitor_daemon_state *state = data;int result;/*
@@ -373,10 +679,12 @@ static int handle_client(void *data,if (command_len !=strlen (command))BUG ("FSMonitor assumes text messages" ); +trace_printf_key (&trace_fsmonitor,"requested token: %s" , command); +trace2_region_enter ("fsmonitor" ,"handle_client" , the_repository);trace2_data_string ("fsmonitor" , the_repository,"request" , command); - result =0 ;/* TODO Do something here. */ + result =do_handle_client (state, command, reply, reply_data);trace2_region_leave ("fsmonitor" ,"handle_client" , the_repository);
Signature · 1 line
gitgitgadget
PATCH v3 23/34 t/helper/test-touch: add helper to touch a series of files
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create test-tool touch that can update a series of files using either a pattern given on the command line or a list of files read from stdin.
This will be used in a later commit to speed up p7519 which needs to generate/update many thousands of files.
--- Makefile | 1 + t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/helper/test-touch.c | 126 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+) create mode 100644 t/helper/test-touch.c
Makefile +1 -0
@@ -757,6 +757,7 @@ TEST_BUILTINS_OBJS += test-string-list.o TEST_BUILTINS_OBJS += test-submodule-config.o TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o TEST_BUILTINS_OBJS += test-subprocess.o +TEST_BUILTINS_OBJS += test-touch.o TEST_BUILTINS_OBJS += test-trace2.o TEST_BUILTINS_OBJS += test-urlmatch-normalization.o TEST_BUILTINS_OBJS += test-userdiff.o
t/helper/test-tool.c +1 -0
@@ -73,6 +73,7 @@ static struct test_cmd cmds[] = { { "submodule-config", cmd__submodule_config }, { "submodule-nested-repo-config", cmd__submodule_nested_repo_config }, { "subprocess", cmd__subprocess }, + { "touch", cmd__touch }, { "trace2", cmd__trace2 }, { "userdiff", cmd__userdiff }, { "urlmatch-normalization", cmd__urlmatch_normalization },
t/helper/test-tool.h +1 -0
@@ -63,6 +63,7 @@ int cmd__string_list(int argc, const char **argv);int cmd__submodule_config (int argc,const char **argv);int cmd__submodule_nested_repo_config (int argc,const char **argv);int cmd__subprocess (int argc,const char **argv); +int cmd__touch (int argc,const char **argv);int cmd__trace2 (int argc,const char **argv);int cmd__userdiff (int argc,const char **argv);int cmd__urlmatch_normalization (int argc,const char **argv);
t/helper/test-touch.c new file +126 -0
126 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v3 23/34] t/helper/test-touch: add helper to touch a series of files
Quoted 9 lines · Jeff Hostetler via GitGitGadget
t/helper/test-touch.c new file +3
-0@@ -0,0 +1,126 @@ +/* +* test-touch.c: variation on /usr/bin/touch to speed up tests +* with a large number of files (primarily on Windows where child
t/helper/test-touch.c line 13
Quoted 10 lines
t/helper/test-touch.c new file +10
-0+* process are very, very expensive). +*/ + +#include "test-tool.h" +#include "cache.h" +#include "parse-options.h" + +char *seq_pattern; +int seq_start =1 ; +int seq_count =1 ;
With this in, "make sparse" dies like this:
SP t/helper/test-touch.ct/helper/test-touch.c:11:6: error: symbol 'seq_pattern' was not declared. Should it be static? t/helper/test-touch.c:12:5: error: symbol 'seq_start' was not declared. Should it be static? t/helper/test-touch.c:13:5: error: symbol 'seq_count' was not declared. Should it be static?
Re: [PATCH v3 23/34] t/helper/test-touch: add helper to touch a series of files
Quoted 24 lines · Junio C Hamano
Quoted 9 lines · Jeff Hostetler via GitGitGadget
t/helper/test-touch.c new file +3
-0@@ -0,0 +1,126 @@ +/* +* test-touch.c: variation on /usr/bin/touch to speed up tests +* with a large number of files (primarily on Windows where child t/helper/test-touch.c line 13
Quoted 10 lines
t/helper/test-touch.c new file +10
-0+* process are very, very expensive). +*/ + +#include "test-tool.h" +#include "cache.h" +#include "parse-options.h" + +char *seq_pattern; +int seq_start =1 ; +int seq_count =1 ;With this in, "make sparse" dies like this:
Quoted 5 lines
SP
t/helper/test-touch.ct/helper/test-touch.c:11:6: error: symbol 'seq_pattern' was not declared. Should it be static?t/helper/test-touch.c:12:5: error: symbol 'seq_start' was not declared. Should it be static?t/helper/test-touch.c:13:5: error: symbol 'seq_count' was not declared. Should it be static?
I'll fix. Thanks!
Jeff
PATCH v3 24/34 t/perf/p7519: speed up test using "test-tool touch"
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.
This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.
--- t/perf/p7519-fsmonitor.sh | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
t/perf/p7519-fsmonitor.sh +6 -8
@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" ' fi && mkdir 1_file 10_files 100_files 1000_files 10000_files && - for i in $(test_seq 1 10); do touch 10_files/$i; done && - for i in $(test_seq 1 100); do touch 100_files/$i; done && - for i in $(test_seq 1 1000); do touch 1000_files/$i; done && - for i in $(test_seq 1 10000); do touch 10000_files/$i; done && + test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 && + test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 && + test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 && + test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 && + git add 1_file 10_files 100_files 1000_files 10000_files && git commit -qm "Add files" &&
@@ -200,15 +201,12 @@ test_fsmonitor_suite() { # Update the mtimes on upto 100k files to make status think # that they are dirty. For simplicity, omit any files with # LFs (i.e. anything that ls-files thinks it needs to dquote). - # Then fully backslash-quote the paths to capture any - # whitespace so that they pass thru xargs properly. # test_perf_w_drop_caches "status (dirty) ($DESC)" ' git ls-files | \ head -100000 | \ grep -v \" | \ - sed '\''s/\(.\)/\\\1/g'\'' | \ - xargs test-tool chmtime -300 && + test-tool touch stdin && git status '
Signature · 1 line
gitgitgadget
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
Quoted 40 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.
This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.
Signed-off-by Jeff Hostetler <[email protected]>--- t/perf/p7519-fsmonitor.sh | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)t/perf/p7519-fsmonitor.sh +5
-6@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" ' fi && mkdir 1_file 10_files 100_files 1000_files 10000_files && - for i in $(test_seq 1 10); do touch 10_files/$i; done && - for i in $(test_seq 1 100); do touch 100_files/$i; done && - for i in $(test_seq 1 1000); do touch 1000_files/$i; done && - for i in $(test_seq 1 10000); do touch 10000_files/$i; done && + test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 && + test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 && + test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 && + test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 && + git add 1_file 10_files 100_files 1000_files 10000_files && git commit -qm "Add files" &&@@ -200,15 +201,12 @@ test_fsmonitor_suite() { # Update the mtimes on upto 100k files to make status think # that they are dirty. For simplicity, omit any files with # LFs (i.e. anything that ls-files thinks it needs to dquote). - # Then fully backslash-quote the paths to capture any - # whitespace so that they pass thru xargs properly.
t/perf/p7519-fsmonitor.sh test_fsmonitor_suite() { · line 211
Quoted 10 lines
t/perf/p7519-fsmonitor.sh +1
-2# test_perf_w_drop_caches "status (dirty) ($DESC)" ' git ls-files | \ head -100000 | \ grep -v \" | \ - sed '\''s/\(.\)/\\\1/g'\'' | \ - xargs test-tool chmtime -300 && + test-tool touch stdin && git status '
Did you try to replace this with some variant of:
test_seq 1 10000 | xargs touch
Which (depending on your xargs version) would invoke "touch" commands with however many argv items it thinks you can handle.
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
Quoted 56 lines · Ævar Arnfjörð Bjarmason
Quoted 40 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.
This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.
Signed-off-by Jeff Hostetler <[email protected]>--- t/perf/p7519-fsmonitor.sh | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)t/perf/p7519-fsmonitor.sh +5
-6@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" ' fi && mkdir 1_file 10_files 100_files 1000_files 10000_files && - for i in $(test_seq 1 10); do touch 10_files/$i; done && - for i in $(test_seq 1 100); do touch 100_files/$i; done && - for i in $(test_seq 1 1000); do touch 1000_files/$i; done && - for i in $(test_seq 1 10000); do touch 10000_files/$i; done && + test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 && + test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 && + test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 && + test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 && + git add 1_file 10_files 100_files 1000_files 10000_files && git commit -qm "Add files" &&@@ -200,15 +201,12 @@ test_fsmonitor_suite() { # Update the mtimes on upto 100k files to make status think # that they are dirty. For simplicity, omit any files with # LFs (i.e. anything that ls-files thinks it needs to dquote). - # Then fully backslash-quote the paths to capture any - # whitespace so that they pass thru xargs properly.t/perf/p7519-fsmonitor.sh test_fsmonitor_suite() { · line 211
Quoted 10 lines
t/perf/p7519-fsmonitor.sh +1
-2# test_perf_w_drop_caches "status (dirty) ($DESC)" ' git ls-files | \ head -100000 | \ grep -v \" | \ - sed '\''s/\(.\)/\\\1/g'\'' | \ - xargs test-tool chmtime -300 && + test-tool touch stdin && git status 'Did you try to replace this with some variant of:
Quoted 5 lines
test_seq 1 10000 | xargs touchWhich (depending on your xargs version) would invoke "touch" commands with however many argv items it thinks you can handle.
a quick test on my Windows machine shows that
test_seq 1 10000 | xargs touch
takes 3.1 seconds.
just a simple
test_seq 1 10000 >/dev/null
take 0.2 seconds.
using my test-tool helper cuts that time in half.
Jeff
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
Quoted 16 lines · Jeff Hostetler
Quoted 7 lines · Ævar Arnfjörð Bjarmason
Did you try to replace this with some variant of:
test_seq 1 10000 | xargs touchWhich (depending on your xargs version) would invoke "touch" commands with however many argv items it thinks you can handle.
a quick test on my Windows machine shows that
test_seq 1 10000 | xargs touchtakes 3.1 seconds.
Quoted 7 lines
just a simple
test_seq 1 10000 >/dev/nulltake 0.2 seconds.
using my test-tool helper cuts that time in half.
Yeah, test_seq is pretty bad; it's just a loop in shell. Is there a 'seq' on windows, and does using it instead of test_seq make things faster with Ævar's suggested command?
I'd really like to modify test_seq to use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.
Maybe it'd even make sense to write a 'test-tool seq' and make test_seq use that just so we can rip out that super lame shell looping.
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
Quoted 12 lines · Elijah Newren
Quoted 11 lines · Jeff Hostetler
a quick test on my Windows machine shows that
test_seq 1 10000 | xargs touchtakes 3.1 seconds.
just a simple
test_seq 1 10000 >/dev/null
Quoted 7 lines
Quoted 3 lines
take 0.2 seconds.
using my test-tool helper cuts that time in half.
Yeah,
test_seqis pretty bad; it's just a loop in shell. Is there a 'seq' on windows, and does using it instead oftest_seqmake things faster with Ævar's suggested command?
Unless I am misreading Jeff's message, I do not think that makes sense. Counting to 10000 in shell loop is trivial (0.2 seconds), but letting touch invoked 10000 times to create (or smudge mtime of, but I suspect that is not what is going on here) 10000 files takes 3.1 seconds, and of course a native binary that creates 10000 files with a single invocation would be faster.
Quoted 3 lines
I'd really like to modify
test_seqto use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.
So, if I am reading Jeff correctly, that optimizes something that is not a bottleneck.
Quoted 3 lines
Maybe it'd even make sense to write a 'test-tool seq' and make
test_sequse that just so we can rip out that super lame shell looping.
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
Quoted 30 lines · Junio C Hamano
Quoted 12 lines · Elijah Newren
Quoted 11 lines · Jeff Hostetler
a quick test on my Windows machine shows that
test_seq 1 10000 | xargs touchtakes 3.1 seconds.
just a simple
test_seq 1 10000 >/dev/nullQuoted 7 lines
Quoted 3 lines
take 0.2 seconds.
using my test-tool helper cuts that time in half.
Yeah,
test_seqis pretty bad; it's just a loop in shell. Is there a 'seq' on windows, and does using it instead oftest_seqmake things faster with Ævar's suggested command?Unless I am misreading Jeff's message, I do not think that makes sense. Counting to 10000 in shell loop is trivial (0.2 seconds), but letting touch invoked 10000 times to create (or smudge mtime of, but I suspect that is not what is going on here) 10000 files takes 3.1 seconds, and of course a native binary that creates 10000 files with a single invocation would be faster.
Quoted 6 lines
Quoted 3 lines
I'd really like to modify
test_seqto use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.So, if I am reading Jeff correctly, that optimizes something that is not a bottleneck.
Oh, indeed. Sorry, I misread.
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
In reply to Elijah NewrenQuoted 21 lines · Elijah Newren
Quoted 20 lines · Jeff Hostetler
Quoted 7 lines · Ævar Arnfjörð Bjarmason
Did you try to replace this with some variant of:
test_seq 1 10000 | xargs touchWhich (depending on your xargs version) would invoke "touch" commands with however many argv items it thinks you can handle.
a quick test on my Windows machine shows that
test_seq 1 10000 | xargs touchtakes 3.1 seconds.
just a simple
test_seq 1 10000 >/dev/null
Quoted 8 lines
Quoted 3 lines
take 0.2 seconds.
using my test-tool helper cuts that time in half.
Yeah,
test_seqis pretty bad; it's just a loop in shell. Is there a 'seq' on windows, and does using it instead oftest_seqmake things faster with Ævar's suggested command?
The Git for Windows SDK bash environment does have a /usr/bin/seq which appears to be from GNU coreutils 8.32. (This is different from the version that I have on my Mac (which doesn't have a version number).)
Using it:
seq 1 10000 >/dev/null
takes 0.04 seconds instead of 0.2.
However, it doesn't help the touch.
seq 1 10000 | xargs touch
still takes ~3.1 seconds.
FWIW, the xargs is clustering the 10,000 files into ~4 command lines, so there is a little bit of Windows process overhead, but not that much.
seq 1 10000 | xargs wc -l | grep total
Quoted 8 lines
I'd really like to modify
test_seqto use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.Maybe it'd even make sense to write a 'test-tool seq' and make
test_sequse that just so we can rip out that super lame shell looping.
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
Quoted 4 lines · Jeff Hostetler
FWIW, the xargs is clustering the 10,000 files into ~4 command lines, so there is a little bit of Windows process overhead, but not that much.
Quoted 9 lines
seq 1 10000 | xargs wc -l | grep total
Quoted 7 lines
I'd really like to modify
test_seqto use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.
Maybe it'd even make sense to write a 'test-tool seq' and maketest_sequse that just so we can rip out that super lame shell looping.
So what lame in this picture is not shell, or process overhead, but I/O performance.
I've seen some noises about Windows file creation performance raised as an issue when doing initial checkout followed by "git clone", and an idea floated to create a bunch of open file handles for writing in threads when checkout (really the caller that repeatedly calls entry.c:write_entry() by iterating the in-core index) starts, and write out the contents in parallel, as a workaround. When I heard it, I somehow thought it was meant as a not-so-funny joke, but from the sounds of it, the I/O performance may be so horrible to require such a hack to be usable there. Sigh...
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
Quoted 19 lines · Junio C Hamano
Quoted 4 lines · Jeff Hostetler
FWIW, the xargs is clustering the 10,000 files into ~4 command lines, so there is a little bit of Windows process overhead, but not that much.
Quoted 9 lines
seq 1 10000 | xargs wc -l | grep total
Quoted 7 lines
I'd really like to modify
test_seqto use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.
Maybe it'd even make sense to write a 'test-tool seq' and maketest_sequse that just so we can rip out that super lame shell looping.So what lame in this picture is not shell, or process overhead, but I/O performance.
Quoted 10 lines
I've seen some noises about Windows file creation performance raised as an issue when doing initial checkout followed by "git clone", and an idea floated to create a bunch of open file handles for writing in threads when checkout (really the caller that repeatedly calls entry.c:write_entry() by iterating the in-core index) starts, and write out the contents in parallel, as a workaround. When I heard it, I somehow thought it was meant as a not-so-funny joke, but from the sounds of it, the I/O performance may be so horrible to require such a hack to be usable there. Sigh...
Yes, there are some things here (that I believe to be I/O related) on Windows that I want to look at when I wrap up FSMonitor. And yes, some of them sound pretty stupid.
Jeff
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
In reply to Jeff HostetlerQuoted 64 lines · Jeff Hostetler
Quoted 51 lines · Ævar Arnfjörð Bjarmason
Quoted 38 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.
This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.
Signed-off-by Jeff Hostetler <[email protected]>--- t/perf/p7519-fsmonitor.sh | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)t/perf/p7519-fsmonitor.sh +5
-4@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" ' fi && mkdir 1_file 10_files 100_files 1000_files 10000_files && - for i in $(test_seq 1 10); do touch 10_files/$i; done && - for i in $(test_seq 1 100); do touch 100_files/$i; done && - for i in $(test_seq 1 1000); do touch 1000_files/$i; done && - for i in $(test_seq 1 10000); do touch 10000_files/$i; done && + test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 && + test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 && + test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 && + test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 && + git add 1_file 10_files 100_files 1000_files 10000_files && git commit -qm "Add files" && @@ -200,15 +201,12 @@ test_fsmonitor_suite() { # Update the mtimes on upto 100k files to make status think# that they are dirty. For simplicity, omit any files with # LFs (i.e. anything that ls-files thinks it needs to dquote).t/perf/p7519-fsmonitor.sh +0
-2- # Then fully backslash-quote the paths to capture any - # whitespace so that they pass thru xargs properly.t/perf/p7519-fsmonitor.sh test_fsmonitor_suite() { · line 211
Quoted 10 lines
t/perf/p7519-fsmonitor.sh +1
-2# test_perf_w_drop_caches "status (dirty) ($DESC)" ' git ls-files | \ head -100000 | \ grep -v \" | \ - sed '\''s/\(.\)/\\\1/g'\'' | \ - xargs test-tool chmtime -300 && + test-tool touch stdin && git status 'Did you try to replace this with some variant of:
Quoted 5 lines
test_seq 1 10000 | xargs touchWhich (depending on your xargs version) would invoke "touch" commands
with however many argv items it thinks you can handle.a quick test on my Windows machine shows that
test_seq 1 10000 | xargs touchtakes 3.1 seconds.
Quoted 7 lines
just a simple
test_seq 1 10000 >/dev/nulltake 0.2 seconds.
using my test-tool helper cuts that time in half.
There's what Elijah mentioned about test_seq, so maybe it's just that.
But what I was suggesting was using the xargs mode where it does N arguments at a time.
Does this work for you, and does it cause xargs to invoke "touch" with the relevant N number of arguments, and does it help with the performance?
test_seq 1 10000 | xargs touch
test_seq 1 10000 | xargs -n 10 touch
test_seq 1 10000 | xargs -n 100 touch
test_seq 1 10000 | xargs -n 1000 touchetc.
Also I didn't notice this before, but the -300 part of "chmtime -300" was redundant before then? I.e. you're implicitly changing it to "=+0" instead with your "touch" helper, are you not?
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
Quoted 80 lines · Ævar Arnfjörð Bjarmason
Quoted 64 lines · Jeff Hostetler
Quoted 51 lines · Ævar Arnfjörð Bjarmason
Quoted 38 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.
This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.
Signed-off-by Jeff Hostetler <[email protected]>--- t/perf/p7519-fsmonitor.sh | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)t/perf/p7519-fsmonitor.sh +5
-4@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" ' fi && mkdir 1_file 10_files 100_files 1000_files 10000_files && - for i in $(test_seq 1 10); do touch 10_files/$i; done && - for i in $(test_seq 1 100); do touch 100_files/$i; done && - for i in $(test_seq 1 1000); do touch 1000_files/$i; done && - for i in $(test_seq 1 10000); do touch 10000_files/$i; done && + test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 && + test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 && + test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 && + test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 && + git add 1_file 10_files 100_files 1000_files 10000_files && git commit -qm "Add files" && @@ -200,15 +201,12 @@ test_fsmonitor_suite() { # Update the mtimes on upto 100k files to make status think# that they are dirty. For simplicity, omit any files with # LFs (i.e. anything that ls-files thinks it needs to dquote).t/perf/p7519-fsmonitor.sh +0
-2- # Then fully backslash-quote the paths to capture any - # whitespace so that they pass thru xargs properly.t/perf/p7519-fsmonitor.sh test_fsmonitor_suite() { · line 211
Quoted 10 lines
t/perf/p7519-fsmonitor.sh +1
-2# test_perf_w_drop_caches "status (dirty) ($DESC)" ' git ls-files | \ head -100000 | \ grep -v \" | \ - sed '\''s/\(.\)/\\\1/g'\'' | \ - xargs test-tool chmtime -300 && + test-tool touch stdin && git status 'Did you try to replace this with some variant of:
Quoted 5 lines
test_seq 1 10000 | xargs touchWhich (depending on your xargs version) would invoke "touch" commands
with however many argv items it thinks you can handle.a quick test on my Windows machine shows that
test_seq 1 10000 | xargs touchtakes 3.1 seconds.
Quoted 7 lines
just a simple
test_seq 1 10000 >/dev/nulltake 0.2 seconds.
using my test-tool helper cuts that time in half.
There's what Elijah mentioned about
test_seq, so maybe it's just that.But what I was suggesting was using the xargs mode where it does N arguments at a time.
Quoted 8 lines
Does this work for you, and does it cause xargs to invoke "touch" with the relevant N number of arguments, and does it help with the performance?
test_seq 1 10000 | xargs touch test_seq 1 10000 | xargs -n 10 touch test_seq 1 10000 | xargs -n 100 touch test_seq 1 10000 | xargs -n 1000 touch
The GFW SDK version of xargs does have -n N and it does work as advertised. And it does slow down things considerably. Letting it do ~2500 per command in 4 commands took the 3.1 seconds listed above.
Add a -n 100 to it takes 5.7 seconds, so process creation overhead is a factor here.
Quoted 7 lines
etc.
Also I didn't notice this before, but the -300 part of "chmtime -300" was redundant before then? I.e. you're implicitly changing it to "=+0" instead with your "touch" helper, are you not?
Right. I'm changing it to the current time.
Jeff
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
Quoted 87 lines · Jeff Hostetler
Quoted 81 lines · Ævar Arnfjörð Bjarmason
Quoted 64 lines · Jeff Hostetler
Quoted 51 lines · Ævar Arnfjörð Bjarmason
Quoted 38 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.
This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.
Signed-off-by Jeff Hostetler <[email protected]>--- t/perf/p7519-fsmonitor.sh | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)t/perf/p7519-fsmonitor.sh +5
-4@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" ' fi && mkdir 1_file 10_files 100_files 1000_files 10000_files && - for i in $(test_seq 1 10); do touch 10_files/$i; done && - for i in $(test_seq 1 100); do touch 100_files/$i; done && - for i in $(test_seq 1 1000); do touch 1000_files/$i; done && - for i in $(test_seq 1 10000); do touch 10000_files/$i; done && + test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 && + test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 && + test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 && + test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 && + git add 1_file 10_files 100_files 1000_files 10000_files && git commit -qm "Add files" && @@ -200,15 +201,12 @@ test_fsmonitor_suite() { # Update the mtimes on upto 100k files to make status think# that they are dirty. For simplicity, omit any files with # LFs (i.e. anything that ls-files thinks it needs to dquote).t/perf/p7519-fsmonitor.sh +0
-2- # Then fully backslash-quote the paths to capture any - # whitespace so that they pass thru xargs properly.t/perf/p7519-fsmonitor.sh test_fsmonitor_suite() { · line 211
Quoted 10 lines
t/perf/p7519-fsmonitor.sh +1
-2# test_perf_w_drop_caches "status (dirty) ($DESC)" ' git ls-files | \ head -100000 | \ grep -v \" | \ - sed '\''s/\(.\)/\\\1/g'\'' | \ - xargs test-tool chmtime -300 && + test-tool touch stdin && git status 'Did you try to replace this with some variant of:
Quoted 5 lines
test_seq 1 10000 | xargs touchWhich (depending on your xargs version) would invoke "touch" commands
with however many argv items it thinks you can handle.a quick test on my Windows machine shows that
test_seq 1 10000 | xargs touchtakes 3.1 seconds.
Quoted 7 lines
just a simple
test_seq 1 10000 >/dev/nulltake 0.2 seconds.
using my test-tool helper cuts that time in half.
There's what Elijah mentioned about
test_seq, so maybe it's just that.
But what I was suggesting was using the xargs mode where it does N arguments at a time.
Does this work for you, and does it cause xargs to invoke "touch" with
the relevant N number of arguments, and does it help with the performance?Quoted 4 lines
test_seq 1 10000 | xargs touch test_seq 1 10000 | xargs -n 10 touch test_seq 1 10000 | xargs -n 100 touch test_seq 1 10000 | xargs -n 1000 touch
Quoted 6 lines
The GFW SDK version of xargs does have
-n Nand it does work as advertised. And it does slow down things considerably. Letting it do ~2500 per command in 4 commands took the 3.1 seconds listed above.Add a -n 100 to it takes 5.7 seconds, so process creation overhead is a factor here.
Doesn't -n 2500 being faster than -n 100 suggest the opposite of process overhead being the deciding factor? With -n 2500 you'll invoke 4 touch processes, so one takes 2500/3.1 =~ 0.8s to run, whereas with -n 100 you invoke 100 of them, so if the overall time is then 5.7 seconds that's 5.7/100 =~ ~0.06s.
Or am I misunderstanding you, or does some implicit parallelism kick in with that version of xargs depending on -n?
Quoted 8 lines
Quoted 6 lines
etc.
Also I didn't notice this before, but the -300 part of "chmtime -300"
was redundant before then? I.e. you're implicitly changing it to "=+0" instead with your "touch" helper, are you not?Right. I'm changing it to the current time.
If that "while we're at it change the behavior of the test" is wanted I think it should be called out in the commit message. Right now it looks like it might be an unintentional regression in the test.
Re: [PATCH v3 24/34] t/perf/p7519: speed up test using "test-tool touch"
In reply to Jeff Hostetler via GitGitGadgetQuoted 20 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.
This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.
Signed-off-by Jeff Hostetler <[email protected]>--- t/perf/p7519-fsmonitor.sh | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)t/perf/p7519-fsmonitor.sh +0
-0@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" ' fi &&
t/perf/p7519-fsmonitor.sh test_expect_success "one time repo setup" ' · line 125
Quoted 10 lines
t/perf/p7519-fsmonitor.sh +4
-4mkdir 1_file 10_files 100_files 1000_files 10000_files && - for i in $(test_seq 1 10); do touch 10_files/$i; done && - for i in $(test_seq 1 100); do touch 100_files/$i; done && - for i in $(test_seq 1 1000); do touch 1000_files/$i; done && - for i in $(test_seq 1 10000); do touch 10000_files/$i; done && + test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 && + test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 && + test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 && + test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 &&
The big win in taking *minutes* off of the run time of this test was getting rid of the for loops and one touch invocation per file.
So whether we keep my test-tool touch command or switch to test_seq or seq is open for debate. Mine seems quicker, but it is more or less round off error in the larger picture considering what we started with.
I'll play with this a bit.
Jeff
PATCH v3 25/34 t/perf: avoid copying builtin fsmonitor files into test repo
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Do not try to copy a fsmonitor--daemon socket from the current development directory into the test trash directory.
When we run the perf suite without an explicit source repo set, we copy of the current $GIT_DIR into the test trash directory. Unix domain sockets cannot be copied in that manner, so the test setup fails.
Additionally, omit any other fsmonitor--daemon temp files inside the $GIT_DIR directory.
--- t/perf/perf-lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/perf/perf-lib.sh +1 -1
@@ -74,7 +74,7 @@ test_perf_copy_repo_contents () { for stuff in "$1"/* do case "$stuff" in - */objects|*/hooks|*/config|*/commondir|*/gitdir|*/worktrees) + */objects|*/hooks|*/config|*/commondir|*/gitdir|*/worktrees|*/fsmonitor--daemon*) ;; *) cp -R "$stuff" "$repo/.git/" || exit 1
Signature · 1 line
gitgitgadget
Re: [PATCH v3 25/34] t/perf: avoid copying builtin fsmonitor files into test repo
Quoted 4 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Do not try to copy a fsmonitor--daemon socket from the current development directory into the test trash directory.
Okey, the */fsmonitor--daemon* rule covers that...
Quoted 7 lines
When we run the perf suite without an explicit source repo set, we copy of the current $GIT_DIR into the test trash directory. Unix domain sockets cannot be copied in that manner, so the test setup fails.
Additionally, omit any other fsmonitor--daemon temp files inside the $GIT_DIR directory.
So is the "any other" also matched by that rule? Not knowing the files part of this is just phrasing, would be less confusing (if that's true, and you didn't just forget to add a match for them) as:
The */fsmonitor--daemon* glob will also match temporary files the daemon creates, but that's OK. We'd like to ignore these too.
PATCH v3 26/34 t/perf/p7519: add fsmonitor--daemon test cases
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Repeat all of the fsmonitor perf tests using git fsmonitor--daemon and the "Simple IPC" interface.
--- t/perf/p7519-fsmonitor.sh | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-)
t/perf/p7519-fsmonitor.sh +34 -3
@@ -24,7 +24,8 @@ test_description="Test core.fsmonitor" # GIT_PERF_7519_SPLIT_INDEX: used to configure core.splitIndex # GIT_PERF_7519_FSMONITOR: used to configure core.fsMonitor. May be an # absolute path to an integration. May be a space delimited list of -# absolute paths to integrations. +# absolute paths to integrations. (This hook or list of hooks does not +# include the built-in fsmonitor--daemon.) # # The big win for using fsmonitor is the elimination of the need to scan the # working directory looking for changed and untracked files. If the file
@@ -136,10 +137,16 @@ test_expect_success "one time repo setup" ' setup_for_fsmonitor() { # set INTEGRATION_SCRIPT depending on the environment - if test -n "$INTEGRATION_PATH" + if test -n "$USE_FSMONITOR_DAEMON" then + git config core.useBuiltinFSMonitor true && + INTEGRATION_SCRIPT=false + elif test -n "$INTEGRATION_PATH" + then + git config core.useBuiltinFSMonitor false && INTEGRATION_SCRIPT="$INTEGRATION_PATH" else + git config core.useBuiltinFSMonitor false && # # Choose integration script based on existence of Watchman. # Fall back to an empty integration script.
@@ -175,7 +182,10 @@ test_perf_w_drop_caches () { } test_fsmonitor_suite() { - if test -n "$INTEGRATION_SCRIPT"; then + if test -n "$USE_FSMONITOR_DAEMON" + then + DESC="builtin fsmonitor--daemon" + elif test -n "$INTEGRATION_SCRIPT"; then DESC="fsmonitor=$(basename $INTEGRATION_SCRIPT)" else DESC="fsmonitor=disabled"
@@ -283,4 +293,25 @@ test_expect_success "setup without fsmonitor" ' test_fsmonitor_suite trace_stop +# +# Run a full set of perf tests using the built-in fsmonitor--daemon. +# It does not use the Hook API, so it has a different setup. +# Explicitly start the daemon here and before we start client commands +# so that we can later add custom tracing. +# +if test_have_prereq FSMONITOR_DAEMON +then + USE_FSMONITOR_DAEMON=t + + trace_start fsmonitor--daemon--server + git fsmonitor--daemon start + + trace_start fsmonitor--daemon--client + test_expect_success "setup for fsmonitor--daemon" 'setup_for_fsmonitor' + test_fsmonitor_suite + + git fsmonitor--daemon stop + trace_stop +fi + test_done
Signature · 1 line
gitgitgadget
PATCH v3 27/34 t7527: create test for fsmonitor--daemon
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
--- t/t7527-builtin-fsmonitor.sh | 497 +++++++++++++++++++++++++++++++++++ 1 file changed, 497 insertions(+) create mode 100755 t/t7527-builtin-fsmonitor.sh
t/t7527-builtin-fsmonitor.sh new file +497 -0
@@ -0,0 +1,497 @@ +#!/bin/sh + +test_description='built-in file system watcher' + +. ./test-lib.sh + +if ! test_have_prereq FSMONITOR_DAEMON +then + skip_all="fsmonitor--daemon is not supported on this platform" + test_done +fi + +stop_daemon_delete_repo () { + r=$1 + git -C $r fsmonitor--daemon stop >/dev/null 2>/dev/null + rm -rf $1 + return 0 +} + +start_daemon () { + case "$#" in + 1) r="-C $1";; + *) r=""; + esac + + git $r fsmonitor--daemon start || return $? + git $r fsmonitor--daemon status || return $? + + return 0 +} + +test_expect_success 'explicit daemon start and stop' ' + test_when_finished "stop_daemon_delete_repo test_explicit" && + + git init test_explicit && + start_daemon test_explicit && + + git -C test_explicit fsmonitor--daemon stop && + test_must_fail git -C test_explicit fsmonitor--daemon status +' + +test_expect_success 'implicit daemon start' ' + test_when_finished "stop_daemon_delete_repo test_implicit" && + + git init test_implicit && + test_must_fail git -C test_implicit fsmonitor--daemon status && + + # query will implicitly start the daemon. + # + # for test-script simplicity, we send a V1 timestamp rather than + # a V2 token. either way, the daemon response to any query contains + # a new V2 token. (the daemon may complain that we sent a V1 request, + # but this test case is only concerned with whether the daemon was + # implicitly started.) + + GIT_TRACE2_EVENT="$(pwd)/.git/trace" \ + test-tool -C test_implicit fsmonitor-client query --token 0 >actual && + nul_to_q <actual >actual.filtered && + grep "builtin:" actual.filtered && + + # confirm that a daemon was started in the background. + # + # since the mechanism for starting the background daemon is platform + # dependent, just confirm that the foreground command received a + # response from the daemon. + + grep :\"query/response-length\" .git/trace && + + git -C test_implicit fsmonitor--daemon status && + git -C test_implicit fsmonitor--daemon stop && + test_must_fail git -C test_implicit fsmonitor--daemon status +' + +test_expect_success 'implicit daemon stop (delete .git)' ' + test_when_finished "stop_daemon_delete_repo test_implicit_1" && + + git init test_implicit_1 && + + start_daemon test_implicit_1 && + + # deleting the .git directory will implicitly stop the daemon. + rm -rf test_implicit_1/.git && + + # [1] Create an empty .git directory so that the following Git + # command will stay relative to the `-C` directory. + # + # Without this, the Git command will override the requested + # -C argument and crawl out to the containing Git source tree. + # This would make the test result dependent upon whether we + # were using fsmonitor on our development worktree. + # + sleep 1 && + mkdir test_implicit_1/.git && + + test_must_fail git -C test_implicit_1 fsmonitor--daemon status +' + +test_expect_success 'implicit daemon stop (rename .git)' ' + test_when_finished "stop_daemon_delete_repo test_implicit_2" && + + git init test_implicit_2 && + + start_daemon test_implicit_2 && + + # renaming the .git directory will implicitly stop the daemon. + mv test_implicit_2/.git test_implicit_2/.xxx && + + # See [1] above. + # + sleep 1 && + mkdir test_implicit_2/.git && + + test_must_fail git -C test_implicit_2 fsmonitor--daemon status +' + +test_expect_success 'cannot start multiple daemons' ' + test_when_finished "stop_daemon_delete_repo test_multiple" && + + git init test_multiple && + + start_daemon test_multiple && + + test_must_fail git -C test_multiple fsmonitor--daemon start 2>actual && + grep "fsmonitor--daemon is already running" actual && + + git -C test_multiple fsmonitor--daemon stop && + test_must_fail git -C test_multiple fsmonitor--daemon status +' + +# These tests use the main repo in the trash directory + +test_expect_success 'setup' ' + >tracked && + >modified && + >delete && + >rename && + mkdir dir1 && + >dir1/tracked && + >dir1/modified && + >dir1/delete && + >dir1/rename && + mkdir dir2 && + >dir2/tracked && + >dir2/modified && + >dir2/delete && + >dir2/rename && + mkdir dirtorename && + >dirtorename/a && + >dirtorename/b && + + cat >.gitignore <<-\EOF && + .gitignore + expect* + actual* + EOF + + git -c core.useBuiltinFSMonitor= add . && + test_tick && + git -c core.useBuiltinFSMonitor= commit -m initial && + + git config core.useBuiltinFSMonitor true +' + +# The test already explicitly stopped (or tried to stop) the daemon. +# This is here in case something else fails first. +# +redundant_stop_daemon () { + git fsmonitor--daemon stop + return 0 +} + +test_expect_success 'update-index implicitly starts daemon' ' + test_when_finished redundant_stop_daemon && + + test_must_fail git fsmonitor--daemon status && + + GIT_TRACE2_EVENT="$(pwd)/.git/trace_implicit_1" \ + git update-index --fsmonitor && + + git fsmonitor--daemon status && + test_might_fail git fsmonitor--daemon stop && + + grep \"event\":\"start\".*\"fsmonitor--daemon\" .git/trace_implicit_1 +' + +test_expect_success 'status implicitly starts daemon' ' + test_when_finished redundant_stop_daemon && + + test_must_fail git fsmonitor--daemon status && + + GIT_TRACE2_EVENT="$(pwd)/.git/trace_implicit_2" \ + git status >actual && + + git fsmonitor--daemon status && + test_might_fail git fsmonitor--daemon stop && + + grep \"event\":\"start\".*\"fsmonitor--daemon\" .git/trace_implicit_2 +' + +edit_files() { + echo 1 >modified + echo 2 >dir1/modified + echo 3 >dir2/modified + >dir1/untracked +} + +delete_files() { + rm -f delete + rm -f dir1/delete + rm -f dir2/delete +} + +create_files() { + echo 1 >new + echo 2 >dir1/new + echo 3 >dir2/new +} + +rename_files() { + mv rename renamed + mv dir1/rename dir1/renamed + mv dir2/rename dir2/renamed +} + +file_to_directory() { + rm -f delete + mkdir delete + echo 1 >delete/new +} + +directory_to_file() { + rm -rf dir1 + echo 1 >dir1 +} + +verify_status() { + git status >actual && + GIT_INDEX_FILE=.git/fresh-index git read-tree master && + GIT_INDEX_FILE=.git/fresh-index git -c core.useBuiltinFSMonitor= status >expect && + test_cmp expect actual && + echo HELLO AFTER && + cat .git/trace && + echo HELLO AFTER +} + +# The next few test cases confirm that our fsmonitor daemon sees each type +# of OS filesystem notification that we care about. At this layer we just +# ensure we are getting the OS notifications and do not try to confirm what +# is reported by `git status`. +# +# We run a simple query after modifying the filesystem just to introduce +# a bit of a delay so that the trace logging from the daemon has time to +# get flushed to disk. +# +# We `reset` and `clean` at the bottom of each test (and before stopping the +# daemon) because these commands might implicitly restart the daemon. + +clean_up_repo_and_stop_daemon () { + git reset --hard HEAD + git clean -fd + git fsmonitor--daemon stop + rm -f .git/trace +} + +test_expect_success 'edit some files' ' + test_when_finished clean_up_repo_and_stop_daemon && + + ( + GIT_TRACE_FSMONITOR="$(pwd)/.git/trace" && + export GIT_TRACE_FSMONITOR && + + start_daemon + ) && + + edit_files && + + test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + + grep "^event: dir1/modified$" .git/trace && + grep "^event: dir2/modified$" .git/trace && + grep "^event: modified$" .git/trace && + grep "^event: dir1/untracked$" .git/trace +' + +test_expect_success 'create some files' ' + test_when_finished clean_up_repo_and_stop_daemon && + + ( + GIT_TRACE_FSMONITOR="$(pwd)/.git/trace" && + export GIT_TRACE_FSMONITOR && + + start_daemon + ) && + + create_files && + + test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + + grep "^event: dir1/new$" .git/trace && + grep "^event: dir2/new$" .git/trace && + grep "^event: new$" .git/trace +' + +test_expect_success 'delete some files' ' + test_when_finished clean_up_repo_and_stop_daemon && + + ( + GIT_TRACE_FSMONITOR="$(pwd)/.git/trace" && + export GIT_TRACE_FSMONITOR && + + start_daemon + ) && + + delete_files && + + test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + + grep "^event: dir1/delete$" .git/trace && + grep "^event: dir2/delete$" .git/trace && + grep "^event: delete$" .git/trace +' + +test_expect_success 'rename some files' ' + test_when_finished clean_up_repo_and_stop_daemon && + + ( + GIT_TRACE_FSMONITOR="$(pwd)/.git/trace" && + export GIT_TRACE_FSMONITOR && + + start_daemon + ) && + + rename_files && + + test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + + grep "^event: dir1/rename$" .git/trace && + grep "^event: dir2/rename$" .git/trace && + grep "^event: rename$" .git/trace && + grep "^event: dir1/renamed$" .git/trace && + grep "^event: dir2/renamed$" .git/trace && + grep "^event: renamed$" .git/trace +' + +test_expect_success 'rename directory' ' + test_when_finished clean_up_repo_and_stop_daemon && + + ( + GIT_TRACE_FSMONITOR="$(pwd)/.git/trace" && + export GIT_TRACE_FSMONITOR && + + start_daemon + ) && + + mv dirtorename dirrenamed && + + test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + + grep "^event: dirtorename/*$" .git/trace && + grep "^event: dirrenamed/*$" .git/trace +' + +test_expect_success 'file changes to directory' ' + test_when_finished clean_up_repo_and_stop_daemon && + + ( + GIT_TRACE_FSMONITOR="$(pwd)/.git/trace" && + export GIT_TRACE_FSMONITOR && + + start_daemon + ) && + + file_to_directory && + + test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + + grep "^event: delete$" .git/trace && + grep "^event: delete/new$" .git/trace +' + +test_expect_success 'directory changes to a file' ' + test_when_finished clean_up_repo_and_stop_daemon && + + ( + GIT_TRACE_FSMONITOR="$(pwd)/.git/trace" && + export GIT_TRACE_FSMONITOR && + + start_daemon + ) && + + directory_to_file && + + test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + + grep "^event: dir1$" .git/trace +' + +# The next few test cases exercise the token-resync code. When filesystem +# drops events (because of filesystem velocity or because the daemon isn't +# polling fast enough), we need to discard the cached data (relative to the +# current token) and start collecting events under a new token. +# +# the 'test-tool fsmonitor-client flush' command can be used to send a +# "flush" message to a running daemon and ask it to do a flush/resync. + +test_expect_success 'flush cached data' ' + test_when_finished "stop_daemon_delete_repo test_flush" && + + git init test_flush && + + ( + GIT_TEST_FSMONITOR_TOKEN=true && + export GIT_TEST_FSMONITOR_TOKEN && + + GIT_TRACE_FSMONITOR="$(pwd)/.git/trace_daemon" && + export GIT_TRACE_FSMONITOR && + + start_daemon test_flush + ) && + + # The daemon should have an initial token with no events in _0 and + # then a few (probably platform-specific number of) events in _1. + # These should both have the same <token_id>. + + test-tool -C test_flush fsmonitor-client query --token "builtin:test_00000001:0" >actual_0 && + nul_to_q <actual_0 >actual_q0 && + + touch test_flush/file_1 && + touch test_flush/file_2 && + + test-tool -C test_flush fsmonitor-client query --token "builtin:test_00000001:0" >actual_1 && + nul_to_q <actual_1 >actual_q1 && + + grep "file_1" actual_q1 && + + # Force a flush. This will change the <token_id>, reset the <seq_nr>, and + # flush the file data. Then create some events and ensure that the file + # again appears in the cache. It should have the new <token_id>. + + test-tool -C test_flush fsmonitor-client flush >flush_0 && + nul_to_q <flush_0 >flush_q0 && + grep "^builtin:test_00000002:0Q/Q$" flush_q0 && + + test-tool -C test_flush fsmonitor-client query --token "builtin:test_00000002:0" >actual_2 && + nul_to_q <actual_2 >actual_q2 && + + grep "^builtin:test_00000002:0Q$" actual_q2 && + + touch test_flush/file_3 && + + test-tool -C test_flush fsmonitor-client query --token "builtin:test_00000002:0" >actual_3 && + nul_to_q <actual_3 >actual_q3 && + + grep "file_3" actual_q3 +' + +# The next few test cases create repos where the .git directory is NOT +# inside the one of the working directory. That is, where .git is a file +# that points to a directory elsewhere. This happens for submodules and +# non-primary worktrees. + +test_expect_success 'setup worktree base' ' + git init wt-base && + echo 1 >wt-base/file1 && + git -C wt-base add file1 && + git -C wt-base commit -m "c1" +' + +test_expect_success 'worktree with .git file' ' + git -C wt-base worktree add ../wt-secondary && + + ( + GIT_TRACE2_PERF="$(pwd)/trace2_wt_secondary" && + export GIT_TRACE2_PERF && + + GIT_TRACE_FSMONITOR="$(pwd)/trace_wt_secondary" && + export GIT_TRACE_FSMONITOR && + + start_daemon wt-secondary + ) && + + git -C wt-secondary fsmonitor--daemon stop && + test_must_fail git -C wt-secondary fsmonitor--daemon status +' + +# NEEDSWORK: Repeat one of the "edit" tests on wt-secondary and +# confirm that we get the same events and behavior -- that is, that +# fsmonitor--daemon correctly watches BOTH the working directory and +# the external GITDIR directory and behaves the same as when ".git" +# is a directory inside the working directory. + +test_expect_success 'cleanup worktrees' ' + stop_daemon_delete_repo wt-secondary && + stop_daemon_delete_repo wt-base +' + +test_done
Signature · 1 line
gitgitgadget
Re: [PATCH v3 27/34] t7527: create test for fsmonitor--daemon
Quoted 6 lines · Jeff Hostetler via GitGitGadget
t/t7527-builtin-fsmonitor.sh new file +6
-0+ grep \"event\":\"start\".*\"fsmonitor--daemon\" .git/trace_implicit_1 +' + +test_expect_success 'status implicitly starts daemon' ' + test_when_finished redundant_stop_daemon && +
t/t7527-builtin-fsmonitor.sh line 198
Quoted 10 lines
t/t7527-builtin-fsmonitor.sh new file +10
-0+ test_must_fail git fsmonitor--daemon status && + + GIT_TRACE2_EVENT="$(pwd)/.git/trace_implicit_2" \ + git status >actual && + + git fsmonitor--daemon status && + test_might_fail git fsmonitor--daemon stop && + + grep \"event\":\"start\".*\"fsmonitor--daemon\" .git/trace_implicit_2 +'
Seems like this and test_region could eventually be factored into some common function that would be more flexible about grabbing trace2 data.
PATCH v3 28/34 fsmonitor--daemon: periodically truncate list of modified files
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Teach fsmonitor--daemon to periodically truncate the list of modified files to save some memory.
Clients will ask for the set of changes relative to a token that they found in the FSMN index extension in the index. (This token is like a point in time, but different). Clients will then update the index to contain the response token (so that subsequent commands will be relative to this new token).
Therefore, the daemon can gradually truncate the in-memory list of changed paths as they become obsolete (older than the previous token). Since we may have multiple clients making concurrent requests with a skew of tokens and clients may be racing to the talk to the daemon, we lazily truncate the list.
We introduce a 5 minute delay and truncate batches 5 minutes after they are considered obsolete.
--- builtin/fsmonitor--daemon.c | 80 +++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)
builtin/fsmonitor--daemon.c +80 -0
@@ -300,6 +300,77 @@ static void fsmonitor_batch__combine(struct fsmonitor_batch *batch_dest, batch_src->interned_paths[k]; } +/* +* To keep the batch list from growing unbounded in response to filesystem +* activity, we try to truncate old batches from the end of the list as +* they become irrelevant. +* +* We assume that the .git/index will be updated with the most recent token +* any time the index is updated. And future commands will only ask for +* recent changes *since* that new token. So as tokens advance into the +* future, older batch items will never be requested/needed. So we can +* truncate them without loss of functionality. +* +* However, multiple commands may be talking to the daemon concurrently +* or perform a slow command, so a little "token skew" is possible. +* Therefore, we want this to be a little bit lazy and have a generous +* delay. +* +* The current reader thread walked backwards in time from `token->batch_head` +* back to `batch_marker` somewhere in the middle of the batch list. +* +* Let's walk backwards in time from that marker an arbitrary delay +* and truncate the list there. Note that these timestamps are completely +* artificial (based on when we pinned the batch item) and not on any +* filesystem activity. +*/ +#define MY_TIME_DELAY_SECONDS (5 * 60)/* seconds */ + +static void with_lock__truncate_old_batches ( +struct fsmonitor_daemon_state *state, +const struct fsmonitor_batch *batch_marker) +{ +/* assert current thread holding state->main_lock */ + +const struct fsmonitor_batch *batch; +struct fsmonitor_batch *rest; +struct fsmonitor_batch *p; + +if (!batch_marker) +return ; + +trace_printf_key (&trace_fsmonitor,"Truncate: mark (%" PRIu64",%" PRIu64")" , + batch_marker->batch_seq_nr, + (uint64_t )batch_marker->pinned_time); + +for (batch = batch_marker; batch; batch = batch->next) { +time_t t; + +if (!batch->pinned_time)/* an overflow batch */ +continue ; + + t = batch->pinned_time +MY_TIME_DELAY_SECONDS ; +if (t > batch_marker->pinned_time)/* too close to marker */ +continue ; + + goto truncate_past_here; + } + +return ; + +truncate_past_here: + state->current_token_data->batch_tail = (struct fsmonitor_batch *)batch; + + rest = ((struct fsmonitor_batch *)batch)->next; + ((struct fsmonitor_batch *)batch)->next =NULL ; + +for (p = rest; p; p =fsmonitor_batch__pop (p)) { +trace_printf_key (&trace_fsmonitor, +"Truncate: kill (%" PRIu64",%" PRIu64")" , + p->batch_seq_nr, (uint64_t )p->pinned_time); + } +} +static void fsmonitor_free_token_data (struct fsmonitor_token_data *token) {struct fsmonitor_batch *p;
@@ -645,6 +716,15 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,* that work. */ fsmonitor_free_token_data (token_data); + }else if (batch) { +/* +* This batch is the first item in the list +* that is older than the requested sequence +* number and might be considered to be +* obsolete. See if we can truncate the list +* and save some memory. +*/ +with_lock__truncate_old_batches (state, batch); } }
Signature · 1 line
gitgitgadget
PATCH v3 29/34 fsmonitor--daemon: use a cookie file to sync with file system
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Teach fsmonitor--daemon client threads to create a cookie file inside the .git directory and then wait until FS events for the cookie are observed by the FS listener thread.
This helps address the racy nature of file system events by blocking the client response until the kernel has drained any event backlog.
This is especially important on MacOS where kernel events are only issued with a limited frequency. See the latency argument of FSeventStreamCreate(). The kernel only signals every latency seconds, but does not guarantee that the kernel queue is completely drained, so we may have to wait more than one interval. If we increase the frequency, the system is more likely to drop events. We avoid these issues by having each client thread create a unique cookie file and then wait until it is seen in the event stream.
--- builtin/fsmonitor--daemon.c | 228 +++++++++++++++++++++++++++++++++++- fsmonitor--daemon.h | 5 + 2 files changed, 232 insertions(+), 1 deletion(-)
builtin/fsmonitor--daemon.c +227 -1
@@ -94,6 +94,149 @@ static int do_as_client__status(void) } } +enum fsmonitor_cookie_item_result { +FCIR_ERROR =-1 ,/* could not create cookie file ? */ +FCIR_INIT =0 , +FCIR_SEEN , +FCIR_ABORT , +}; + +struct fsmonitor_cookie_item { +struct hashmap_entry entry; +const char *name; +enum fsmonitor_cookie_item_result result; +}; + +static int cookies_cmp (const void *data,const struct hashmap_entry *he1, +const struct hashmap_entry *he2,const void *keydata) +{ +const struct fsmonitor_cookie_item *a = + container_of(he1, const struct fsmonitor_cookie_item, entry); +const struct fsmonitor_cookie_item *b = + container_of(he2, const struct fsmonitor_cookie_item, entry); + +return strcmp (a->name, keydata ? keydata : b->name); +} + +static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie ( +struct fsmonitor_daemon_state *state) +{ +/* assert current thread holding state->main_lock */ + +int fd; +struct fsmonitor_cookie_item *cookie; +struct strbuf cookie_pathname =STRBUF_INIT ; +struct strbuf cookie_filename =STRBUF_INIT ; +enum fsmonitor_cookie_item_result result; +int my_cookie_seq; + +CALLOC_ARRAY (cookie,1 ); + + my_cookie_seq = state->cookie_seq++; + +strbuf_addf (&cookie_filename,"%i-%i" ,getpid (), my_cookie_seq); + +strbuf_addbuf (&cookie_pathname, &state->path_cookie_prefix); +strbuf_addbuf (&cookie_pathname, &cookie_filename); + + cookie->name =strbuf_detach (&cookie_filename,NULL ); + cookie->result =FCIR_INIT ; +hashmap_entry_init (&cookie->entry,strhash (cookie->name)); + +hashmap_add (&state->cookies, &cookie->entry); + +trace_printf_key (&trace_fsmonitor,"cookie-wait: '%s' '%s'" , + cookie->name, cookie_pathname.buf); + +/* +* Create the cookie file on disk and then wait for a notification +* that the listener thread has seen it. +*/ + fd =open (cookie_pathname.buf,O_WRONLY |O_CREAT |O_EXCL ,0600 ); +if (fd >=0 ) { +close (fd); +unlink (cookie_pathname.buf); + +/* +* NEEDSWORK: This is an infinite wait (well, unless another +* thread sends us an abort). I'd like to change this to +* use `pthread_cond_timedwait()` and return an error/timeout +* and let the caller do the trivial response thing. +*/ +while (cookie->result ==FCIR_INIT ) +pthread_cond_wait (&state->cookies_cond, + &state->main_lock); + }else { +error_errno (_ ("could not create fsmonitor cookie '%s'" ), + cookie->name); + + cookie->result =FCIR_ERROR ; + } + +hashmap_remove (&state->cookies, &cookie->entry,NULL ); + + result = cookie->result; + +free ((char *)cookie->name); +free (cookie); +strbuf_release (&cookie_pathname); + +return result; +} + +/* +* Mark these cookies as _SEEN and wake up the corresponding client threads. +*/ +static void with_lock__mark_cookies_seen (struct fsmonitor_daemon_state *state, +const struct string_list *cookie_names) +{ +/* assert current thread holding state->main_lock */ + +int k; +int nr_seen =0 ; + +for (k =0 ; k < cookie_names->nr; k++) { +struct fsmonitor_cookie_item key; +struct fsmonitor_cookie_item *cookie; + + key.name = cookie_names->items[k].string; +hashmap_entry_init (&key.entry,strhash (key.name)); + + cookie =hashmap_get_entry (&state->cookies, &key, entry,NULL ); +if (cookie) { +trace_printf_key (&trace_fsmonitor,"cookie-seen: '%s'" , + cookie->name); + cookie->result =FCIR_SEEN ; + nr_seen++; + } + } + +if (nr_seen) +pthread_cond_broadcast (&state->cookies_cond); +} + +/* +* Set _ABORT on all pending cookies and wake up all client threads. +*/ +static void with_lock__abort_all_cookies (struct fsmonitor_daemon_state *state) +{ +/* assert current thread holding state->main_lock */ + +struct hashmap_iter iter; +struct fsmonitor_cookie_item *cookie; +int nr_aborted =0 ; + +hashmap_for_each_entry (&state->cookies, &iter, cookie, entry) { +trace_printf_key (&trace_fsmonitor,"cookie-abort: '%s'" , + cookie->name); + cookie->result =FCIR_ABORT ; + nr_aborted++; + } + +if (nr_aborted) +pthread_cond_broadcast (&state->cookies_cond); +} +/* * Requests to and from a FSMonitor Protocol V2 provider use an opaque * "token" as a virtual timestamp. Clients can request a summary of all
@@ -397,6 +540,9 @@ static void fsmonitor_free_token_data(struct fsmonitor_token_data *token)* We should create a new token and start fresh (as if we just * booted up). * +* [2] Some of those lost events may have been for cookie files. We +* should assume the worst and abort them rather letting them starve. +* * If there are no concurrent threads readering the current token data * series, we can free it now. Otherwise, let the last reader free * it.
@@ -418,6 +564,8 @@ static void with_lock__do_force_resync(struct fsmonitor_daemon_state *state) state->current_token_data = new_one;fsmonitor_free_token_data (free_me); + +with_lock__abort_all_cookies (state); }void fsmonitor_force_resync (struct fsmonitor_daemon_state *state)
@@ -492,6 +640,8 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,int hash_ret;int do_trivial =0 ;int do_flush =0 ; +int do_cookie =0 ; +enum fsmonitor_cookie_item_result cookie_result;/* * We expect `command` to be of the form:
@@ -552,6 +702,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,* We have a V2 valid token: * "builtin:<token_id>:<seq_nr>" */ + do_cookie =1 ; } }
@@ -560,6 +711,30 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,if (!state->current_token_data)BUG ("fsmonitor state does not have a current token" ); +/* +* Write a cookie file inside the directory being watched in +* an effort to flush out existing filesystem events that we +* actually care about. Suspend this client thread until we +* see the filesystem events for this cookie file. +* +* Creating the cookie lets us guarantee that our FS listener +* thread has drained the kernel queue and we are caught up +* with the kernel. +* +* If we cannot create the cookie (or otherwise guarantee that +* we are caught up), we send a trivial response. We have to +* assume that there might be some very, very recent activity +* on the FS still in flight. +*/ +if (do_cookie) { + cookie_result =with_lock__wait_for_cookie (state); +if (cookie_result !=FCIR_SEEN ) { +error (_ ("fsmonitor: cookie_result '%d' != SEEN" ), + cookie_result); + do_trivial =1 ; + } + } +if (do_flush)with_lock__do_force_resync (state);
@@ -771,7 +946,9 @@ static int handle_client(void *data,return result; } -#define FSMONITOR_COOKIE_PREFIX ".fsmonitor-daemon-" +#define FSMONITOR_DIR "fsmonitor--daemon" +#define FSMONITOR_COOKIE_DIR "cookies" +#define FSMONITOR_COOKIE_PREFIX (FSMONITOR_DIR "/" FSMONITOR_COOKIE_DIR "/")enum fsmonitor_path_type fsmonitor_classify_path_workdir_relative (const char *rel)
@@ -924,6 +1101,9 @@ void fsmonitor_publish(struct fsmonitor_daemon_state *state, } } +if (cookie_names->nr) +with_lock__mark_cookies_seen (state, cookie_names); +pthread_mutex_unlock (&state->main_lock); }
@@ -1013,7 +1193,9 @@ static int fsmonitor_run_daemon(void)memset (&state,0 ,sizeof (state)); +hashmap_init (&state.cookies, cookies_cmp,NULL ,0 );pthread_mutex_init (&state.main_lock,NULL ); +pthread_cond_init (&state.cookies_cond,NULL ); state.error_code =0 ; state.current_token_data =fsmonitor_new_token_data ();
@@ -1038,6 +1220,44 @@ static int fsmonitor_run_daemon(void) state.nr_paths_watching =2 ; } +/* +* We will write filesystem syncing cookie files into +* <gitdir>/<fsmonitor-dir>/<cookie-dir>/<pid>-<seq>. +* +* The extra layers of subdirectories here keep us from +* changing the mtime on ".git/" or ".git/foo/" when we create +* or delete cookie files. +* +* There have been problems with some IDEs that do a +* non-recursive watch of the ".git/" directory and run a +* series of commands any time something happens. +* +* For example, if we place our cookie files directly in +* ".git/" or ".git/foo/" then a `git status` (or similar +* command) from the IDE will cause a cookie file to be +* created in one of those dirs. This causes the mtime of +* those dirs to change. This triggers the IDE's watch +* notification. This triggers the IDE to run those commands +* again. And the process repeats and the machine never goes +* idle. +* +* Adding the extra layers of subdirectories prevents the +* mtime of ".git/" and ".git/foo" from changing when a +* cookie file is created. +*/ +strbuf_init (&state.path_cookie_prefix,0 ); +strbuf_addbuf (&state.path_cookie_prefix, &state.path_gitdir_watch); + +strbuf_addch (&state.path_cookie_prefix,'/' ); +strbuf_addstr (&state.path_cookie_prefix,FSMONITOR_DIR ); +mkdir (state.path_cookie_prefix.buf,0777 ); + +strbuf_addch (&state.path_cookie_prefix,'/' ); +strbuf_addstr (&state.path_cookie_prefix,FSMONITOR_COOKIE_DIR ); +mkdir (state.path_cookie_prefix.buf,0777 ); + +strbuf_addch (&state.path_cookie_prefix,'/' ); +/* * Confirm that we can create platform-specific resources for the * filesystem listener before we bother starting all the threads.
@@ -1050,6 +1270,7 @@ static int fsmonitor_run_daemon(void) err =fsmonitor_run_daemon_1 (&state); done: +pthread_cond_destroy (&state.cookies_cond);pthread_mutex_destroy (&state.main_lock);fsmonitor_fs_listen__dtor (&state);
@@ -1057,6 +1278,11 @@ done:strbuf_release (&state.path_worktree_watch);strbuf_release (&state.path_gitdir_watch); +strbuf_release (&state.path_cookie_prefix); + +/* +* NEEDSWORK: Consider "rm -rf <gitdir>/<fsmonitor-dir>" +*/ return err; }
fsmonitor--daemon.h +5 -0
@@ -45,6 +45,11 @@ struct fsmonitor_daemon_state {struct fsmonitor_token_data *current_token_data; +struct strbuf path_cookie_prefix; +pthread_cond_t cookies_cond; +int cookie_seq; +struct hashmap cookies; +int error_code;struct fsmonitor_daemon_backend_data *backend_data;
Signature · 1 line
gitgitgadget
Re: [PATCH v3 29/34] fsmonitor--daemon: use a cookie file to sync with file system
Quoted 10 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Teach fsmonitor--daemon client threads to create a cookie file inside the .git directory and then wait until FS events for the cookie are observed by the FS listener thread.
This helps address the racy nature of file system events by blocking the client response until the kernel has drained any event backlog.
Quoted 8 lines
This is especially important on MacOS where kernel events are only issued with a limited frequency. See the
latencyargument ofFSeventStreamCreate(). The kernel only signals everylatencyseconds, but does not guarantee that the kernel queue is completely drained, so we may have to wait more than one interval. If we increase the frequency, the system is more likely to drop events. We avoid these issues by having each client thread create a unique cookie file and then wait until it is seen in the event stream.
Is this a guaranteed property of any API fsmonitor might need to work with (linux, darwin, Windows) that if I perform a bunch of FS operations on my working tree, that if I finish up by touching this cookie file that that'll happen last?
I'd think that wouldn't be the case, i.e. on POSIX filesystems unless you run around fsyncing both files and directories you're not guaranteed that they're on disk, and even then the kernel might decide to sync your cookie earlier, won't it?
E.g. on Linux you can even have cross-FS watches, and mix & match different FS types. I'd expect to get events in whatever implementation-defined order the VFS layer + FS decided to sync them to disk in & get to firing off an event for me.
Or do these APIs all guarantee that a linear view of the world is presented to the API consumer?
Quoted 26 lines
Co-authored-by Kevin Willford <[email protected]>Co-authored-by Johannes Schindelin <[email protected]>Signed-off-by Jeff Hostetler <[email protected]>--- builtin/fsmonitor--daemon.c | 228 +++++++++++++++++++++++++++++++++++- fsmonitor--daemon.h | 5 + 2 files changed, 232 insertions(+), 1 deletion(-)builtin/fsmonitor--daemon.c +10
-0@@ -94,6 +94,149 @@ static int do_as_client__status(void) } } +enum fsmonitor_cookie_item_result { +FCIR_ERROR =-1 ,/* could not create cookie file ? */ +FCIR_INIT =0 , +FCIR_SEEN , +FCIR_ABORT , +}; + +struct fsmonitor_cookie_item { +struct hashmap_entry entry; +const char *name;
builtin/fsmonitor--daemon.c static int do_as_client__status(void) · line 116
Quoted 10 lines
builtin/fsmonitor--daemon.c +10
-0+enum fsmonitor_cookie_item_result result; +}; + +static int cookies_cmp (const void *data,const struct hashmap_entry *he1, +const struct hashmap_entry *he2,const void *keydata) +{ +const struct fsmonitor_cookie_item *a = + container_of(he1, const struct fsmonitor_cookie_item, entry); +const struct fsmonitor_cookie_item *b = + container_of(he2, const struct fsmonitor_cookie_item, entry);
Re earlier comments about verbose names, these are just enums in a builtin/*.c file, so a name like "cookie_item" is fine, and then the whole thing might even fit on one line.. :)
Quoted 28 lines
[...]
builtin/fsmonitor--daemon.c +27
-0+/* +* We will write filesystem syncing cookie files into +* <gitdir>/<fsmonitor-dir>/<cookie-dir>/<pid>-<seq>. +* +* The extra layers of subdirectories here keep us from +* changing the mtime on ".git/" or ".git/foo/" when we create +* or delete cookie files. +* +* There have been problems with some IDEs that do a +* non-recursive watch of the ".git/" directory and run a +* series of commands any time something happens. +* +* For example, if we place our cookie files directly in +* ".git/" or ".git/foo/" then a `git status` (or similar +* command) from the IDE will cause a cookie file to be +* created in one of those dirs. This causes the mtime of +* those dirs to change. This triggers the IDE's watch +* notification. This triggers the IDE to run those commands +* again. And the process repeats and the machine never goes +* idle. +* +* Adding the extra layers of subdirectories prevents the +* mtime of ".git/" and ".git/foo" from changing when a +* cookie file is created. +*/ +strbuf_init (&state.path_cookie_prefix,0 ); +strbuf_addbuf (&state.path_cookie_prefix, &state.path_gitdir_watch);
builtin/fsmonitor--daemon.c static int fsmonitor_run_daemon(void) · line 1259
Quoted 10 lines
builtin/fsmonitor--daemon.c +10
-0+ +strbuf_addch (&state.path_cookie_prefix,'/' ); +strbuf_addstr (&state.path_cookie_prefix,FSMONITOR_DIR ); +mkdir (state.path_cookie_prefix.buf,0777 ); + +strbuf_addch (&state.path_cookie_prefix,'/' ); +strbuf_addstr (&state.path_cookie_prefix,FSMONITOR_COOKIE_DIR ); +mkdir (state.path_cookie_prefix.buf,0777 ); + +strbuf_addch (&state.path_cookie_prefix,'/' );
So, on some stupid IDEs (would be nice to have specifics in the commit message, which ones/versions?) this avoids causing infinite activity, but on slightly more industrious stupid IDEs that would do their own recursive watch we'll have the same problem?
Perhaps we should just consider creating it at the top-level and those IDEs will eventually sort out their bugs, sooner than later if this feature ships...
builtin/fsmonitor--daemon.c done: · line 1285
Quoted 5 lines
builtin/fsmonitor--daemon.c +5
-0+strbuf_release (&state.path_cookie_prefix); + +/* +* NEEDSWORK: Consider "rm -rf <gitdir>/<fsmonitor-dir>" +*/
That would also make this trivial, presumably it's a "needswork" since you have this recursive structure, but if it's at the top-level we already did the unlink() above, so NEEDSWORK solved then?
Re: [PATCH v3 29/34] fsmonitor--daemon: use a cookie file to sync with file system
Quoted 32 lines · Ævar Arnfjörð Bjarmason
Quoted 10 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Teach fsmonitor--daemon client threads to create a cookie file inside the .git directory and then wait until FS events for the cookie are observed by the FS listener thread.
This helps address the racy nature of file system events by blocking the client response until the kernel has drained any event backlog.
Quoted 8 lines
This is especially important on MacOS where kernel events are only issued with a limited frequency. See the
latencyargument ofFSeventStreamCreate(). The kernel only signals everylatencyseconds, but does not guarantee that the kernel queue is completely drained, so we may have to wait more than one interval. If we increase the frequency, the system is more likely to drop events. We avoid these issues by having each client thread create a unique cookie file and then wait until it is seen in the event stream.Is this a guaranteed property of any API fsmonitor might need to work with (linux, darwin, Windows) that if I perform a bunch of FS operations on my working tree, that if I finish up by touching this cookie file that that'll happen last?
I'd think that wouldn't be the case, i.e. on POSIX filesystems unless you run around fsyncing both files and directories you're not guaranteed that they're on disk, and even then the kernel might decide to sync your cookie earlier, won't it?
Quoted 8 lines
E.g. on Linux you can even have cross-FS watches, and mix & match different FS types. I'd expect to get events in whatever implementation-defined order the VFS layer + FS decided to sync them to disk in & get to firing off an event for me.
Or do these APIs all guarantee that a linear view of the world is presented to the API consumer?
Theoretically, none of these APIs guarantee a complete linear ordering. We receive events from the FS in the order that the FS decides to perform the actual IO. And the inner workings of the FS is private. Even if we directly read the journal rather than listening for notifications, we probably still don't know whether the FS reordered the queue of things heading to disk.
However in practice, the events for the cookie files do tend to arrive in order. And the net effect is that the worker thread in the daemon is sync'd up with IO activity that was initiated before the request.
BTW Watchman also uses cookie files for this same reason.
It should also be noted that some operations are just racy. If you're doing a bunch of IO in one window and a 'git status' in another window, your result will be racy -- status (without FSM) makes 2 passes on the
disk: the first to verify mtimes on items in the index and the second to look for untracked files. the status result may be "blurry" (for lack of a better word). So the same questions
"does the FS reorder my IO?",
"did status see the fully sync'd FS?",
and etc can also be asked in the normal (non FSM) case, right?
So it may be the case that having an fsmonitor (mine, Watchman, etc) and the untracked-cache, we'll have less skew in status results because the status process shouldn't have to do any scanning. But I'm not sure I want to make that assertion yet.
Thanks,
Jeff
PATCH v3 30/34 fsmonitor: enhance existing comments
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
--- fsmonitor.c | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-)
fsmonitor.c +30 -7
@@ -360,9 +360,25 @@ void refresh_fsmonitor(struct index_state *istate) } apply_results: -/* a fsmonitor process can return '/' to indicate all entries are invalid */ +/* +* The response from FSMonitor (excluding the header token) is +* either: +* +* [a] a (possibly empty) list of NUL delimited relative +* pathnames of changed paths. This list can contain +* files and directories. Directories have a trailing +* slash. +* +* [b] a single '/' to indicate the provider had no +* information and that we should consider everything +* invalid. We call this a trivial response. +*/ if (query_success && query_result.buf[bol] !='/' ) { -/* Mark all entries returned by the monitor as dirty */ +/* +* Mark all pathnames returned by the monitor as dirty. +* +* This updates both the cache-entries and the untracked-cache. +*/ buf = query_result.buf;for (i = bol; i < query_result.len; i++) {if (buf[i] !='\0' )
@@ -377,11 +393,15 @@ apply_results:if (istate->untracked) istate->untracked->use_fsmonitor =1 ; }else { - -/* We only want to run the post index changed hook if we've actually changed entries, so keep track -* if we actually changed entries or not */ +/* +* We received a trivial response, so invalidate everything. +* +* We only want to run the post index changed hook if +* we've actually changed entries, so keep track if we +* actually changed entries or not. +*/ int is_cache_changed =0 ; -/* Mark all entries invalid */ +for (i =0 ; i < istate->cache_nr; i++) {if (istate->cache[i]->ce_flags &CE_FSMONITOR_VALID ) { is_cache_changed =1 ;
@@ -389,7 +409,10 @@ apply_results: } } -/* If we're going to check every file, ensure we save the results */ +/* +* If we're going to check every file, ensure we save +* the results. +*/ if (is_cache_changed) istate->cache_changed |=FSMONITOR_CHANGED ;
Signature · 1 line
gitgitgadget
PATCH v3 31/34 fsmonitor: force update index after large responses
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Set the FSMONITOR_CHANGED bit on istate->cache_changed when FSMonitor returns a very large repsonse to ensure that the index is written to disk.
Normally, when the FSMonitor response includes a tracked file, the index is always updated. Similarly, the index might be updated when the response alters the untracked-cache (when enabled). However, in cases where neither of those cause the index to be considered changed, the FSMonitor response is wasted. Subsequent Git commands will make requests with the same token and receive the same response.
If that response is very large, performance may suffer. It would be more efficient to force update the index now (and the token in the index extension) in order to reduce the size of the response received by future commands.
This was observed on Windows after a large checkout. On Windows, the kernel emits events for the files that are changed as they are changed. However, it might delay events for the containing directories until the system is more idle (or someone scans the directory (so it seems)). The first status following a checkout would get the list of files. The subsequent status commands would get the list of directories as the events trickled out. But they would never catch up because the token was not advanced because the index wasn't updated.
This list of directories caused wt_status_collect_untracked() to unnecessarily spend time actually scanning them during each command.
--- fsmonitor.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-)
fsmonitor.c +49 -1
@@ -236,6 +236,45 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)untracked_cache_invalidate_path (istate, name,0 ); } +/* +* The number of pathnames that we need to receive from FSMonitor +* before we force the index to be updated. +* +* Note that any pathname within the set of received paths MAY cause +* cache-entry or istate flag bits to be updated and thus cause the +* index to be updated on disk. +* +* However, the response may contain many paths (such as ignored +* paths) that will not update any flag bits. And thus not force the +* index to be updated. (This is fine and normal.) It also means +* that the token will not be updated in the FSMonitor index +* extension. So the next Git command will find the same token in the +* index, make the same token-relative request, and receive the same +* response (plus any newly changed paths). If this response is large +* (and continues to grow), performance could be impacted. +* +* For example, if the user runs a build and it writes 100K object +* files but doesn't modify any source files, the index would not need +* to be updated. The FSMonitor response (after the build and +* relative to a pre-build token) might be 5MB. Each subsequent Git +* command will receive that same 100K/5MB response until something +* causes the index to be updated. And `refresh_fsmonitor()` will +* have to iterate over those 100K paths each time. +* +* Performance could be improved if we optionally force update the +* index after a very large response and get an updated token into +* the FSMonitor index extension. This should allow subsequent +* commands to get smaller and more current responses. +* +* The value chosen here does not need to be precise. The index +* will be updated automatically the first time the user touches +* a tracked file and causes a command like `git status` to +* update an mtime to be updated and/or set a flag bit. +* +* NEEDSWORK: Does this need to be a config value? +*/ +static int fsmonitor_force_update_threshold =100 ; +void refresh_fsmonitor (struct index_state *istate) {struct strbuf query_result =STRBUF_INIT ;
@@ -379,19 +418,28 @@ apply_results:* * This updates both the cache-entries and the untracked-cache. */ +int count =0 ; + buf = query_result.buf;for (i = bol; i < query_result.len; i++) {if (buf[i] !='\0' )continue ;fsmonitor_refresh_callback (istate, buf + bol); bol = i +1 ; + count++; } -if (bol < query_result.len) +if (bol < query_result.len) {fsmonitor_refresh_callback (istate, buf + bol); + count++; + }/* Now mark the untracked cache for fsmonitor usage */ if (istate->untracked) istate->untracked->use_fsmonitor =1 ; + +if (count > fsmonitor_force_update_threshold) + istate->cache_changed |=FSMONITOR_CHANGED ; + }else {/* * We received a trivial response, so invalidate everything.
Signature · 1 line
gitgitgadget
PATCH v3 32/34 t7527: test status with untracked-cache and fsmonitor--daemon
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create 2x2 test matrix with the untracked-cache and fsmonitor--daemon features and a series of edits and verify that status output is identical.
--- t/t7527-builtin-fsmonitor.sh | 87 ++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+)
t/t7527-builtin-fsmonitor.sh +87 -0
@@ -152,6 +152,8 @@ test_expect_success 'setup' ' .gitignore expect* actual* + flush* + trace* EOF git -c core.useBuiltinFSMonitor= add . &&
@@ -494,4 +496,89 @@ test_expect_success 'cleanup worktrees' ' stop_daemon_delete_repo wt-base ' +# The next few tests perform arbitrary/contrived file operations and +# confirm that status is correct. That is, that the data (or lack of +# data) from fsmonitor doesn't cause incorrect results. And doesn't +# cause incorrect results when the untracked-cache is enabled. + +test_lazy_prereq UNTRACKED_CACHE ' + { git update-index --test-untracked-cache; ret=$?; } && + test $ret -ne 1 +' + +test_expect_success 'Matrix: setup for untracked-cache,fsmonitor matrix' ' + test_might_fail git config --unset core.useBuiltinFSMonitor && + git update-index --no-fsmonitor && + test_might_fail git fsmonitor--daemon stop +' + +matrix_clean_up_repo () { + git reset --hard HEAD + git clean -fd +} + +matrix_try () { + uc=$1 + fsm=$2 + fn=$3 + + test_expect_success "Matrix[uc:$uc][fsm:$fsm] $fn" ' + matrix_clean_up_repo && + $fn && + if test $uc = false -a $fsm = false + then + git status --porcelain=v1 >.git/expect.$fn + else + git status --porcelain=v1 >.git/actual.$fn + test_cmp .git/expect.$fn .git/actual.$fn + fi + ' + + return $? +} + +uc_values="false" +test_have_prereq UNTRACKED_CACHE && uc_values="false true" +for uc_val in $uc_values +do + if test $uc_val = false + then + test_expect_success "Matrix[uc:$uc_val] disable untracked cache" ' + git config core.untrackedcache false && + git update-index --no-untracked-cache + ' + else + test_expect_success "Matrix[uc:$uc_val] enable untracked cache" ' + git config core.untrackedcache true && + git update-index --untracked-cache + ' + fi + + fsm_values="false true" + for fsm_val in $fsm_values + do + if test $fsm_val = false + then + test_expect_success "Matrix[uc:$uc_val][fsm:$fsm_val] disable fsmonitor" ' + test_might_fail git config --unset core.useBuiltinFSMonitor && + git update-index --no-fsmonitor && + test_might_fail git fsmonitor--daemon stop 2>/dev/null + ' + else + test_expect_success "Matrix[uc:$uc_val][fsm:$fsm_val] enable fsmonitor" ' + git config core.useBuiltinFSMonitor true && + git fsmonitor--daemon start && + git update-index --fsmonitor + ' + fi + + matrix_try $uc_val $fsm_val edit_files + matrix_try $uc_val $fsm_val delete_files + matrix_try $uc_val $fsm_val create_files + matrix_try $uc_val $fsm_val rename_files + matrix_try $uc_val $fsm_val file_to_directory + matrix_try $uc_val $fsm_val directory_to_file + done +done + test_done
Signature · 1 line
gitgitgadget
PATCH v3 33/34 fsmonitor: handle shortname for .git
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
On Windows, teach FSMonitor to recognize the shortname of ".git" as an alias for ".git".
Sometimes we receive FS events using the shortname, such as when a CMD shell runs "RENAME GIT~1 FOO" or "RMDIR GIT~1". The FS notification arrives using whatever combination of long and shortnames used by the other process. (Shortnames do seem to be case normalized, however.)
NEEDSWORK: This only addresses the case of removing or renaming the ".git" directory using the shortname alias, so that the daemon properly shuts down. I'm leaving it a task for later to handle the general case of shortnames and report them to the fsmonitor client process. This would include tracked and untracked paths that just happen to have a shortname alias.
--- compat/fsmonitor/fsmonitor-fs-listen-win32.c | 192 +++++++++++++++---- t/t7527-builtin-fsmonitor.sh | 65 +++++++ 2 files changed, 217 insertions(+), 40 deletions(-)
compat/fsmonitor/fsmonitor-fs-listen-win32.c +152 -40
@@ -48,6 +48,8 @@ struct fsmonitor_daemon_backend_data#define LISTENER_HAVE_DATA_WORKTREE 1#define LISTENER_HAVE_DATA_GITDIR 2int nr_listener_handles; + +struct strbuf dot_git_shortname; };/*
@@ -250,6 +252,62 @@ static void cancel_rdcw_watch(struct one_watch *watch) watch->is_active = FALSE; } +/* +* Process a single relative pathname event. +* Return 1 if we should shutdown. +*/ +static int process_1_worktree_event ( +FILE_NOTIFY_INFORMATION *info, +struct string_list *cookie_list, +struct fsmonitor_batch **batch, +const struct strbuf *path, +enum fsmonitor_path_type t) +{ +const char *slash; + +switch (t) { +case IS_INSIDE_DOT_GIT_WITH_COOKIE_PREFIX : +/* special case cookie files within .git */ + +/* Use just the filename of the cookie file. */ + slash =find_last_dir_sep (path->buf); +string_list_append (cookie_list, + slash ? slash +1 : path->buf); +break ; + +case IS_INSIDE_DOT_GIT : +/* ignore everything inside of "<worktree>/.git/" */ +break ; + +case IS_DOT_GIT : +/* "<worktree>/.git" was deleted (or renamed away) */ +if ((info->Action ==FILE_ACTION_REMOVED ) || + (info->Action ==FILE_ACTION_RENAMED_OLD_NAME )) { +trace2_data_string ("fsmonitor" ,NULL , +"fsm-listen/dotgit" , +"removed" ); +return 1 ; + } +break ; + +case IS_WORKDIR_PATH : +/* queue normal pathname */ +if (!*batch) + *batch =fsmonitor_batch__new (); +fsmonitor_batch__add_path (*batch, path->buf); +break ; + +case IS_GITDIR : +case IS_INSIDE_GITDIR : +case IS_INSIDE_GITDIR_WITH_COOKIE_PREFIX : +default : +BUG ("unexpected path classification '%d' for '%s'" , + t, path->buf); + } + +return 0 ; +} +/* * Process filesystem events that happen anywhere (recursively) under the * <worktree> root directory. For a normal working directory, this includes
@@ -294,7 +352,6 @@ static int process_worktree_events(struct fsmonitor_daemon_state *state)*/ for (;;) {FILE_NOTIFY_INFORMATION *info = (void *)p; -const char *slash;enum fsmonitor_path_type t;strbuf_reset (&path);
@@ -303,45 +360,45 @@ static int process_worktree_events(struct fsmonitor_daemon_state *state) t =fsmonitor_classify_path_workdir_relative (path.buf); -switch (t) { -case IS_INSIDE_DOT_GIT_WITH_COOKIE_PREFIX : -/* special case cookie files within .git */ - -/* Use just the filename of the cookie file. */ - slash =find_last_dir_sep (path.buf); -string_list_append (&cookie_list, - slash ? slash +1 : path.buf); -break ; - -case IS_INSIDE_DOT_GIT : -/* ignore everything inside of "<worktree>/.git/" */ -break ; - -case IS_DOT_GIT : -/* "<worktree>/.git" was deleted (or renamed away) */ -if ((info->Action ==FILE_ACTION_REMOVED ) || - (info->Action ==FILE_ACTION_RENAMED_OLD_NAME )) { -trace2_data_string ("fsmonitor" ,NULL , -"fsm-listen/dotgit" , -"removed" ); - goto force_shutdown; - } -break ; - -case IS_WORKDIR_PATH : -/* queue normal pathname */ -if (!batch) - batch =fsmonitor_batch__new (); -fsmonitor_batch__add_path (batch, path.buf); -break ; - -case IS_GITDIR : -case IS_INSIDE_GITDIR : -case IS_INSIDE_GITDIR_WITH_COOKIE_PREFIX : -default : -BUG ("unexpected path classification '%d' for '%s'" , - t, path.buf); - } +if (process_1_worktree_event (info, &cookie_list, &batch, + &path, t)) + goto force_shutdown; + +/* +* NEEDSWORK: If `path` contains a shortname (that is, +* if any component within it is a shortname), we +* should expand it to a longname (See +* `GetLongPathNameW()`) and re-normalize, classify, +* and process it because our client is probably +* expecting "normal" paths. +* +* HOWEVER, if our process has called `chdir()` to get +* us out of the root of the worktree (so that the +* root directory is not busy), then we have to be +* careful to convert the paths in the INFO array +* (which are relative to the directory of the RDCW +* watch and not the CWD) into absolute paths before +* calling GetLongPathNameW() and then convert the +* computed value back to a RDCW-relative pathname +* (which is what we and the client expect). +* +* FOR NOW, just handle case (1) exactly so that we +* shutdown properly when ".git" is deleted via the +* shortname alias. +* +* We might see case (2) events for cookie files, but +* we can ignore them. +* +* FOR LATER, handle case (3) where the worktree +* events contain shortnames. We should convert +* them to longnames to avoid confusing the client. +*/ +if (data->dot_git_shortname.len && + !strcmp (path.buf, data->dot_git_shortname.buf) && +process_1_worktree_event (info, &cookie_list, &batch, + &data->dot_git_shortname, +IS_DOT_GIT )) + goto force_shutdown; skip_this_path:if (!info->NextEntryOffset)
@@ -415,6 +472,14 @@ static int process_gitdir_events(struct fsmonitor_daemon_state *state) t, path.buf); } +/* +* WRT shortnames, this external gitdir will not see +* case (1) nor case (3) events. +* +* We might see case (2) events for cookie files, but +* we can ignore them. +*/ + skip_this_path:if (!info->NextEntryOffset)break ;
@@ -493,6 +558,7 @@ clean_shutdown:int fsmonitor_fs_listen__ctor (struct fsmonitor_daemon_state *state) {struct fsmonitor_daemon_backend_data *data; +char shortname[16 ];/* a padded 8.3 buffer */ CALLOC_ARRAY (data,1 );
@@ -523,6 +589,52 @@ int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state) data->nr_listener_handles++; } +/* +* NEEDSWORK: Properly handle 8.3 shortnames. RDCW events can +* contain a shortname (if another application uses a +* shortname in a system call). We care about aliasing and +* the use of shortnames for: +* +* (1) ".git", +* -- if an external process deletes ".git" using "GIT~1", +* we need to catch that and shutdown. +* +* (2) our cookie files, +* -- if an external process deletes one of our cookie +* files using a shortname, we will get a shortname +* event for it. However, we should have already +* gotten a longname event for it when we created the +* cookie, so we can safely discard the shortname +* events for cookie files. +* +* (3) the spelling of modified files that we report to clients. +* -- we need to report the longname to the client because +* that is what they are expecting. Presumably, the +* client is going to lookup the paths that we report +* in their index and untracked-cache, so we should +* normalize the data for them. (Technically, they +* could adapt, so we could relax this maybe.) +* +* FOR NOW, while our CWD is at the root of the worktree we +* can easily get the spelling of the shortname of ".git" (if +* the volume has shortnames enabled). For most worktrees +* this value will be "GIT~1", but we don't want to assume +* that. +* +* Capture this so that we can handle (1). +* +* We leave (3) for a future effort. +*/ +strbuf_init (&data->dot_git_shortname,0 ); +GetShortPathNameA (".git" , shortname,sizeof (shortname)); +if (!strcmp (".git" , shortname)) +trace_printf_key (&trace_fsmonitor,"No shortname for '.git'" ); +else { +trace_printf_key (&trace_fsmonitor, +"Shortname of '.git' is '%s'" , shortname); +strbuf_addstr (&data->dot_git_shortname, shortname); + } + state->backend_data = data;return 0 ;
t/t7527-builtin-fsmonitor.sh +65 -0
71 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH v3 34/34 t7527: test FS event reporing on MacOS WRT case and Unicode
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Confirm that MacOS FS events are reported with a normalized spelling.
APFS (and/or HFS+) is case-insensitive. This means that case-independent lookups ( [ -d .git ] and [ -d .GIT ] ) should both succeed. But that doesn't tell us how FS events are reported if we try "rm -rf .git" versus "rm -rf .GIT". Are the events reported using the on-disk spelling of the pathname or in the spelling used by the command.
NEEDSWORK: I was only able to test case. It would be nice to add tests that use different Unicode spellings/normalizations and understand the differences between APFS and HFS+ in this area. We should confirm that the spelling of the workdir paths that the daemon sends to clients are always properly normalized.
--- t/t7527-builtin-fsmonitor.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
t/t7527-builtin-fsmonitor.sh +30 -0
@@ -178,6 +178,36 @@ test_expect_success MINGW,SHORTNAMES 'implicit daemon stop (rename GIT~2)' ' test_must_fail git -C test_implicit_1s2 fsmonitor--daemon status ' +# Confirm that MacOS hides all of the Unicode normalization and/or +# case folding from the FS events. That is, are the pathnames in the +# FS events reported using the spelling on the disk or in the spelling +# used by the other process. +# +# Note that we assume that the filesystem is set to case insensitive. +# +# NEEDSWORK: APFS handles Unicode and Unicode normalization +# differently than HFS+. I only have an APFS partition, so +# more testing here would be helpful. +# + +# Rename .git using alternate spelling and confirm that the daemon +# sees the event using the correct spelling and shutdown. +test_expect_success UTF8_NFD_TO_NFC 'MacOS event spelling (rename .GIT)' ' + test_when_finished "stop_daemon_delete_repo test_apfs" && + + git init test_apfs && + start_daemon test_apfs && + + [ -d test_apfs/.git ] && + [ -d test_apfs/.GIT ] && + + mv test_apfs/.GIT test_apfs/.FOO && + sleep 1 && + mv test_apfs/.FOO test_apfs/.git && + + test_must_fail git -C test_apfs fsmonitor--daemon status +' + test_expect_success 'cannot start multiple daemons' ' test_when_finished "stop_daemon_delete_repo test_multiple" &&
Signature · 1 line
gitgitgadget
Re: [PATCH v3 34/34] t7527: test FS event reporing on MacOS WRT case and Unicode
t/t7527-builtin-fsmonitor.sh test_expect_success MINGW,SHORTNAMES 'implicit daemon stop (rename GIT~2)' ' · line 202
Quoted 2 lines · Jeff Hostetler via GitGitGadget
t/t7527-builtin-fsmonitor.sh +2
-0+ [ -d test_apfs/.git ] && + [ -d test_apfs/.GIT ] &&
Better as "test_path_is_dir".
Quoted 10 lines · Jeff Hostetler via GitGitGadget
Here is V3 of my patch series to add a builtin FSMonitor daemon to Git. I rebased this series onto v2.32.0.
V3 addresses most of the previous review comments and things we've learned from our experimental testing of V2. (A version of V2 was shipped as an experimental feature in the v2.32.0-based releases of Git for Windows and VFS for Git.)
There are still a few items that I need to address, but that list is getting very short.
...
Quoted 2 lines
fsmonitor-fs-listen-win32: stub in backend for Windows
fsmonitor-fs-listen-macos: stub in backend for MacOS
I left some light comments on the repo-settings.c part of this to follow up from a previous round.
Any other testing of it is stalled by there being no linux backend for it as part of this series. I see from spelunking repos that Johannes had a WIP compat/fsmonitor/linux.c which looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.
I'd really prefer for git not to have features that place free platforms at a disadvantage against proprietary platforms if it can be avoided, and in this case the lack of a Linux backend also means much less widespread testing of the feature among the development community / CI.
Quoted 14 lines · Ævar Arnfjörð Bjarmason
Quoted 10 lines · Jeff Hostetler via GitGitGadget
Here is V3 of my patch series to add a builtin FSMonitor daemon to Git. I rebased this series onto v2.32.0.
V3 addresses most of the previous review comments and things we've learned from our experimental testing of V2. (A version of V2 was shipped as an experimental feature in the v2.32.0-based releases of Git for Windows and VFS for Git.)
There are still a few items that I need to address, but that list is getting very short.
Quoted 6 lines
...
Quoted 2 lines
fsmonitor-fs-listen-win32: stub in backend for Windows
fsmonitor-fs-listen-macos: stub in backend for MacOSI left some light comments on the
repo-settings.cpart of this to follow up from a previous round.
Thanks.
Quoted 6 lines
Any other testing of it is stalled by there being no linux backend for it as part of this series. I see from spelunking repos that Johannes had a WIP
compat/fsmonitor/linux.cwhich looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.
The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.
Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.
I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.
Quoted 6 lines
I'd really prefer for git not to have features that place free platforms at a disadvantage against proprietary platforms if it can be avoided, and in this case the lack of a Linux backend also means much less widespread testing of the feature among the development community / CI.
This feature is always going to have platform-specific components, so the lack of one platform or another should not stop us from discussing it for the platforms that can be supported.
And given the size and complexity of the platform-specific code, we should not assume that "just test it on Linux" is sufficient. Yes, there are some common/shared routines/data structures in the daemon, but hard/tricky parts are in the platform layer.
Jeff
Quoted 35 lines · Jeff Hostetler
Quoted 10 lines · Ævar Arnfjörð Bjarmason
Quoted 8 lines · Jeff Hostetler via GitGitGadget
Here is V3 of my patch series to add a builtin FSMonitor daemon to Git. I rebased this series onto v2.32.0.
V3 addresses most of the previous review comments and things we've learned from our experimental testing of V2. (A version of V2 was shipped as an experimental feature in the v2.32.0-based releases of Git for Windows and VFS for Git.)
Quoted 8 lines
Quoted 2 lines
There are still a few items that I need to address, but that list is getting very short.
...
Quoted 2 lines
fsmonitor-fs-listen-win32: stub in backend for Windows
fsmonitor-fs-listen-macos: stub in backend for MacOSI left some light comments on the
repo-settings.cpart of this to follow
up from a previous round.Thanks.
Quoted 6 lines
Any other testing of it is stalled by there being no linux backend for
it as part of this series. I see from spelunking repos that Johannes had a WIPcompat/fsmonitor/linux.cwhich looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.
Quoted 8 lines
Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.
I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.
That default limit is a bit annoying, but I don't see how it's a blocker in any way.
You simply adjust the limit. E.g. I deployed and tested the hook version of inotify (using watchman) in a sizable development environment, and written my own code using the API. This was all before fanotify(7) existed. IIRC I set most of the limits to 2^24 or 2^20. I've used it with really large Git repos, including with David Turner's 2015-04-03-1M-git for testing (git ls-files | wc -l on that is around a quarter-million).
If you have a humongous repository and don't have root on your own machine you're out of luck. But I think most people who'd use this are either using their own laptop, or e.g. in a corporate setting where administrators would tweak the sysctl limits given the performance advantages (as I did).
Once you adjust the limits Linux deals with large trees just fine, it just has overly conservative limits for most things in sysctl. The API is a bit annoying, your event loop needs to run around and add watches.
AFAICT you need Linux 5.1 for fanotify(7) to be useful, e.g. Debian stable, RHEL etc. aren't using anything that new. So having an inotify backend as well as possibly a fanotify one would be very useful.
And linux.git's docs suggest that the default limit was bumped from 8192 to 1M in v5.10, a really recent kernel, so if you've got that you've also got fanotify.
In any case, even if Linux's inotify limit was something hardcoded and impossible to change you could still use such an API to test & debug the basics of this feature on that platform.
Quoted 10 lines
Quoted 6 lines
I'd really prefer for git not to have features that place free platforms
at a disadvantage against proprietary platforms if it can be avoided, and in this case the lack of a Linux backend also means much less widespread testing of the feature among the development community / CI.This feature is always going to have platform-specific components, so the lack of one platform or another should not stop us from discussing it for the platforms that can be supported.
(I think per the above that's s/can be/are/)
Quoted 4 lines
And given the size and complexity of the platform-specific code, we should not assume that "just test it on Linux" is sufficient. Yes, there are some common/shared routines/data structures in the daemon, but hard/tricky parts are in the platform layer.
I think we're talking past each other a bit here. I'm not saying that you can get full or meaningful testing for it on Windows if you test it on Linux, or the other way around.
Of course there's platform-specific stuff, although there's also a lot of non-platform-specific stuff, so even a very basic implementation of inotify would make reviwing this easier / give access to more reviewers.
I'm saying that I prefer that Git as a free software project not be in the situation of saying the best use-case for a given size/shape of repo is to use Git in combination with proprietary operating systems over freely licensed ones.
IOW what the FSF has a policy for GNU projects. Now, I think the FSF probably goes too far in that (famously removing rather obscure font rendering features from Emacs on OSX), but "manage lots of data" is a core feature of git.
Quoted 72 lines · Ævar Arnfjörð Bjarmason
Quoted 35 lines · Jeff Hostetler
Quoted 10 lines · Ævar Arnfjörð Bjarmason
Quoted 8 lines · Jeff Hostetler via GitGitGadget
Here is V3 of my patch series to add a builtin FSMonitor daemon to Git. I rebased this series onto v2.32.0.
V3 addresses most of the previous review comments and things we've learned from our experimental testing of V2. (A version of V2 was shipped as an experimental feature in the v2.32.0-based releases of Git for Windows and VFS for Git.)
Quoted 8 lines
Quoted 2 lines
There are still a few items that I need to address, but that list is getting very short.
...
Quoted 2 lines
fsmonitor-fs-listen-win32: stub in backend for Windows
fsmonitor-fs-listen-macos: stub in backend for MacOSI left some light comments on the
repo-settings.cpart of this to follow
up from a previous round.Thanks.
Quoted 6 lines
Any other testing of it is stalled by there being no linux backend for
it as part of this series. I see from spelunking repos that Johannes had a WIPcompat/fsmonitor/linux.cwhich looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.
Quoted 8 lines
Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.
I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.
That default limit is a bit annoying, but I don't see how it's a blocker in any way.
You simply adjust the limit. E.g. I deployed and tested the hook version of inotify (using watchman) in a sizable development environment, and written my own code using the API. This was all before fanotify(7) existed. IIRC I set most of the limits to 2^24 or 2^20. I've used it with really large Git repos, including with David Turner's 2015-04-03-1M-git for testing (
git ls-files | wc -lon that is around a quarter-million).If you have a humongous repository and don't have root on your own machine you're out of luck. But I think most people who'd use this are either using their own laptop, or e.g. in a corporate setting where administrators would tweak the sysctl limits given the performance advantages (as I did).
Once you adjust the limits Linux deals with large trees just fine, it just has overly conservative limits for most things in sysctl. The API is a bit annoying, your event loop needs to run around and add watches.
AFAICT you need Linux 5.1 for fanotify(7) to be useful, e.g. Debian stable, RHEL etc. aren't using anything that new. So having an inotify backend as well as possibly a fanotify one would be very useful.
Quoted 7 lines
And linux.git's docs suggest that the default limit was bumped from 8192 to 1M in v5.10, a really recent kernel, so if you've got that you've also got fanotify.
In any case, even if Linux's inotify limit was something hardcoded and impossible to change you could still use such an API to test & debug the basics of this feature on that platform.
Good points. If the inotify limits can be increased into the millions then we can revisit supporting it. I do worry about possible race conditions as we have to scan the worktree and add/delete a watch for each directory, but we don't need to worry about that right now.
I do want to have Linux support eventually, but I was saving it for a later effort (and/or looking for volunteers). My IPC-based builtin daemon complements the existing hook-based fsmonitor support that Ben Peart and Kevin Willford added a few years ago. That model (and PERL hook script and Watchman integration) work fine for Linux, so the advantages of a builtin monitor aren't as compelling.
For example, on Linux, hook process creation is fast, PERL is fast, and it is easy to just apt-get a tool like Watchman. But on Windows, process creation is slow, NTFS is slow, PERL is available as part of the Git-for-Windows distribution, and installing third-party tools like Watchman onto a collection of enterprise users' machines is a chore, so it made sense of us to pick platforms where the existing hook model has issues and add other platforms later.
Besides, this patch series is already at 34 commits. And some of them are quite large. Adding another platform would just make it even larger.
Right now I'm more interested in the larger question of whether we WANT to have a builtin fsmonitor and do we like the overall design of what I have here? Picking up a new platform, whether it is Linux, or AIX, or Solaris, or Nonstop, or whatever, should nicely fit into one platform-specific file in compat/fsmonitor and not take that long.
Quoted 23 lines
Quoted 11 lines
Quoted 7 lines
I'd really prefer for git not to have features that place free platforms
at a disadvantage against proprietary platforms if it can be avoided, and in this case the lack of a Linux backend also means much less widespread testing of the feature among the development community / CI.This feature is always going to have platform-specific components, so the lack of one platform or another should not stop us from discussing it for the platforms that can be supported.
(I think per the above that's s/can be/are/)
Quoted 4 lines
And given the size and complexity of the platform-specific code, we should not assume that "just test it on Linux" is sufficient. Yes, there are some common/shared routines/data structures in the daemon, but hard/tricky parts are in the platform layer.
I think we're talking past each other a bit here. I'm not saying that you can get full or meaningful testing for it on Windows if you test it on Linux, or the other way around.
Quoted 8 lines
Of course there's platform-specific stuff, although there's also a lot of non-platform-specific stuff, so even a very basic implementation of inotify would make reviwing this easier / give access to more reviewers.
I'm saying that I prefer that Git as a free software project not be in the situation of saying the best use-case for a given size/shape of repo is to use Git in combination with proprietary operating systems over freely licensed ones.
I wouldn't worry about that. Even without Watchman integration, Linux runs things so much faster than anything else it's not funny.
If anything, we need things like fsmonitor on Windows to help keep up with Linux.
Quoted 6 lines
IOW what the FSF has a policy for GNU projects. Now, I think the FSF probably goes too far in that (famously removing rather obscure font rendering features from Emacs on OSX), but "manage lots of data" is a core feature of git.
Jeff
Quoted 78 lines · Jeff Hostetler
Quoted 68 lines · Ævar Arnfjörð Bjarmason
Quoted 35 lines · Jeff Hostetler
Quoted 10 lines · Ævar Arnfjörð Bjarmason
Quoted 8 lines · Jeff Hostetler via GitGitGadget
Here is V3 of my patch series to add a builtin FSMonitor daemon to Git. I rebased this series onto v2.32.0.
V3 addresses most of the previous review comments and things we've learned from our experimental testing of V2. (A version of V2 was shipped as an experimental feature in the v2.32.0-based releases of Git for Windows and VFS for Git.)
Quoted 8 lines
Quoted 2 lines
There are still a few items that I need to address, but that list is getting very short.
...
Quoted 2 lines
fsmonitor-fs-listen-win32: stub in backend for Windows
fsmonitor-fs-listen-macos: stub in backend for MacOSI left some light comments on the
repo-settings.cpart of this to follow
up from a previous round.Thanks.
Quoted 6 lines
Any other testing of it is stalled by there being no linux backend for
it as part of this series. I see from spelunking repos that Johannes had a WIPcompat/fsmonitor/linux.cwhich looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.
Quoted 8 lines
Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.
I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.
That default limit is a bit annoying, but I don't see how it's a blocker
in any way.
You simply adjust the limit. E.g. I deployed and tested the hook version
of inotify (using watchman) in a sizable development environment, and written my own code using the API. This was all before fanotify(7) existed. IIRC I set most of the limits to 2^24 or 2^20. I've used it with really large Git repos, including with David Turner's 2015-04-03-1M-git for testing (git ls-files | wc -lon that is around a quarter-million).
If you have a humongous repository and don't have root on your own machine you're out of luck. But I think most people who'd use this are either using their own laptop, or e.g. in a corporate setting where administrators would tweak the sysctl limits given the performance advantages (as I did).
Once you adjust the limits Linux deals with large trees just fine, it
just has overly conservative limits for most things in sysctl. The API is a bit annoying, your event loop needs to run around and add watches. AFAICT you need Linux 5.1 for fanotify(7) to be useful, e.g. Debian stable, RHEL etc. aren't using anything that new. So having an inotify backend as well as possibly a fanotify one would be very useful.Quoted 8 lines
And linux.git's docs suggest that the default limit was bumped from 8192
to 1M in v5.10, a really recent kernel, so if you've got that you've also got fanotify.
In any case, even if Linux's inotify limit was something hardcoded and
impossible to change you could still use such an API to test & debug the basics of this feature on that platform.
Quoted 2 lines
Good points. If the inotify limits can be increased into the millions then we can revisit supporting it.
I also replied in the side-thread on this topic:
https://lore.kernel.org/git/[email protected]/
Quoted 3 lines
[...] I do worry about possible race
conditions as we have to scan the worktree and add/delete a watch for each directory, but we don't need to worry about that right now.
In the watchman code there's a scheduleRecrawl() in such cases, but it's not just something that happens on Linux.
On Windows it's if ReadDirectoryChangesW() returns ERROR_NOTIFY_ENUM_DIR. This builtin version doesn't check that at all. Here's someone who seems to work at MSFT talking about it:
https://devblogs.microsoft.com/oldnewthing/20110812-00/?p=9913
So isn't this an issue on Windows as well?
Quoted 2 lines
I do want to have Linux support eventually, but I was saving it for a later effort (and/or looking for volunteers).
OK, upthread you said it was a "non-starter" because of the adjustable limits we've been discussing...
Quoted 7 lines
My IPC-based builtin daemon complements the existing hook-based fsmonitor support that Ben Peart and Kevin Willford added a few years ago. That model (and PERL hook script and Watchman integration) work fine for Linux, so the advantages of a builtin monitor aren't as compelling.
For example, on Linux, hook process creation is fast, PERL is fast,[...]
Did you or Ben ever try to reproduce what I noted at:
https://lore.kernel.org/git/CACBZZX5e58bWuf3NdDYTxu2KyZj29hHONzN=rp-7vXd8nURyWQ@mail.gmail.com/
And more recently with reference to that at:
https://lore.kernel.org/git/[email protected]/
I.e. when I tested it back then the issue wasn't Perl's performance, it was something between watchman and the user, happening in git. I.e. watchman would return in the single-digit milliseconds, git would take hundreds or thousands of ms.
Maybe I missed some intervening analysis, but this didn't have to do with process overhead back then.
Also, watchman has a JSON network interface. So for a "native" solution I also wonder if you or Ben tried a solution that involved just talking to that over the local network. The hook/perl script was just a trivial aid to make doing that easier. Wouldn't that be just as fast (or near enough) on Windows?
Ben mentioned in the linked thread that there was some talks between Microsoft (or well, the "Git team") and the watchman people. So again, I missed some in-between discussions. I'm just wondering about the design choices...
Quoted 6 lines
and it is easy to just apt-get a tool like Watchman. But on Windows, process creation is slow, NTFS is slow, PERL is available as part of the Git-for-Windows distribution, and installing third-party tools like Watchman onto a collection of enterprise users' machines is a chore, so it made sense of us to pick platforms where the existing hook model has issues and add other platforms later.
If we're trying to do an end-run around package systems being a pain would including the relevant part of watchman in contrib + talking to it over its network interface be a replacement for some/most/all of this series?
Quoted 10 lines
Besides, this patch series is already at 34 commits. And some of them are quite large. Adding another platform would just make it even larger.
Right now I'm more interested in the larger question of whether we WANT to have a builtin fsmonitor and do we like the overall design of what I have here? Picking up a new platform, whether it is Linux, or AIX, or Solaris, or Nonstop, or whatever, should nicely fit into one platform-specific file in compat/fsmonitor and not take that long.
Sure, FWIW I'm not opposed to this being a built-in per-se, or us re-implementing parts of watchman here, and not being able to test this series meaningfully a lot of these questions are probably easy to answer on a supported OS...
Quoted 30 lines
Quoted 16 lines
Quoted 10 lines
Quoted 6 lines
I'd really prefer for git not to have features that place free platforms
at a disadvantage against proprietary platforms if it can be avoided, and in this case the lack of a Linux backend also means much less widespread testing of the feature among the development community / CI.This feature is always going to have platform-specific components, so the lack of one platform or another should not stop us from discussing it for the platforms that can be supported.
(I think per the above that's s/can be/are/)
Quoted 4 lines
And given the size and complexity of the platform-specific code, we should not assume that "just test it on Linux" is sufficient. Yes, there are some common/shared routines/data structures in the daemon, but hard/tricky parts are in the platform layer.
Quoted 13 lines
I think we're talking past each other a bit here. I'm not saying that
you can get full or meaningful testing for it on Windows if you test it on Linux, or the other way around.
Of course there's platform-specific stuff, although there's also a lot
of non-platform-specific stuff, so even a very basic implementation of inotify would make reviwing this easier / give access to more reviewers. I'm saying that I prefer that Git as a free software project not be in
the situation of saying the best use-case for a given size/shape of repo is to use Git in combination with proprietary operating systems over freely licensed ones.
Quoted 5 lines
I wouldn't worry about that. Even without Watchman integration, Linux runs things so much faster than anything else it's not funny.
If anything, we need things like fsmonitor on Windows to help keep up with Linux.
Even on Linux if you do a "git status" on a humongous repository it can take 1-2 seconds (see old but still valid linked numbers above), whereas if you've been logging fs events and ask a daemon "what changed since time xyz" it can take <5ms.
But that's just been on repositories I've tested. I'd assumed that this part of GVFS wasn't something that could have been fairly easily replaced by having the relevant developers run a Linux distro in vmware or whatever, adn that's been consistent with my own testing on still-big-but-smaller repos than that.
Hi Ævar,
Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Jeff Hostetler
Quoted 5 lines · Ævar Arnfjörð Bjarmason
Any other testing of it is stalled by there being no linux backend for it as part of this series. I see from spelunking repos that Johannes had a WIP
compat/fsmonitor/linux.cwhich looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.
I am a bit surprised that you gave up so easily, it cannot be that hard if you use git rebase -i smartly.
But I think there is an even bigger obstacle lurking than just the challenge of rebasing that experimental backend.
Quoted 11 lines
Quoted 11 lines
The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.
Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.
Quoted 6 lines
Quoted 3 lines
I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.
That default limit is a bit annoying, but I don't see how it's a blocker in any way.
Let me help you to see it.
So let's assume that you start FSMonitor-enabled Git, with the default values. What is going to happen if you have any decently-sized worktree? You run out of handles. What then? Throw your hands in the air? Stop working? Report incorrect results?
Those are real design challenges, and together with the race problems Jeff mentioned, they pose a much bigger obstacle than the rebasing you mentioned above.
Quoted 8 lines
You simply adjust the limit. E.g. I deployed and tested the hook version of inotify (using watchman) in a sizable development environment, and written my own code using the API. This was all before fanotify(7) existed. IIRC I set most of the limits to 2^24 or 2^20. I've used it with really large Git repos, including with David Turner's 2015-04-03-1M-git for testing (
git ls-files | wc -lon that is around a quarter-million).
Quoted 5 lines
If you have a humongous repository and don't have root on your own machine you're out of luck. But I think most people who'd use this are either using their own laptop, or e.g. in a corporate setting where administrators would tweak the sysctl limits given the performance advantages (as I did).
This conjecture that most people who'd use this are using their own laptop or have a corporate setting where administrators would tweak the sysctl limits according to engineers' wishes strikes me as totally made up from thin air, nothing else.
In other words, I find it an incredibly unconvincing argument.
I prefer not to address the rest of your mail, as I found it not only a lengthy tangent (basically trying to talk Jeff into adding Linux support in what could have been a much shorter mail), but actively distracting from the already long patch series Jeff presented. It is so long, in fact, that we had to put in an exemption in GitGitGadget because it is already longer than a usually-unreasonable 30 patches. Also, at this point, insisting on Linux support (in so many words) is unhelpful.
Let me summarize why I think this is unhelpful: In Git, it is our tradition to develop incrementally, for better or worse. Jeff's effort brought us to a point where we already have Windows and macOS support, i.e. support for the most prevalent development platforms (see e.g.
https://insights.stackoverflow.com/survey/2020#technology-developers-primary-operating-systems).
We already established multiple obstacles for Linux support, therefore demanding Linux support to be included Right Now would increase the patch series even further, making it even less reviewable, being even less incremental, hold up the current known-to-work-well state, force Jeff to work on something he probably cannot work on right now, and therefore delaying the entire effort even further.
Ciao,
Johannes
Quoted 26 lines · Johannes Schindelin
Quoted 3 lines · Ævar Arnfjörð Bjarmason
Quoted 1 line · Jeff Hostetler
On 7/1/21 1:40 PM, Ævar Arnfjörð Bjarmason wrote:
[...]
Quoted 11 lines
Quoted 11 lines
The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.
Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.
Quoted 6 lines
Quoted 3 lines
I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.
That default limit is a bit annoying, but I don't see how it's a blocker in any way.
Let me help you to see it.
Quoted 8 lines
So let's assume that you start FSMonitor-enabled Git, with the default values. What is going to happen if you have any decently-sized worktree? You run out of handles. What then? Throw your hands in the air? Stop working? Report incorrect results?
Those are real design challenges, and together with the race problems Jeff mentioned, they pose a much bigger obstacle than the rebasing you mentioned above.
You report an error and tell the user to raise the limit, and cover this in your install docs. It's what watchman does:
https://github.com/facebook/watchman/blob/master/watchman/error_category.cpp#L28-L45
https://facebook.github.io/watchman/docs/install.html#system-specific-preparation
Quoted 14 lines
Quoted 8 lines
You simply adjust the limit. E.g. I deployed and tested the hook version of inotify (using watchman) in a sizable development environment, and written my own code using the API. This was all before fanotify(7) existed. IIRC I set most of the limits to 2^24 or 2^20. I've used it with really large Git repos, including with David Turner's 2015-04-03-1M-git for testing (
git ls-files | wc -lon that is around a quarter-million).Quoted 5 lines
If you have a humongous repository and don't have root on your own machine you're out of luck. But I think most people who'd use this are either using their own laptop, or e.g. in a corporate setting where administrators would tweak the sysctl limits given the performance advantages (as I did).
Quoted 6 lines
This conjecture that most people who'd use this are using their own laptop or have a corporate setting where administrators would tweak the sysctl limits according to engineers' wishes strikes me as totally made up from thin air, nothing else.
In other words, I find it an incredibly unconvincing argument.
It's from a sample size of one experience of deploying this in a BigCorp setting.
But sure, perhaps things are done differently where you work/have worked. My experience is that even if you're dealing with some BOFHs and e.g. are using shared racked development servers it's generally not an insurmountable task to get some useful software installed, or some system configuration tweaked.
In this case we're talking about ~40MB of kernel memory for 1 million dirs IIRC, that coupled with the target audience that benefits most from this probably being deployments that are *painfully* aware of their "git status" slowness...
Quoted 7 lines
I prefer not to address the rest of your mail, as I found it not only a lengthy tangent (basically trying to talk Jeff into adding Linux support in what could have been a much shorter mail), but actively distracting from the already long patch series Jeff presented. It is so long, in fact, that we had to put in an exemption in GitGitGadget because it is already longer than a usually-unreasonable 30 patches. Also, at this point, insisting on Linux support (in so many words) is unhelpful.
This part of the tread started because Jeff H. claimed upthread:
[...]inotify was a non-starter, so I removed the Linux version from our patch series
But after I noted that it works just fine, you just need to change some sysctl limits.
It seems at this point we're debating whether some installations of Linux have BOFH-y enough administrators that they won't tweak sysctl limits for you. OK, but given that I've run this thing in a production setting it's clearly not a "non-starter". I think it could be useful for a lot of users.
I'll reply with more (and hopefully helpful) specifics to Jeff's mail.
Quoted 11 lines
Let me summarize why I think this is unhelpful: In Git, it is our tradition to develop incrementally, for better or worse. Jeff's effort brought us to a point where we already have Windows and macOS support, i.e. support for the most prevalent development platforms (see e.g.
https://insights.stackoverflow.com/survey/2020#technology-developers-primary-operating-systems).
We already established multiple obstacles for Linux support, therefore demanding Linux support to be included Right Now would increase the patch series even further, making it even less reviewable, being even less incremental, hold up the current known-to-work-well state, force Jeff to work on something he probably cannot work on right now, and therefore delaying the entire effort even further.
I think we just disagree. I wouldn't call my opinion "unhelpful" any more than yours.
I don't think Git's ever had anything like a major feature (built in, config settings, etc. etc.) contributed by a propriterary OS vendor that works on that vendor's OS, as well as another vendor's propriterary OS, but not comparable free systems
Is that less incremental and perhaps less practical? Sure. It's not an entirely practical viewpoint. I work on free software partly for idealistic reasons. I'd prefer if the project I'm working on doesn't give users a carrot to pick proprietary systems over free ones.
But ultimately it's not my call, but Junio's.
Hi Ævar,
Quoted 2 lines · Ævar Arnfjörð Bjarmason
I think we just disagree. I wouldn't call my opinion "unhelpful" any more than yours.
You still misunderstand. This is not about any "opinion" of yours, it is about your delay tactics to make it deliberately difficult to finish this patch series, by raising the bar beyond what is reasonable for a single patch series.
And you keep doing it. I would appreciate if you just stopped with all those tangents and long and many replies that do not seem designed to help the patch series stabilize, but do the opposite.
Ciao,
Johannes
Quoted 8 lines · Johannes Schindelin
Quoted 2 lines · Ævar Arnfjörð Bjarmason
I think we just disagree. I wouldn't call my opinion "unhelpful" any more than yours.
You still misunderstand. This is not about any "opinion" of yours, it is about your delay tactics to make it deliberately difficult to finish this patch series,
The whole job of a reviewer is to *delay* the inclusion of the patch series being reviewed until it has passed his/her personal standards.
Ævar is just doing his job (metaphorically).
Quoted 2 lines
by raising the bar beyond what is reasonable for a single patch series.
It is not up to you to determine what is reasonable for a patch series, and given that you have a vested interest you are also not an objective observer.
I have hundreds of patches pending review, and I would love for anyone to try find issues with them, even if I ultimately disagreed with their assessment.
The Git project doesn't owe your patches any preferential treatment. The review process will take as long as the review process takes. You can't put deadlines on open source projects.
Signature · 1 line
Felipe Contreras
Quoted 2 lines · Johannes Schindelin
In Git, it is our tradition to develop incrementally, for better or worse.
We develop incrementally, but the first version of a big feature has to have as many eyes on it as possible in order to track potential issues in the future.
It is very common to look back at an API and say "oh, if only wad thought of that back then". Therefore we should do our best to think about it now.
We want to avoid future aw-shucks.
Quoted 10 lines
Jeff's effort
brought us to a point where we already have Windows and macOS support, i.e. support for the most prevalent development platforms (see e.g.
https://insights.stackoverflow.com/survey/2020#technology-developers-primary-operating-systems).
We already established multiple obstacles for Linux support, therefore demanding Linux support to be included Right Now would increase the patch series even further, making it even less reviewable, being even less incremental, hold up the current known-to-work-well state, force Jeff to work on something he probably cannot work on right now, and therefore delaying the entire effort even further.
This is a red herring.
You don't need to send the Linux support as part of the patch series, you can simply provide a branch for the people that want to give it a try.
Even if it's not ready for wide use, even if it needs a specific version of Linux, even if it's a proof of concept, you can provide it.
The real reason it's not provided is laziness (this is not an attack, laziness is good trait in a programmer, although not always).
Cheers.
Signature · 1 line
Felipe Contreras