[PATCH 00/16] Builtin FSMonitor Part 2.5

[PATCH 00/16] Builtin FSMonitor Part 2.5

[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".

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
-- 
gitgitgadget

[PATCH 01/16] t/test-lib: avoid using git on LHS of pipe

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! help: include fsmonitor--daemon feature flag in version info

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/test-lib.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 46cd596e7f5..5d819c1bc11 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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
 '
-- 
gitgitgadget

Re: [PATCH 01/16] t/test-lib: avoid using git on LHS of pipe

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> 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.

> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  t/test-lib.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 46cd596e7f5..5d819c1bc11 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -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
>  '

[PATCH 02/16] update-index: convert advise() messages back to warning()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! update-index: convert fsmonitor warnings to advise

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/update-index.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index d335f1ac72a..75d646377cc 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -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"));
 	}
-- 
gitgitgadget

Re: [PATCH 02/16] update-index: convert advise() messages back to warning()

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> 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.

> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  builtin/update-index.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index d335f1ac72a..75d646377cc 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -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 02/16] update-index: convert advise() messages back to warning()

[Jeff Hostetler]

On 3/14/22 2:08 AM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> 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.

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

[PATCH 03/16] compat/fsmonitor/fsm-listen-darwin: split out GCC-specific declarations

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 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

diff --git a/compat/fsmonitor/fsm-darwin-gcc.h b/compat/fsmonitor/fsm-darwin-gcc.h
new file mode 100644
index 00000000000..1c75c3d48e7
--- /dev/null
+++ b/compat/fsmonitor/fsm-darwin-gcc.h
@@ -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 */
diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c
index 5c5de1ae702..9a73fb607d5 100644
--- a/compat/fsmonitor/fsm-listen-darwin.c
+++ b/compat/fsmonitor/fsm-listen-darwin.c
@@ -1,95 +1,6 @@
 #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);
+#include "fsm-darwin-gcc.h"
 #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
-- 
gitgitgadget

[PATCH 04/16] t/helper/fsmonitor-client: cleanup call to parse_options()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/helper/test-fsmonitor-client.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/t/helper/test-fsmonitor-client.c b/t/helper/test-fsmonitor-client.c
index f7a5b3a32fa..d59a640f1f9 100644
--- a/t/helper/test-fsmonitor-client.c
+++ b/t/helper/test-fsmonitor-client.c
@@ -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();
 
-- 
gitgitgadget

Re: [PATCH 04/16] t/helper/fsmonitor-client: cleanup call to parse_options()

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> 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.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  t/helper/test-fsmonitor-client.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/t/helper/test-fsmonitor-client.c b/t/helper/test-fsmonitor-client.c
> index f7a5b3a32fa..d59a640f1f9 100644
> --- a/t/helper/test-fsmonitor-client.c
> +++ b/t/helper/test-fsmonitor-client.c
> @@ -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.

>  	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"),

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.

>  		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();

Looks good.

[PATCH 05/16] fsmonitor--daemon: refactor cookie handling for readability

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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().

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/fsmonitor--daemon.c | 53 +++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 97ca2a356e5..02a99ce98a2 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -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);
 
-- 
gitgitgadget

Re: [PATCH 05/16] fsmonitor--daemon: refactor cookie handling for readability

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> 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().
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  builtin/fsmonitor--daemon.c | 53 +++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
> index 97ca2a356e5..02a99ce98a2 100644
> --- a/builtin/fsmonitor--daemon.c
> +++ b/builtin/fsmonitor--daemon.c
> @@ -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);

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 05/16] fsmonitor--daemon: refactor cookie handling for readability

[Derrick Stolee]

On 3/14/2022 4:00 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:

