Builtin FSMonitor Part 2.5
PATCH 1/16 t/test-lib: avoid using git on LHS of pipe
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! help: include fsmonitor--daemon feature flag in version info
--- t/test-lib.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
t/test-lib.sh +2 -1
@@ -1803,5 +1803,6 @@ GIT_TEST_MAINT_SCHEDULER="none:exit 1" # Does this platform support `git fsmonitor--daemon` # test_lazy_prereq FSMONITOR_DAEMON ' - git version --build-options | grep "feature:" | grep "fsmonitor--daemon" + git version --build-options >output && + grep "feature: fsmonitor--daemon" output '
Signature · 1 line
gitgitgadget
PATCH 5/16 fsmonitor--daemon: refactor cookie handling for readability
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! fsmonitor--daemon: use a cookie file to sync with file system
Use implicit definitions for FCIR_ enum values.
Remove const from cookie->name.
Reverse if then and else branches around open() to ease readability.
Document that we don't care about errors from close() and unlink().
--- builtin/fsmonitor--daemon.c | 53 +++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 23 deletions(-)
builtin/fsmonitor--daemon.c +30 -23
@@ -109,14 +109,14 @@ static int do_as_client__status(void)enum fsmonitor_cookie_item_result {FCIR_ERROR =-1 ,/* could not create cookie file ? */ -FCIR_INIT =0 , +FCIR_INIT ,FCIR_SEEN ,FCIR_ABORT , };struct fsmonitor_cookie_item {struct hashmap_entry entry; -const char *name; +char *name;enum fsmonitor_cookie_item_result result; };
@@ -166,37 +166,44 @@ static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie(* 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); - -/* -* Technically, 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, but we -* don't have that routine in our thread-utils. -* -* After extensive beta testing I'm not really worried about -* this. Also note that the above open() and unlink() calls -* will cause at least two FS events on that path, so the odds -* of getting stuck are pretty slim. -*/ -while (cookie->result ==FCIR_INIT ) -pthread_cond_wait (&state->cookies_cond, - &state->main_lock); - }else { +if (fd <0 ) {error_errno (_ ("could not create fsmonitor cookie '%s'" ), cookie->name); cookie->result =FCIR_ERROR ; + goto done; } +/* +* Technically, close() and unlink() can fail, but we don't +* care here. We only created the file to trigger a watch +* event from the FS to know that when we're up to date. +*/ +close (fd); +unlink (cookie_pathname.buf); + +/* +* Technically, 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, but we +* don't have that routine in our thread-utils. +* +* After extensive beta testing I'm not really worried about +* this. Also note that the above open() and unlink() calls +* will cause at least two FS events on that path, so the odds +* of getting stuck are pretty slim. +*/ +while (cookie->result ==FCIR_INIT ) +pthread_cond_wait (&state->cookies_cond, + &state->main_lock); + +done:hashmap_remove (&state->cookies, &cookie->entry,NULL ); result = cookie->result; -free ((char *)cookie->name); +free (cookie->name);free (cookie);strbuf_release (&cookie_pathname);
Signature · 1 line
gitgitgadget
PATCH 2/16 update-index: convert advise() messages back to warning()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! update-index: convert fsmonitor warnings to advise
--- builtin/update-index.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
builtin/update-index.c +6 -6
@@ -1238,18 +1238,18 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)if (fsmonitor >0 ) {enum fsmonitor_mode fsm_mode =fsm_settings__get_mode (r);if (fsm_mode ==FSMONITOR_MODE_DISABLED ) { -advise (_ ("core.fsmonitor is unset; " -"set it if you really want to " -"enable fsmonitor" )); +warning (_ ("core.fsmonitor is unset; " +"set it if you really want to " +"enable fsmonitor" )); }add_fsmonitor (&the_index);report (_ ("fsmonitor enabled" )); }else if (!fsmonitor) {enum fsmonitor_mode fsm_mode =fsm_settings__get_mode (r);if (fsm_mode >FSMONITOR_MODE_DISABLED ) -advise (_ ("core.fsmonitor is set; " -"remove it if you really want to " -"disable fsmonitor" )); +warning (_ ("core.fsmonitor is set; " +"remove it if you really want to " +"disable fsmonitor" ));remove_fsmonitor (&the_index);report (_ ("fsmonitor disabled" )); }
Signature · 1 line
gitgitgadget
PATCH 10/16 t7527: delete unused verify_status() function
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! t7527: create test for fsmonitor--daemon
Remove unused function verify_status().
--- t/t7527-builtin-fsmonitor.sh | 10 ---------- 1 file changed, 10 deletions(-)
t/t7527-builtin-fsmonitor.sh +0 -10
@@ -303,16 +303,6 @@ directory_to_file () { 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.fsmonitor=false 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
Signature · 1 line
gitgitgadget
PATCH 11/16 fsmonitor-ipc: add _() to calls to die()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! fsmonitor-ipc: create client routines for git-fsmonitor--daemon
Fix translation markings.
--- fsmonitor-ipc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
fsmonitor-ipc.c +2 -2
@@ -152,7 +152,7 @@ int fsmonitor_ipc__send_command(const char *command, state =ipc_client_try_connect (fsmonitor_ipc__get_path (), &options, &connection);if (state !=IPC_STATE__LISTENING ) { -die ("fsmonitor--daemon is not running" ); +die (_ ("fsmonitor--daemon is not running" ));return -1 ; }
@@ -161,7 +161,7 @@ int fsmonitor_ipc__send_command(const char *command,ipc_client_close_connection (connection);if (ret ==-1 ) { -die ("could not send '%s' command to fsmonitor--daemon" , c); +die (_ ("could not send '%s' command to fsmonitor--daemon" ), c);return -1 ; }
Signature · 1 line
gitgitgadget
PATCH 15/16 fsmonitor--daemon: add _() to calls to error()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! fsmonitor--daemon: implement 'start' command
Fixup translation on die().
--- builtin/fsmonitor--daemon.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
builtin/fsmonitor--daemon.c +4 -4
@@ -1381,7 +1381,7 @@ static int try_to_start_background_daemon(void)* immediately exited). */ if (fsmonitor_ipc__get_state () ==IPC_STATE__LISTENING ) -die ("fsmonitor--daemon is already running '%s'" , +die (_ ("fsmonitor--daemon is already running '%s'" ), the_repository->worktree);if (fsmonitor__announce_startup) {
@@ -1411,13 +1411,13 @@ static int try_to_start_background_daemon(void)default :case SBGR_ERROR :case SBGR_CB_ERROR : -return error ("daemon failed to start" ); +return error (_ ("daemon failed to start" ));case SBGR_TIMEOUT : -return error ("daemon not online yet" ); +return error (_ ("daemon not online yet" ));case SBGR_DIED : -return error ("daemon terminated" ); +return error (_ ("daemon terminated" )); } }
Signature · 1 line
gitgitgadget
cover Builtin FSMonitor Part 2.5
I'm calling this series part 2.5. It should be inserted between parts 2 and
3. I'll send a new version of part 3 after I send part 2.5.
This series is a bunch of "fixup!" commits for part 2. It addresses feedback on part 2 that wasn't received until after part 2 had graduated to "next".
I was originally planning to include them at the beginning of part 3, but since there are so many, I thought it would be easier to add a fixup series in the middle. (And since GGG has a limit of 30 commits, I couldn't add them to either existing series without going over the limit.)
Most of the commits deal with translation markings for die/error messages and fixing && chains in the tests. And there were a few readability improvements.
Jeff Hostetler (16):
t/test-lib: avoid using git on LHS of pipe
update-index: convert advise() messages back to warning() compat/fsmonitor/fsm-listen-darwin: split out GCC-specific
declarations
t/helper/fsmonitor-client: cleanup call to parse_options()
fsmonitor--daemon: refactor cookie handling for readability t/perf/p7519: use grep rather than egrep in test t/perf/p7519: cleanup coding style
t7527: add parameters to start_daemon to handle args and subshell
t7527: fix && chaining in matrix_try()
t7527: delete unused verify_status() function
fsmonitor-ipc: add _() to calls to die() compat/fsmonitor/fsm-listen-darwin: add _() to calls to error() compat/fsmonitor/fsm-listen-win32: add _() to calls to error()
fsmonitor--daemon: add _() to calls to die()
fsmonitor--daemon: add _() to calls to error()
fsmonitor-settings: simplify initialization of settings data
builtin/fsmonitor--daemon.c | 63 ++++---- builtin/update-index.c | 12 +- compat/fsmonitor/fsm-darwin-gcc.h | 92 +++++++++++ compat/fsmonitor/fsm-listen-darwin.c | 95 +---------- compat/fsmonitor/fsm-listen-win32.c | 6 +- fsmonitor-ipc.c | 4 +- fsmonitor-settings.c | 3 +- t/helper/test-fsmonitor-client.c | 17 +- t/perf/p7519-fsmonitor.sh | 23 +-- t/t7527-builtin-fsmonitor.sh | 233 +++++++++++++-------------- t/test-lib.sh | 3 +- 11 files changed, 276 insertions(+), 275 deletions(-) create mode 100644 compat/fsmonitor/fsm-darwin-gcc.h
base-commit: 1a9241e1fee9d418e436849853f031329e792192
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1174%2Fjeffhostetler%2Fbuiltin-fsmonitor-part2.5-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1174/jeffhostetler/builtin-fsmonitor-part2.5-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1174
Signature · 1 line
gitgitgadget
PATCH 12/16 compat/fsmonitor/fsm-listen-darwin: add _() to calls to error()
From: Jeff Hostetler <[email protected]>
fixup! compat/fsmonitor/fsm-listen-darwin: implement FSEvent listener \
on MacOS
--- compat/fsmonitor/fsm-listen-darwin.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
compat/fsmonitor/fsm-listen-darwin.c +2 -2
@@ -335,7 +335,7 @@ int fsm_listen__ctor(struct fsmonitor_daemon_state *state)return 0 ; failed: -error ("Unable to create FSEventStream." ); +error (_ ("Unable to create FSEventStream." ));FREE_AND_NULL (state->backend_data);return -1 ;
@@ -383,7 +383,7 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state) data->stream_scheduled =1 ;if (!FSEventStreamStart (data->stream)) { -error ("Failed to start the FSEventStream" ); +error (_ ("Failed to start the FSEventStream" )); goto force_error_stop_without_loop; } data->stream_started =1 ;
Signature · 1 line
gitgitgadget
PATCH 3/16 compat/fsmonitor/fsm-listen-darwin: split out GCC-specific declarations
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! compat/fsmonitor/fsm-listen-darwin: add MacOS header files \
for FSEvent
Split out GCC-specific MacOS declarations into separate file from the clang-specific header file includes to reduce visual noise.
Eliminate unnecessary includes when using clang.
--- compat/fsmonitor/fsm-darwin-gcc.h | 92 ++++++++++++++++++++++++++++ compat/fsmonitor/fsm-listen-darwin.c | 91 +-------------------------- 2 files changed, 93 insertions(+), 90 deletions(-) create mode 100644 compat/fsmonitor/fsm-darwin-gcc.h
compat/fsmonitor/fsm-darwin-gcc.h new file +92 -0
@@ -0,0 +1,92 @@ +#ifndef FSM_DARWIN_GCC_H +#define FSM_DARWIN_GCC_H + +#ifndef __clang__ +/* +* 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); + +#endif /* !clang */ +#endif /* FSM_DARWIN_GCC_H */
compat/fsmonitor/fsm-listen-darwin.c +1 -90
96 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH 6/16 t/perf/p7519: use grep rather than egrep in test
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! t/perf/p7519: speed up test on Windows
Use grep rather than egrep
--- t/perf/p7519-fsmonitor.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/perf/p7519-fsmonitor.sh +1 -1
@@ -218,7 +218,7 @@ test_fsmonitor_suite () { git ls-files | \ head -100000 | \ grep -v \" | \ - egrep -v " ." | \ + grep -v " ." | \ xargs test-tool chmtime -300 && git status '
Signature · 1 line
gitgitgadget
PATCH 7/16 t/perf/p7519: cleanup coding style
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! t/perf/p7519: add fsmonitor--daemon test cases
Add "_hook" to name of shell function to set up for Watchman/hook test runs.
Fix code style of added "if then".
Add body of builtin test to a test_expect_success.
--- t/perf/p7519-fsmonitor.sh | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
t/perf/p7519-fsmonitor.sh +11 -10
@@ -141,7 +141,7 @@ test_expect_success "one time repo setup" ' fi ' -setup_for_fsmonitor () { +setup_for_fsmonitor_hook () { # set INTEGRATION_SCRIPT depending on the environment if test -n "$INTEGRATION_PATH" then
@@ -182,8 +182,7 @@ test_perf_w_drop_caches () { } test_fsmonitor_suite () { - if test -n "$USE_FSMONITOR_DAEMON" - then + if test -n "$USE_FSMONITOR_DAEMON"; then DESC="builtin fsmonitor--daemon" elif test -n "$INTEGRATION_SCRIPT"; then DESC="fsmonitor=$(basename $INTEGRATION_SCRIPT)"
@@ -264,11 +263,11 @@ test_fsmonitor_suite () { trace_start fsmonitor-watchman if test -n "$GIT_PERF_7519_FSMONITOR"; then for INTEGRATION_PATH in $GIT_PERF_7519_FSMONITOR; do - test_expect_success "setup for fsmonitor $INTEGRATION_PATH" 'setup_for_fsmonitor' + test_expect_success "setup for fsmonitor $INTEGRATION_PATH" 'setup_for_fsmonitor_hook' test_fsmonitor_suite done else - test_expect_success "setup for fsmonitor" 'setup_for_fsmonitor' + test_expect_success "setup for fsmonitor hook" 'setup_for_fsmonitor_hook' test_fsmonitor_suite fi
@@ -306,13 +305,15 @@ if test_have_prereq FSMONITOR_DAEMON then USE_FSMONITOR_DAEMON=t - trace_start fsmonitor--daemon--server - git fsmonitor--daemon start + test_expect_success "setup for builtin fsmonitor" ' + trace_start fsmonitor--daemon--server && + git fsmonitor--daemon start && - trace_start fsmonitor--daemon--client + trace_start fsmonitor--daemon--client && - git config core.fsmonitor true - git update-index --fsmonitor + git config core.fsmonitor true && + git update-index --fsmonitor + ' test_fsmonitor_suite
Signature · 1 line
gitgitgadget
PATCH 14/16 fsmonitor--daemon: add _() to calls to die()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! fsmonitor--daemon: implement 'run' command
Fix translation marking on die().
--- builtin/fsmonitor--daemon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/fsmonitor--daemon.c +1 -1
@@ -1326,7 +1326,7 @@ static int try_to_run_foreground_daemon(int detach_console)* common error case. */ if (fsmonitor_ipc__get_state () ==IPC_STATE__LISTENING ) -die ("fsmonitor--daemon is already running '%s'" , +die (_ ("fsmonitor--daemon is already running '%s'" ), the_repository->worktree);if (fsmonitor__announce_startup) {
Signature · 1 line
gitgitgadget
PATCH 9/16 t7527: fix && chaining in matrix_try()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! t7527: test status with untracked-cache and fsmonitor--daemon
--- t/t7527-builtin-fsmonitor.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
t/t7527-builtin-fsmonitor.sh +3 -3
@@ -536,9 +536,9 @@ matrix_clean_up_repo () { } matrix_try () { - uc=$1 - fsm=$2 - fn=$3 + uc=$1 && + fsm=$2 && + fn=$3 && test_expect_success "Matrix[uc:$uc][fsm:$fsm] $fn" ' matrix_clean_up_repo &&
Signature · 1 line
gitgitgadget
PATCH 8/16 t7527: add parameters to start_daemon to handle args and subshell
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! t7527: create test for fsmonitor--daemon
Add parameters to start_daemon() and handle subshell and logging args.
Remove /dev/null from commands.
Fix &&-chaining of functions.
--- t/t7527-builtin-fsmonitor.sh | 217 ++++++++++++++++++----------------- 1 file changed, 111 insertions(+), 106 deletions(-)
t/t7527-builtin-fsmonitor.sh +111 -106
@@ -11,30 +11,85 @@ then fi stop_daemon_delete_repo () { - r=$1 - git -C $r fsmonitor--daemon stop >/dev/null 2>/dev/null + r=$1 && + test_might_fail git -C $r fsmonitor--daemon stop && rm -rf $1 - return 0 } start_daemon () { - case "$#" in - 1) r="-C $1";; - *) r=""; - esac + r="" && + tf="" && + t2="" && + tk="" && - git $r fsmonitor--daemon start || return $? - git $r fsmonitor--daemon status || return $? + while test "$#" -ne 0 + do + case "$1" in + -C) + shift; + test "$#" -ne 0 || + { echo >&2 "error: -C requires arg"; exit 1; } + r="-C $1" + shift + ;; + -tf) + shift; + test "$#" -ne 0 || + { echo >&2 "error: -tf requires arg"; exit 1; } + tf="$1" + shift + ;; + -t2) + shift; + test "$#" -ne 0 || + { echo >&2 "error: -t2 requires arg"; exit 1; } + t2="$1" + shift + ;; + -tk) + shift; + test "$#" -ne 0 || + { echo >&2 "error: -tk requires arg"; exit 1; } + tk="$1" + shift + ;; + *) + echo >&2 "error: unknown option: '$1'" + exit 1 + ;; + esac + done && + + ( + if test ! -z "$tf" + then + GIT_TRACE_FSMONITOR="$tf" + export GIT_TRACE_FSMONITOR + fi && + + if test ! -z "$t2" + then + GIT_TRACE2_PERF="$t2" + export GIT_TRACE2_PERF + fi && + + if test ! -z "$tk" + then + GIT_TEST_FSMONITOR_TOKEN="$tk" + export GIT_TEST_FSMONITOR_TOKEN + fi && - return 0 + git $r fsmonitor--daemon start && + git $r fsmonitor--daemon status + ) } # Is a Trace2 data event present with the given catetory and key? # We do not care what the value is. # have_t2_data_event () { - c=$1 - k=$2 + c=$1 && + k=$2 && grep -e '"event":"data".*"category":"'"$c"'".*"key":"'"$k"'"' }
@@ -43,7 +98,7 @@ 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 && + start_daemon -C test_explicit && git -C test_explicit fsmonitor--daemon stop && test_must_fail git -C test_explicit fsmonitor--daemon status
@@ -63,7 +118,7 @@ test_expect_success 'implicit daemon start' ' # but this test case is only concerned with whether the daemon was # implicitly started.) - GIT_TRACE2_EVENT="$(pwd)/.git/trace" \ + 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 &&
@@ -86,7 +141,7 @@ test_expect_success 'implicit daemon stop (delete .git)' ' git init test_implicit_1 && - start_daemon test_implicit_1 && + start_daemon -C test_implicit_1 && # deleting the .git directory will implicitly stop the daemon. rm -rf test_implicit_1/.git &&
@@ -110,7 +165,7 @@ test_expect_success 'implicit daemon stop (rename .git)' ' git init test_implicit_2 && - start_daemon test_implicit_2 && + start_daemon -C test_implicit_2 && # renaming the .git directory will implicitly stop the daemon. mv test_implicit_2/.git test_implicit_2/.xxx &&
@@ -128,7 +183,7 @@ test_expect_success 'cannot start multiple daemons' ' git init test_multiple && - start_daemon test_multiple && + start_daemon -C test_multiple && test_must_fail git -C test_multiple fsmonitor--daemon start 2>actual && grep "fsmonitor--daemon is already running" actual &&
@@ -177,8 +232,7 @@ test_expect_success 'setup' ' # This is here in case something else fails first. # redundant_stop_daemon () { - git fsmonitor--daemon stop - return 0 + test_might_fail git fsmonitor--daemon stop } test_expect_success 'update-index implicitly starts daemon' '
@@ -186,7 +240,7 @@ test_expect_success 'update-index implicitly starts daemon' ' test_must_fail git fsmonitor--daemon status && - GIT_TRACE2_EVENT="$(pwd)/.git/trace_implicit_1" \ + GIT_TRACE2_EVENT="$PWD/.git/trace_implicit_1" \ git update-index --fsmonitor && git fsmonitor--daemon status &&
@@ -202,7 +256,7 @@ test_expect_success 'status implicitly starts daemon' ' test_must_fail git fsmonitor--daemon status && - GIT_TRACE2_EVENT="$(pwd)/.git/trace_implicit_2" \ + GIT_TRACE2_EVENT="$PWD/.git/trace_implicit_2" \ git status >actual && git fsmonitor--daemon status &&
@@ -214,38 +268,38 @@ test_expect_success 'status implicitly starts daemon' ' ' edit_files () { - echo 1 >modified - echo 2 >dir1/modified - echo 3 >dir2/modified + echo 1 >modified && + echo 2 >dir1/modified && + echo 3 >dir2/modified && >dir1/untracked } delete_files () { - rm -f delete - rm -f dir1/delete + rm -f delete && + rm -f dir1/delete && rm -f dir2/delete } create_files () { - echo 1 >new - echo 2 >dir1/new + echo 1 >new && + echo 2 >dir1/new && echo 3 >dir2/new } rename_files () { - mv rename renamed - mv dir1/rename dir1/renamed + mv rename renamed && + mv dir1/rename dir1/renamed && mv dir2/rename dir2/renamed } file_to_directory () { - rm -f delete - mkdir delete + rm -f delete && + mkdir delete && echo 1 >delete/new } directory_to_file () { - rm -rf dir1 + rm -rf dir1 && echo 1 >dir1 }
@@ -272,25 +326,20 @@ verify_status () { # 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 + git reset --hard HEAD && + git clean -fd && + test_might_fail 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 - ) && + start_daemon -tf "$PWD/.git/trace" && edit_files && - test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + test-tool fsmonitor-client query --token 0 && grep "^event: dir1/modified$" .git/trace && grep "^event: dir2/modified$" .git/trace &&
@@ -301,16 +350,11 @@ test_expect_success 'edit some files' ' 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 - ) && + start_daemon -tf "$PWD/.git/trace" && create_files && - test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + test-tool fsmonitor-client query --token 0 && grep "^event: dir1/new$" .git/trace && grep "^event: dir2/new$" .git/trace &&
@@ -320,16 +364,11 @@ test_expect_success 'create some files' ' 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 - ) && + start_daemon -tf "$PWD/.git/trace" && delete_files && - test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + test-tool fsmonitor-client query --token 0 && grep "^event: dir1/delete$" .git/trace && grep "^event: dir2/delete$" .git/trace &&
@@ -339,16 +378,11 @@ test_expect_success 'delete some files' ' 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 - ) && + start_daemon -tf "$PWD/.git/trace" && rename_files && - test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + test-tool fsmonitor-client query --token 0 && grep "^event: dir1/rename$" .git/trace && grep "^event: dir2/rename$" .git/trace &&
@@ -361,16 +395,11 @@ test_expect_success 'rename some files' ' 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 - ) && + start_daemon -tf "$PWD/.git/trace" && mv dirtorename dirrenamed && - test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + test-tool fsmonitor-client query --token 0 && grep "^event: dirtorename/*$" .git/trace && grep "^event: dirrenamed/*$" .git/trace
@@ -379,16 +408,11 @@ test_expect_success 'rename directory' ' 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 - ) && + start_daemon -tf "$PWD/.git/trace" && file_to_directory && - test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + test-tool fsmonitor-client query --token 0 && grep "^event: delete$" .git/trace && grep "^event: delete/new$" .git/trace
@@ -397,16 +421,11 @@ test_expect_success 'file changes to directory' ' 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 - ) && + start_daemon -tf "$PWD/.git/trace" && directory_to_file && - test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 && + test-tool fsmonitor-client query --token 0 && grep "^event: dir1$" .git/trace '
@@ -424,15 +443,7 @@ test_expect_success 'flush cached data' ' 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 - ) && + start_daemon -C test_flush -tf "$PWD/.git/trace_daemon" -tk true && # The daemon should have an initial token with no events in _0 and # then a few (probably platform-specific number of) events in _1.
@@ -441,8 +452,8 @@ test_expect_success 'flush cached data' ' 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_flush/file_1 && + > 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 &&
@@ -462,7 +473,7 @@ test_expect_success 'flush cached data' ' grep "^builtin:test_00000002:0Q$" actual_q2 && - touch test_flush/file_3 && + > 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 &&
@@ -485,15 +496,9 @@ test_expect_success 'setup worktree base' ' 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 - ) && + start_daemon -C wt-secondary \ + -tf "$PWD/trace_wt_secondary" \ + -t2 "$PWD/trace2_wt_secondary" && git -C wt-secondary fsmonitor--daemon stop && test_must_fail git -C wt-secondary fsmonitor--daemon status
Signature · 1 line
gitgitgadget
PATCH 4/16 t/helper/fsmonitor-client: cleanup call to parse_options()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! t/helper/fsmonitor-client: create IPC client to talk to \
FSMonitor Daemon
Elminate unnecessary code in cmd__fsmonitor_client() WRT parsing of options.
Fix name of test-tool in usage.
Don't localize die() message.
--- t/helper/test-fsmonitor-client.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
t/helper/test-fsmonitor-client.c +6 -11
@@ -49,7 +49,7 @@ static int do_send_query(const char *token) ret =fsmonitor_ipc__send_query (token, &answer);if (ret <0 ) -die (_ ("could not query fsmonitor--daemon" )); +die ("could not query fsmonitor--daemon" );write_in_full (1 , answer.buf, answer.len);strbuf_release (&answer);
@@ -85,8 +85,8 @@ int cmd__fsmonitor_client(int argc, const char **argv)const char *token =NULL ;const char *const fsmonitor_client_usage[] = { -N_ ("test-helper fsmonitor-client query [<token>]" ), -N_ ("test-helper fsmonitor-client flush" ), +N_ ("test-tool fsmonitor-client query [<token>]" ), +N_ ("test-tool fsmonitor-client flush" ),NULL , };
@@ -96,17 +96,12 @@ int cmd__fsmonitor_client(int argc, const char **argv) OPT_END() }; -if (argc <2 ) -usage_with_options (fsmonitor_client_usage, options); + argc =parse_options (argc, argv,NULL , options, fsmonitor_client_usage,0 ); -if (argc ==2 && !strcmp (argv[1 ],"-h" )) +if (argc !=1 )usage_with_options (fsmonitor_client_usage, options); - subcmd = argv[1 ]; - argv--; - argc++; - - argc =parse_options (argc, argv,NULL , options, fsmonitor_client_usage,0 ); + subcmd = argv[0 ];setup_git_directory ();
Signature · 1 line
gitgitgadget
PATCH 13/16 compat/fsmonitor/fsm-listen-win32: add _() to calls to error()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! compat/fsmonitor/fsm-listen-win32: implement FSMonitor backend \
on Windows
--- compat/fsmonitor/fsm-listen-win32.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
compat/fsmonitor/fsm-listen-win32.c +3 -3
@@ -82,7 +82,7 @@ static int normalize_path_in_utf8(FILE_NOTIFY_INFORMATION *info,if (len >0 ) goto normalize;if (GetLastError () !=ERROR_INSUFFICIENT_BUFFER ) { -error ("[GLE %ld] could not convert path to UTF-8: '%.*ls'" , +error (_ ("[GLE %ld] could not convert path to UTF-8: '%.*ls'" ),GetLastError (), (int )(info->FileNameLength /sizeof (WCHAR )), info->FileName);
@@ -185,7 +185,7 @@ static int start_rdcw_watch(struct fsmonitor_daemon_backend_data *data,if (watch->is_active)return 0 ; -error ("ReadDirectoryChangedW failed on '%s' [GLE %ld]" , +error (_ ("ReadDirectoryChangedW failed on '%s' [GLE %ld]" ), watch->path.buf,GetLastError ());return -1 ; }
@@ -228,7 +228,7 @@ static int recv_rdcw_watch(struct one_watch *watch)* sure it is worth it. */ -error ("GetOverlappedResult failed on '%s' [GLE %ld]" , +error (_ ("GetOverlappedResult failed on '%s' [GLE %ld]" ), watch->path.buf, gle);return -1 ; }
Signature · 1 line
gitgitgadget
PATCH 16/16 fsmonitor-settings: simplify initialization of settings data
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
fixup! fsmonitor: config settings are repository-specific
--- fsmonitor-settings.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
fsmonitor-settings.c +1 -2
@@ -22,11 +22,10 @@ static void lookup_fsmonitor_settings(struct repository *r)return ;CALLOC_ARRAY (s,1 ); + s->mode =FSMONITOR_MODE_DISABLED ; r->settings.fsmonitor = s; -fsm_settings__set_disabled (r); -/* * Overload the existing "core.fsmonitor" config setting (which * has historically been either unset or a hook pathname) to
Signature · 1 line
gitgitgadget
Quoted 5 lines · Jeff Hostetler via GitGitGadget
I'm calling this series part 2.5. It should be inserted between parts 2 and
3. I'll send a new version of part 3 after I send part 2.5.This series is a bunch of "fixup!" commits for part 2. It addresses feedback on part 2 that wasn't received until after part 2 had graduated to "next".
Better late than never, but I am sensing that part 2 was under-reviewed and we pushed it too hastily?
Quoted 3 lines
I was originally planning to include them at the beginning of part 3, but since there are so many, I thought it would be easier to add a fixup series in the middle.
I think that is prudent. Let's give part 3 (which ought to be more interesting than part 2.5) a more solid ground to build upon, and to prepare for that, let's first avoid repeating the mistake of merging a series in an under-reviewed state to 'next'.
Will queue (but not tonight).
Thanks.
Re: [PATCH 01/16] t/test-lib: avoid using git on LHS of pipe
In reply to Jeff Hostetler via GitGitGadgetQuoted 3 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
fixup! help: include fsmonitor--daemon feature flag in version info
Do you want me to kick part-2 out of 'next' so that these can be squashed into the original commits that need fixing? If not, you would want to have a real commit log message instead.
The patch text looks good, even though I had to see if 'output' may clobber or collide with names used in tests that want the prerequisite.
Quoted 17 lines
Signed-off-by Jeff Hostetler <[email protected]>--- t/test-lib.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)t/test-lib.sh +2
-1@@ -1803,5 +1803,6 @@ GIT_TEST_MAINT_SCHEDULER="none:exit 1" # Does this platform support `git fsmonitor--daemon` # test_lazy_prereq FSMONITOR_DAEMON ' - git version --build-options | grep "feature:" | grep "fsmonitor--daemon" + git version --build-options >output && + grep "feature: fsmonitor--daemon" output '
Re: [PATCH 02/16] update-index: convert advise() messages back to warning()
In reply to Jeff Hostetler via GitGitGadgetQuoted 3 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
fixup! update-index: convert fsmonitor warnings to advise
Same comment as 01/16 applies here. "convert ... back to ..." in the title refers to the fact that builtin-fsmonitor-part2 topic turned warning() into advise() without a good justification, I think. Flipping and flopping between warning and advise, without giving any justification going either direction, is not a good move.
I only have looked at one eighth of this part 2.5, but it starts to look that ejecting part-2 and redoing it may result in a cleaner code that is easier to understand, perhaps? For example, instead of applying this patch, we can just get rid of 1a9241e1 (update-index:
convert fsmonitor warnings to advise, 2022-03-01). As I read more, my impression will certainly change, I would expect. Let's see.
Quoted 34 lines
Signed-off-by Jeff Hostetler <[email protected]>--- builtin/update-index.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)builtin/update-index.c +6
-6@@ -1238,18 +1238,18 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)if (fsmonitor >0 ) {enum fsmonitor_mode fsm_mode =fsm_settings__get_mode (r);if (fsm_mode ==FSMONITOR_MODE_DISABLED ) { -advise (_ ("core.fsmonitor is unset; " -"set it if you really want to " -"enable fsmonitor" )); +warning (_ ("core.fsmonitor is unset; " +"set it if you really want to " +"enable fsmonitor" )); }add_fsmonitor (&the_index);report (_ ("fsmonitor enabled" )); }else if (!fsmonitor) {enum fsmonitor_mode fsm_mode =fsm_settings__get_mode (r);if (fsm_mode >FSMONITOR_MODE_DISABLED ) -advise (_ ("core.fsmonitor is set; " -"remove it if you really want to " -"disable fsmonitor" )); +warning (_ ("core.fsmonitor is set; " +"remove it if you really want to " +"disable fsmonitor" ));remove_fsmonitor (&the_index);report (_ ("fsmonitor disabled" )); }
Re: [PATCH 09/16] t7527: fix && chaining in matrix_try()
In reply to Jeff Hostetler via GitGitGadgetQuoted 14 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
fixup! t7527: test status with untracked-cache and fsmonitor--daemon
Signed-off-by Jeff Hostetler <[email protected]>--- t/t7527-builtin-fsmonitor.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)t/t7527-builtin-fsmonitor.sh +0
-0
t/t7527-builtin-fsmonitor.sh matrix_clean_up_repo () { · line 541
Quoted 9 lines
t/t7527-builtin-fsmonitor.sh +3
-3@@ -536,9 +536,9 @@ matrix_clean_up_repo () { } matrix_try () { - uc=$1 - fsm=$2 - fn=$3 + uc=$1 && + fsm=$2 && + fn=$3 &&
After seeing up to this step, I am reasonably well convinced that what we want is to kick the jh/builtin-fsmonitor-part2 topic back to 'seen', and you send instead of part2.5 an updated part2, with range-diff since the last round and this final iteration. A change like the above will be seen in the range-diff in the cover letter and most of them, like the above, will become trivial improvements, and then the result can hopefully be placed back in 'next' reasonably fast.
Opinions?
Re: [PATCH 04/16] t/helper/fsmonitor-client: cleanup call to parse_options()
In reply to Jeff Hostetler via GitGitGadgetQuoted 22 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
fixup! t/helper/fsmonitor-client: create IPC client to talk to \
FSMonitor DaemonElminate unnecessary code in
cmd__fsmonitor_client()WRT parsing of options.Fix name of test-tool in usage.
Don't localize
die()message.Signed-off-by Jeff Hostetler <[email protected]>--- t/helper/test-fsmonitor-client.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)t/helper/test-fsmonitor-client.c +0
-0
t/helper/test-fsmonitor-client.c static int do_send_query(const char *token) · line 52
Quoted 5 lines
t/helper/test-fsmonitor-client.c +1
-1@@ -49,7 +49,7 @@ static int do_send_query(const char *token) ret =fsmonitor_ipc__send_query (token, &answer);if (ret <0 ) -die (_ ("could not query fsmonitor--daemon" )); +die ("could not query fsmonitor--daemon" );
Good, since this is just a test helper.
t/helper/test-fsmonitor-client.c int cmd__fsmonitor_client(int argc, const char **argv) · line 89
Quoted 10 lines
t/helper/test-fsmonitor-client.c +2
-2write_in_full (1 , answer.buf, answer.len);strbuf_release (&answer);@@ -85,8 +85,8 @@ int cmd__fsmonitor_client(int argc, const char **argv)const char *token =NULL ;const char *const fsmonitor_client_usage[] = { -N_ ("test-helper fsmonitor-client query [<token>]" ), -N_ ("test-helper fsmonitor-client flush" ), +N_ ("test-tool fsmonitor-client query [<token>]" ), +N_ ("test-tool fsmonitor-client flush" ),
These are still marked for N_() translation.
Even if tehse were built-ins only the one containing <token> should have N_(), as the other one is a literal command whose translation won't change.
Quoted 13 lines
t/helper/test-fsmonitor-client.c +2
-3NULL, };@@ -96,17 +96,12 @@ int cmd__fsmonitor_client(int argc, const char **argv) OPT_END() }; -if (argc <2 ) -usage_with_options (fsmonitor_client_usage, options); + argc =parse_options (argc, argv,NULL , options, fsmonitor_client_usage,0 ); -if (argc ==2 && !strcmp (argv[1 ],"-h" )) +if (argc !=1 )
t/helper/test-fsmonitor-client.c int cmd__fsmonitor_client(int argc, const char **argv) · line 106
Quoted 10 lines
t/helper/test-fsmonitor-client.c +1
-5usage_with_options (fsmonitor_client_usage, options); - subcmd = argv[1 ]; - argv--; - argc++; - - argc =parse_options (argc, argv,NULL , options, fsmonitor_client_usage,0 ); + subcmd = argv[0 ];setup_git_directory ();
Looks good.
Re: [PATCH 07/16] t/perf/p7519: cleanup coding style
In reply to Jeff Hostetler via GitGitGadgetQuoted 3 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Fix code style of added "if then".
*nod*
Quoted 21 lines
Add body of builtin test to a
test_expect_success.Signed-off-by Jeff Hostetler <[email protected]>--- t/perf/p7519-fsmonitor.sh | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)t/perf/p7519-fsmonitor.sh +1
-1@@ -141,7 +141,7 @@ test_expect_success "one time repo setup" ' fi ' -setup_for_fsmonitor () { +setup_for_fsmonitor_hook () { # set INTEGRATION_SCRIPT depending on the environment if test -n "$INTEGRATION_PATH" then
t/perf/p7519-fsmonitor.sh test_perf_w_drop_caches () { · line 186
Quoted 7 lines
t/perf/p7519-fsmonitor.sh +1
-2@@ -182,8 +182,7 @@ test_perf_w_drop_caches () { } test_fsmonitor_suite () { - if test -n "$USE_FSMONITOR_DAEMON" - then + if test -n "$USE_FSMONITOR_DAEMON"; then DESC="builtin fsmonitor--daemon"
This "; then" etc. still uses the non-standard style (and this was added in aa072da617e (t/perf/p7519: add fsmonitor--daemon test cases, 2022-03-01).
Re: [PATCH 05/16] fsmonitor--daemon: refactor cookie handling for readability
In reply to Jeff Hostetler via GitGitGadgetQuoted 66 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
fixup! fsmonitor--daemon: use a cookie file to sync with file system
Use implicit definitions for
FCIR_enum values.Remove const from
cookie->name.Reverse if then and else branches around
open()to ease readability.Document that we don't care about errors from
close()andunlink().Signed-off-by Jeff Hostetler <[email protected]>--- builtin/fsmonitor--daemon.c | 53 +++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 23 deletions(-)builtin/fsmonitor--daemon.c +3
-22@@ -109,14 +109,14 @@ static int do_as_client__status(void)enum fsmonitor_cookie_item_result {FCIR_ERROR =-1 ,/* could not create cookie file ? */ -FCIR_INIT =0 , +FCIR_INIT ,FCIR_SEEN ,FCIR_ABORT , };struct fsmonitor_cookie_item {struct hashmap_entry entry; -const char *name; +char *name;enum fsmonitor_cookie_item_result result; };@@ -166,37 +166,44 @@ static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie(* 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); - -/* -* Technically, 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, but we -* don't have that routine in our thread-utils. -* -* After extensive beta testing I'm not really worried about -* this. Also note that the above open() and unlink() calls -* will cause at least two FS events on that path, so the odds -* of getting stuck are pretty slim. -*/ -while (cookie->result ==FCIR_INIT ) -pthread_cond_wait (&state->cookies_cond, - &state->main_lock); - }else { +if (fd <0 ) {error_errno (_ ("could not create fsmonitor cookie '%s'" ), cookie->name);
builtin/fsmonitor--daemon.c static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie( · line 182
Quoted 10 lines
builtin/fsmonitor--daemon.c +7
-0cookie->result =FCIR_ERROR ; + goto done; } +/* +* Technically, close() and unlink() can fail, but we don't +* care here. We only created the file to trigger a watch +* event from the FS to know that when we're up to date. +*/ +close (fd);
It still seems odd to explicitly want to ignore close() return values.
I realize that we do in (too many) existing places, but why wouldn't we want to e.g. catch an I/O error here early?
Re: [PATCH 08/16] t7527: add parameters to start_daemon to handle args and subshell
In reply to Jeff Hostetler via GitGitGadgetQuoted 30 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
fixup! t7527: create test for fsmonitor--daemon
Add parameters to
start_daemon()and handle subshell and logging args.Remove
/dev/nullfrom commands.Fix &&-chaining of functions.
Signed-off-by Jeff Hostetler <[email protected]>--- t/t7527-builtin-fsmonitor.sh | 217 ++++++++++++++++++----------------- 1 file changed, 111 insertions(+), 106 deletions(-)t/t7527-builtin-fsmonitor.sh +2
-3@@ -11,30 +11,85 @@ then fi stop_daemon_delete_repo () { - r=$1 - git -C $r fsmonitor--daemon stop >/dev/null 2>/dev/null + r=$1 && + test_might_fail git -C $r fsmonitor--daemon stop && rm -rf $1 - return 0 }
t/t7527-builtin-fsmonitor.sh then · line 23
Quoted 10 lines
t/t7527-builtin-fsmonitor.sh +4
-4start_daemon () { - case "$#" in - 1) r="-C $1";; - *) r=""; - esac + r="" && + tf="" && + t2="" && + tk="" &&
We usually just do :
x= &&
Not:
x="" &&
I.e. no need for the "".
t/t7527-builtin-fsmonitor.sh then · line 31
Quoted 10 lines
t/t7527-builtin-fsmonitor.sh +7
-2- git $r fsmonitor--daemon start || return $? - git $r fsmonitor--daemon status || return $? + while test "$#" -ne 0 + do + case "$1" in + -C) + shift; + test "$#" -ne 0 || + { echo >&2 "error: -C requires arg"; exit 1; }
IMO this exhaustive checking is better just skipped, we e.g.don't do this in "test_commit". Can't you just...
t/t7527-builtin-fsmonitor.sh then · line 32
Quoted 2 lines
t/t7527-builtin-fsmonitor.sh +2
-0+ r="-C $1" + shift
...properly &&-chain these whole things and have the "shift too many" and exit code from "shift" catch this?
Quoted 13 lines
t/t7527-builtin-fsmonitor.sh +13
-0+ ;; + -tf) + shift; + test "$#" -ne 0 || + { echo >&2 "error: -tf requires arg"; exit 1; } + tf="$1" + shift + ;; + -t2) + shift; + test "$#" -ne 0 || + { echo >&2 "error: -t2 requires arg"; exit 1; } + t2="$1"
t/t7527-builtin-fsmonitor.sh then · line 56
Quoted 10 lines
t/t7527-builtin-fsmonitor.sh +10
-0+ shift + ;; + -tk) + shift; + test "$#" -ne 0 || + { echo >&2 "error: -tk requires arg"; exit 1; } + tk="$1" + shift + ;; + *)
ditto.
t/t7527-builtin-fsmonitor.sh then · line 58
Quoted 2 lines
t/t7527-builtin-fsmonitor.sh +2
-0+ echo >&2 "error: unknown option: '$1'" + exit 1
But this sort of case is needed, but should use BUG "..."
t/t7527-builtin-fsmonitor.sh then · line 64
Quoted 6 lines
t/t7527-builtin-fsmonitor.sh +6
-0+ ;; + esac + done && + + ( + if test ! -z "$tf"
Instead of "! -z x" use "-n x" ?
t/t7527-builtin-fsmonitor.sh then · line 76
Quoted 12 lines
t/t7527-builtin-fsmonitor.sh +12
-0+ then + GIT_TRACE_FSMONITOR="$tf" + export GIT_TRACE_FSMONITOR + fi && + + if test ! -z "$t2" + then + GIT_TRACE2_PERF="$t2" + export GIT_TRACE2_PERF + fi && + + if test ! -z "$tk"
ditto.
t/t7527-builtin-fsmonitor.sh test_expect_success 'flush cached data' ' · line 456
Quoted 9 lines
t/t7527-builtin-fsmonitor.sh +6
-2+ then + GIT_TEST_FSMONITOR_TOKEN="$tk" + export GIT_TEST_FSMONITOR_TOKEN + fi && - touch test_flush/file_1 && - touch test_flush/file_2 && + > test_flush/file_1 && + > test_flush/file_2 &&
style: ">x" not "> x" (ditto below).
Re: [PATCH 05/16] fsmonitor--daemon: refactor cookie handling for readability
In reply to Ævar Arnfjörð BjarmasonQuoted 12 lines · Ævar Arnfjörð Bjarmason
builtin/fsmonitor--daemon.c static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie( · line 182
Quoted 6 lines · Jeff Hostetler via GitGitGadget
builtin/fsmonitor--daemon.c +6
-0+/* +* Technically, close() and unlink() can fail, but we don't +* care here. We only created the file to trigger a watch +* event from the FS to know that when we're up to date. +*/ +close (fd);It still seems odd to explicitly want to ignore
close()return values.
Quoted 2 lines
I realize that we do in (too many) existing places, but why wouldn't we want to e.g. catch an I/O error here early?
What exactly do you propose we do here if there is an I/O error during close()?
Thanks,
-Stolee
Re: [PATCH 05/16] fsmonitor--daemon: refactor cookie handling for readability
Quoted 13 lines · Derrick Stolee
Quoted 12 lines · Ævar Arnfjörð Bjarmason
builtin/fsmonitor--daemon.c static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie( · line 182
Quoted 6 lines · Jeff Hostetler via GitGitGadget
builtin/fsmonitor--daemon.c +6
-0+/* +* Technically, close() and unlink() can fail, but we don't +* care here. We only created the file to trigger a watch +* event from the FS to know that when we're up to date. +*/ +close (fd);It still seems odd to explicitly want to ignore
close()return values.
Quoted 5 lines
Quoted 2 lines
I realize that we do in (too many) existing places, but why wouldn't we want to e.g. catch an I/O error here early?
What exactly do you propose we do here if there is an I/O error during
close()?
We created the file to trigger a watch event, but now we have a reason to suspect that the wished-for watch event may not come.
We only did so to know that when we're up to date. Now we may never know? We may go without realizing we are already up to date a bit longer than the reality?
How much damage would it cause us to miss a watch event in this case? Very little? Is it a thing that sysadmins may care if we see too many of, but there is nothing the end user can immediately do about? If it is, perhaps a trace2 event to report it (and other "we do not care here" syscalls that fail)?
Re: [PATCH 16/16] fsmonitor-settings: simplify initialization of settings data
In reply to Jeff Hostetler via GitGitGadgetQuoted 3 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
fixup! fsmonitor: config settings are repository-specific
I cannot tell the validity of this change alone without the base commit in part2 to choose between setting s->mode directly or calling __set_disabled(r).
I've read all the 16 patches here, and I found that the majority of them (except for "it is hard to tell why we are changing in 2.5; it needs a better justification" ones like this) are "oops, the part2 patches had these obvious and trivial mistakes that we should have fixed before we merged them to 'next', or even sending them to the list in the first place" fixes.
Let's kick part2 out of 'next', and replace it with a corrected set of patches and have people review them, this time for real, before merging it back to 'next'. I just do not get the good feel that the series was adequately reviewed---if we have this many "oops" fixups needed after getting merged to 'next'.
Thanks.
Quoted 22 lines
Signed-off-by Jeff Hostetler <[email protected]>--- fsmonitor-settings.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)fsmonitor-settings.c +1
-2@@ -22,11 +22,10 @@ static void lookup_fsmonitor_settings(struct repository *r)return ;CALLOC_ARRAY (s,1 ); + s->mode =FSMONITOR_MODE_DISABLED ; r->settings.fsmonitor = s; -fsm_settings__set_disabled (r); -/* * Overload the existing "core.fsmonitor" config setting (which * has historically been either unset or a hook pathname) to
Re: [PATCH 16/16] fsmonitor-settings: simplify initialization of settings data
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Quoted 8 lines
Quoted 1 line
fixup! fsmonitor: config settings are repository-specific
Let's kick part2 out of 'next', and replace it with a corrected set of patches and have people review them, this time for real, before merging it back to 'next'. I just do not get the good feel that the series was adequately reviewed---if we have this many "oops" fixups needed after getting merged to 'next'.
Yeah if you haven't merged part2 to master yet, wait and i'll send a V7 of part2 that squashes in these fixups.
and addresses any other comments on part2.5 itself.
Thanks and sorry.
Jeff
Re: [PATCH 16/16] fsmonitor-settings: simplify initialization of settings data
Quoted 4 lines · Jeff Hostetler
Yeah if you haven't merged part2 to master yet, wait and i'll send a V7 of part2 that squashes in these fixups.
and addresses any other comments on part2.5 itself.
Thanks. I think that would be cleaner in the end.
Re: [PATCH 02/16] update-index: convert advise() messages back to warning()
In reply to Junio C HamanoQuoted 11 lines · Junio C Hamano
Quoted 3 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
fixup! update-index: convert fsmonitor warnings to advise
Same comment as 01/16 applies here. "convert ... back to ..." in the title refers to the fact that builtin-fsmonitor-part2 topic turned
warning()intoadvise()without a good justification, I think. Flipping and flopping between warning and advise, without giving any justification going either direction, is not a good move.
Sorry for not including the backstory.
Ben wrote the original warning message back in 2017.
In my original version of part 2 I added a more detailed warning message because of the new core.useBuiltinFSMonitor config settings and how it interacted with core.fsmonitor. And we talked about making that longer message advise rather than a warning. So I changed them to advise().
But then we decided to remove core.useBuiltinFSMonitor and overload core.fsmonitor to take a boolean, so the original text of the message was sufficient and correct. So to minimize the diff, I reverted the text change and kept the change from warning() to advise().
But then there were comments from AEvar on either changing all of the nearby warning() calls to advise() *and* to change them to use the advise_type enum. That seemed like a large/disruptive change at this point.
Also, I wasn't really sure of the need for Ben's original warning message, so I'd rather leave it as is than expand the scope.
So I basically reverted the change for this series. If (in a later series) we want to revisit all of the warnings in update-index.c, then we can think about this.
Jeff
Re: [PATCH 05/16] fsmonitor--daemon: refactor cookie handling for readability
In reply to Junio C HamanoQuoted 28 lines · Junio C Hamano
Quoted 13 lines · Derrick Stolee
Quoted 12 lines · Ævar Arnfjörð Bjarmason
builtin/fsmonitor--daemon.c static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie( · line 182
Quoted 6 lines · Jeff Hostetler via GitGitGadget
builtin/fsmonitor--daemon.c +6
-0+/* +* Technically, close() and unlink() can fail, but we don't +* care here. We only created the file to trigger a watch +* event from the FS to know that when we're up to date. +*/ +close (fd);It still seems odd to explicitly want to ignore
close()return values.Quoted 5 lines
Quoted 2 lines
I realize that we do in (too many) existing places, but why wouldn't we want to e.g. catch an I/O error here early?
What exactly do you propose we do here if there is an I/O error during
close()?We created the file to trigger a watch event, but now we have a reason to suspect that the wished-for watch event may not come.
We only did so to know that when we're up to date. Now we may never know? We may go without realizing we are already up to date a bit longer than the reality?
Quoted 8 lines
How much damage would it cause us to miss a watch event in this case? Very little? Is it a thing that sysadmins may care if we see too many of, but there is nothing the end user can immediately do about? If it is, perhaps a trace2 event to report it (and other "we do not care here" syscalls that fail)?
The open(... O_CREAT ...) succeeded, so we actually created a new file and expect a FS event for it. That FS event (when seen by the FS listener thread) will cause our condition to be signaled and allow this thread to wake up and respond to the client.
The odds of the close() failing on a plain file (after a successful open()) are very slim. And there's nothing that we can do about the failure anyway. (And we're not relying on an FS event from theclose() succeeding, so it really doesn't matter.) Technically, it
is possible that the daemon could run out of fd's if this close() fails often, so at some point the daemon might not be able to create new cookie files. But the daemon currently defaults to sending a trivial response to the client -- if this turns out to be a real issue, we could have the daemon restart or something, but I'm not going to worry about that right now.
The odds of a failure in unlink() is a little more interesting. This would mean that a stale cookie file would be left in the cookie directory (and waste a little disk space). But that is not likely either (for a plain file that we just created). Since we're not relying on the FS event for the unlink(), the failure here won't block the current thread either. Deleting stale cookie files is something that we could try to address in the future if it turns out to be a problem.
Jeff