>> +	/*
>> +	 * 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?

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

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 3/14/2022 4:00 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:
>
>>> +	/*
>>> +	 * 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?
>
> 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 05/16] fsmonitor--daemon: refactor cookie handling for readability

[Jeff Hostetler]

On 3/14/22 1:47 PM, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
>> On 3/14/2022 4:00 AM, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:
>>
>>>> +	/*
>>>> +	 * 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?
>>
>> 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)?
> 
> 
> 

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 the
close() 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

[PATCH 06/16] t/perf/p7519: use grep rather than egrep in test

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! t/perf/p7519: speed up test on Windows

Use grep rather than egrep

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/perf/p7519-fsmonitor.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
index a1c552129cc..5f97edf6a11 100755
--- a/t/perf/p7519-fsmonitor.sh
+++ b/t/perf/p7519-fsmonitor.sh
@@ -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
 	'
-- 
gitgitgadget

[PATCH 07/16] t/perf/p7519: cleanup coding style

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/perf/p7519-fsmonitor.sh | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
index 5f97edf6a11..7a7981b0e61 100755
--- a/t/perf/p7519-fsmonitor.sh
+++ b/t/perf/p7519-fsmonitor.sh
@@ -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
 
-- 
gitgitgadget

Re: [PATCH 07/16] t/perf/p7519: cleanup coding style

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>

> Fix code style of added "if then".

*nod*

> Add body of builtin test to a test_expect_success.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  t/perf/p7519-fsmonitor.sh | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
> index 5f97edf6a11..7a7981b0e61 100755
> --- a/t/perf/p7519-fsmonitor.sh
> +++ b/t/perf/p7519-fsmonitor.sh
> @@ -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"

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).

[PATCH 08/16] t7527: add parameters to start_daemon to handle args and subshell

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t7527-builtin-fsmonitor.sh | 217 ++++++++++++++++++-----------------
 1 file changed, 111 insertions(+), 106 deletions(-)

diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 0ccbfb9616f..026382a0d86 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -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
-- 
gitgitgadget

Re: [PATCH 08/16] t7527: add parameters to start_daemon to handle args and subshell

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> 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.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  t/t7527-builtin-fsmonitor.sh | 217 ++++++++++++++++++-----------------
>  1 file changed, 111 insertions(+), 106 deletions(-)
>
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> index 0ccbfb9616f..026382a0d86 100755
> --- a/t/t7527-builtin-fsmonitor.sh
> +++ b/t/t7527-builtin-fsmonitor.sh
> @@ -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="" &&

We usually just do : 

	x= &&

Not:

	x="" &&

I.e. no need for the "".

>  
> -	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...

> +			r="-C $1"
> +			shift

...properly &&-chain these whole things and have the "shift too many"
and exit code from "shift" catch this?

> +			;;
> +		-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
> +			;;
> +		*)

ditto.

> +			echo >&2 "error: unknown option: '$1'"
> +			exit 1

But this sort of case is needed, but should use BUG "..."

> +			;;
> +		esac
> +	done &&
> +
> +	(
> +		if test ! -z "$tf"

Instead of "! -z x" use "-n x" ?

> +		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.

> +		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).

[PATCH 09/16] t7527: fix && chaining in matrix_try()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! t7527: test status with untracked-cache and fsmonitor--daemon

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t7527-builtin-fsmonitor.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 026382a0d86..f60e211dbab 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -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 &&
-- 
gitgitgadget

Re: [PATCH 09/16] t7527: fix && chaining in matrix_try()

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> fixup! t7527: test status with untracked-cache and fsmonitor--daemon
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  t/t7527-builtin-fsmonitor.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> index 026382a0d86..f60e211dbab 100755
> --- a/t/t7527-builtin-fsmonitor.sh
> +++ b/t/t7527-builtin-fsmonitor.sh
> @@ -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?

[PATCH 10/16] t7527: delete unused verify_status() function

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! t7527: create test for fsmonitor--daemon

Remove unused function verify_status().

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t7527-builtin-fsmonitor.sh | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index f60e211dbab..ef8777e7f6d 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -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
-- 
gitgitgadget

[PATCH 11/16] fsmonitor-ipc: add _() to calls to die()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! fsmonitor-ipc: create client routines for git-fsmonitor--daemon

Fix translation markings.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 fsmonitor-ipc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index 91a535f1219..789e7397baa 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -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;
 	}
 
-- 
gitgitgadget

[PATCH 12/16] compat/fsmonitor/fsm-listen-darwin: add _() to calls to error()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! compat/fsmonitor/fsm-listen-darwin: implement FSEvent listener \
on MacOS

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/fsmonitor/fsm-listen-darwin.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c
index 9a73fb607d5..d9d0d0fa0e9 100644
--- a/compat/fsmonitor/fsm-listen-darwin.c
+++ b/compat/fsmonitor/fsm-listen-darwin.c
@@ -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;
-- 
gitgitgadget

[PATCH 13/16] compat/fsmonitor/fsm-listen-win32: add _() to calls to error()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! compat/fsmonitor/fsm-listen-win32: implement FSMonitor backend \
on Windows

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/fsmonitor/fsm-listen-win32.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/compat/fsmonitor/fsm-listen-win32.c b/compat/fsmonitor/fsm-listen-win32.c
index c2d11acbc1e..5b928ab66e5 100644
--- a/compat/fsmonitor/fsm-listen-win32.c
+++ b/compat/fsmonitor/fsm-listen-win32.c
@@ -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;
 }
-- 
gitgitgadget

[PATCH 14/16] fsmonitor--daemon: add _() to calls to die()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! fsmonitor--daemon: implement 'run' command

Fix translation marking on die().

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/fsmonitor--daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 02a99ce98a2..b3687a200ef 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -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) {
-- 
gitgitgadget

[PATCH 15/16] fsmonitor--daemon: add _() to calls to error()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! fsmonitor--daemon: implement 'start' command

Fixup translation on die().

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/fsmonitor--daemon.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index b3687a200ef..a30ecf6cfac 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -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"));
 	}
 }
 
-- 
gitgitgadget

[PATCH 16/16] fsmonitor-settings: simplify initialization of settings data

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

fixup! fsmonitor: config settings are repository-specific

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 fsmonitor-settings.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index 3b54e7a51f6..757d230d538 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -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
-- 
gitgitgadget

Re: [PATCH 16/16] fsmonitor-settings: simplify initialization of settings data

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> 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.



> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  fsmonitor-settings.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
> index 3b54e7a51f6..757d230d538 100644
> --- a/fsmonitor-settings.c
> +++ b/fsmonitor-settings.c
> @@ -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

[Jeff Hostetler]

On 3/14/22 3:49 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> 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

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> 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 00/16] Builtin FSMonitor Part 2.5

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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?

> 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.