[PATCH 0/8] fsmonitor unused parameter cleanups

[PATCH 0/8] fsmonitor unused parameter cleanups

[Jeff King]

Here are a few cleanups of the fsmonitor code to remove or annotate
unused parameters (working towards my goal of making us compile clean
with -Wunused-parameter). I think they should all be pretty
non-controversial, but I'm cc-ing folks active in the area in case patch
2 steps on the toes of any unpublished works in progress.

  [1/8]: fsmonitor: prefer repo_git_path() to git_pathdup()
  [2/8]: fsmonitor/win32: drop unused parameters
  [3/8]: fsmonitor: mark some maybe-unused parameters
  [4/8]: fsmonitor/win32: mark unused parameter in fsm_os__incompatible()
  [5/8]: fsmonitor: mark unused parameters in stub functions
  [6/8]: fsmonitor/darwin: mark unused parameters in system callback
  [7/8]: fsmonitor: mark unused hashmap callback parameters
  [8/8]: run-command: mark unused parameters in start_bg_wait callbacks

 builtin/fsmonitor--daemon.c             | 10 ++++++----
 compat/fsmonitor/fsm-health-darwin.c    |  8 ++++----
 compat/fsmonitor/fsm-ipc-win32.c        |  2 +-
 compat/fsmonitor/fsm-listen-darwin.c    |  4 ++--
 compat/fsmonitor/fsm-listen-win32.c     | 24 ++++++++++--------------
 compat/fsmonitor/fsm-path-utils-win32.c |  7 ++++---
 compat/fsmonitor/fsm-settings-win32.c   |  2 +-
 fsmonitor-ipc.c                         | 10 +++++-----
 fsmonitor-settings.c                    |  3 ++-
 t/helper/test-simple-ipc.c              |  3 ++-
 10 files changed, 37 insertions(+), 36 deletions(-)

-Peff

[PATCH 1/8] fsmonitor: prefer repo_git_path() to git_pathdup()

[Jeff King]

The fsmonitor_ipc__get_path() function ignores its repository argument.
It should use it when constructing repo paths (though in practice, it is
unlikely anything but the_repository is ever passed, so this is cleanup
and future proofing, not a bug fix).

Note that despite the lack of "dup" in the name, repo_git_path() behaves
like git_pathdup() and returns an allocated string.

Signed-off-by: Jeff King <peff@peff.net>
---
 compat/fsmonitor/fsm-ipc-win32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/fsmonitor/fsm-ipc-win32.c b/compat/fsmonitor/fsm-ipc-win32.c
index 8928fa93ce..41984ea48e 100644
--- a/compat/fsmonitor/fsm-ipc-win32.c
+++ b/compat/fsmonitor/fsm-ipc-win32.c
@@ -6,6 +6,6 @@
 const char *fsmonitor_ipc__get_path(struct repository *r) {
 	static char *ret;
 	if (!ret)
-		ret = git_pathdup("fsmonitor--daemon.ipc");
+		ret = repo_git_path(r, "fsmonitor--daemon.ipc");
 	return ret;
 }
-- 
2.42.0.671.g43fbf3903a

[PATCH 2/8] fsmonitor/win32: drop unused parameters

[Jeff King]

A few helper functions (centered around file-watch events) take extra
fsmonitor state parameters that they don't use. These are static helpers
local to the win32 implementation, and don't need to conform to any
particular interface. We can just drop the extra parameters, which
simplifies the code and silences -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 compat/fsmonitor/fsm-listen-win32.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/compat/fsmonitor/fsm-listen-win32.c b/compat/fsmonitor/fsm-listen-win32.c
index a361a7db20..90a2412284 100644
--- a/compat/fsmonitor/fsm-listen-win32.c
+++ b/compat/fsmonitor/fsm-listen-win32.c
@@ -289,8 +289,7 @@ void fsm_listen__stop_async(struct fsmonitor_daemon_state *state)
 	SetEvent(state->listen_data->hListener[LISTENER_SHUTDOWN]);
 }
 
-static struct one_watch *create_watch(struct fsmonitor_daemon_state *state,
-				      const char *path)
+static struct one_watch *create_watch(const char *path)
 {
 	struct one_watch *watch = NULL;
 	DWORD desired_access = FILE_LIST_DIRECTORY;
@@ -361,8 +360,7 @@ static void destroy_watch(struct one_watch *watch)
 	free(watch);
 }
 
-static int start_rdcw_watch(struct fsm_listen_data *data,
-			    struct one_watch *watch)
+static int start_rdcw_watch(struct one_watch *watch)
 {
 	DWORD dwNotifyFilter =
 		FILE_NOTIFY_CHANGE_FILE_NAME |
@@ -735,11 +733,11 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
 
 	state->listen_error_code = 0;
 
-	if (start_rdcw_watch(data, data->watch_worktree) == -1)
+	if (start_rdcw_watch(data->watch_worktree) == -1)
 		goto force_error_stop;
 
 	if (data->watch_gitdir &&
-	    start_rdcw_watch(data, data->watch_gitdir) == -1)
+	    start_rdcw_watch(data->watch_gitdir) == -1)
 		goto force_error_stop;
 
 	for (;;) {
@@ -755,15 +753,15 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
 			}
 			if (result == -2) {
 				/* retryable error */
-				if (start_rdcw_watch(data, data->watch_worktree) == -1)
+				if (start_rdcw_watch(data->watch_worktree) == -1)
 					goto force_error_stop;
 				continue;
 			}
 
 			/* have data */
 			if (process_worktree_events(state) == LISTENER_SHUTDOWN)
 				goto force_shutdown;
-			if (start_rdcw_watch(data, data->watch_worktree) == -1)
+			if (start_rdcw_watch(data->watch_worktree) == -1)
 				goto force_error_stop;
 			continue;
 		}
@@ -776,15 +774,15 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
 			}
 			if (result == -2) {
 				/* retryable error */
-				if (start_rdcw_watch(data, data->watch_gitdir) == -1)
+				if (start_rdcw_watch(data->watch_gitdir) == -1)
 					goto force_error_stop;
 				continue;
 			}
 
 			/* have data */
 			if (process_gitdir_events(state) == LISTENER_SHUTDOWN)
 				goto force_shutdown;
-			if (start_rdcw_watch(data, data->watch_gitdir) == -1)
+			if (start_rdcw_watch(data->watch_gitdir) == -1)
 				goto force_error_stop;
 			continue;
 		}
@@ -821,16 +819,14 @@ int fsm_listen__ctor(struct fsmonitor_daemon_state *state)
 
 	data->hEventShutdown = CreateEvent(NULL, TRUE, FALSE, NULL);
 
-	data->watch_worktree = create_watch(state,
-					    state->path_worktree_watch.buf);
+	data->watch_worktree = create_watch(state->path_worktree_watch.buf);
 	if (!data->watch_worktree)
 		goto failed;
 
 	check_for_shortnames(data->watch_worktree);
 
 	if (state->nr_paths_watching > 1) {
-		data->watch_gitdir = create_watch(state,
-						  state->path_gitdir_watch.buf);
+		data->watch_gitdir = create_watch(state->path_gitdir_watch.buf);
 		if (!data->watch_gitdir)
 			goto failed;
 	}
-- 
2.42.0.671.g43fbf3903a

[PATCH 3/8] fsmonitor: mark some maybe-unused parameters

[Jeff King]

There's a bit of conditionally-compiled code in fsmonitor, so some
function parameters may be unused depending on the build options:

  - in fsmonitor--daemon.c's try_to_run_foreground_daemon(), we take a
    detach_console argument, but it's only used on Windows. This seems
    intentional (and not mistakenly missing other platforms) based on
    the discussion in c284e27ba7 (fsmonitor--daemon: implement 'start'
    command, 2022-03-25), which introduced it.

  - in fsmonitor-setting.c's check_for_incompatible(), we pass the "ipc"
    flag down to the system-specific fsm_os__incompatible() helper. But
    we can only do so if our platform has such a helper.

In both cases we can mark the argument as MAYBE_UNUSED. That annotates
it enough to suppress the compiler's -Wunused-parameter warning, but
without making it impossible to use the variable, as a regular UNUSED
annotation would.

Signed-off-by: Jeff King <peff@peff.net>
---
For a similar case in 2c3c3d88fc (imap-send: mark unused parameters with
NO_OPENSSL, 2023-08-29), I used the old:

  (void)some_parameter_that_might_not_be_used;

trick. But I realized while writing this one that MAYBE_UNUSED fits the
bill a little more nicely, and I don't see any reason we would have
portability problems with it.

 builtin/fsmonitor--daemon.c | 2 +-
 fsmonitor-settings.c        | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 7e99c4d61b..7c4130c981 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1412,7 +1412,7 @@ static int fsmonitor_run_daemon(void)
 	return err;
 }
 
-static int try_to_run_foreground_daemon(int detach_console)
+static int try_to_run_foreground_daemon(int detach_console MAYBE_UNUSED)
 {
 	/*
 	 * Technically, we don't need to probe for an existing daemon
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index b62acf44ae..a6a9e6bc19 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -62,7 +62,8 @@ static enum fsmonitor_reason check_remote(struct repository *r)
 }
 #endif
 
-static enum fsmonitor_reason check_for_incompatible(struct repository *r, int ipc)
+static enum fsmonitor_reason check_for_incompatible(struct repository *r,
+						    int ipc MAYBE_UNUSED)
 {
 	if (!r->worktree) {
 		/*
-- 
2.42.0.671.g43fbf3903a

[PATCH 4/8] fsmonitor/win32: mark unused parameter in fsm_os__incompatible()

[Jeff King]

We never look at the "ipc" argument we receive. It was added in
8f44976882 (fsmonitor: avoid socket location check if using hook,
2022-10-04) to support the darwin fsmonitor code. The win32 code has to
match the same interface, but we should use an annotation to silence
-Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 compat/fsmonitor/fsm-settings-win32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index b6f6744494..0f2aa321f6 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -25,7 +25,7 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
 	return FSMONITOR_REASON_OK;
 }
 
-enum fsmonitor_reason fsm_os__incompatible(struct repository *r, int ipc)
+enum fsmonitor_reason fsm_os__incompatible(struct repository *r, int ipc UNUSED)
 {
 	enum fsmonitor_reason reason;
 
-- 
2.42.0.671.g43fbf3903a

[PATCH 5/8] fsmonitor: mark unused parameters in stub functions

[Jeff King]

The fsmonitor code has some platform-specific functions for which one or
more platforms implement noop or stub functions. We can't get rid of
these functions nor change their interface, since the point is to match
their equivalents in other platforms. But let's annotate their
parameters to quiet the compiler's -Wunused-parameter warning.

Signed-off-by: Jeff King <peff@peff.net>
---
 compat/fsmonitor/fsm-health-darwin.c    |  8 ++++----
 compat/fsmonitor/fsm-path-utils-win32.c |  7 ++++---
 fsmonitor-ipc.c                         | 10 +++++-----
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/compat/fsmonitor/fsm-health-darwin.c b/compat/fsmonitor/fsm-health-darwin.c
index 5b1709d63f..c2afcbe6c8 100644
--- a/compat/fsmonitor/fsm-health-darwin.c
+++ b/compat/fsmonitor/fsm-health-darwin.c
@@ -4,21 +4,21 @@
 #include "fsm-health.h"
 #include "fsmonitor--daemon.h"
 
-int fsm_health__ctor(struct fsmonitor_daemon_state *state)
+int fsm_health__ctor(struct fsmonitor_daemon_state *state UNUSED)
 {
 	return 0;
 }
 
-void fsm_health__dtor(struct fsmonitor_daemon_state *state)
+void fsm_health__dtor(struct fsmonitor_daemon_state *state UNUSED)
 {
 	return;
 }
 
-void fsm_health__loop(struct fsmonitor_daemon_state *state)
+void fsm_health__loop(struct fsmonitor_daemon_state *state UNUSED)
 {
 	return;
 }
 
-void fsm_health__stop_async(struct fsmonitor_daemon_state *state)
+void fsm_health__stop_async(struct fsmonitor_daemon_state *state UNUSED)
 {
 }
diff --git a/compat/fsmonitor/fsm-path-utils-win32.c b/compat/fsmonitor/fsm-path-utils-win32.c
index c8a3e9dcdb..f4f9cc1f33 100644
--- a/compat/fsmonitor/fsm-path-utils-win32.c
+++ b/compat/fsmonitor/fsm-path-utils-win32.c
@@ -132,16 +132,17 @@ int fsmonitor__is_fs_remote(const char *path)
 /*
  * No-op for now.
  */
-int fsmonitor__get_alias(const char *path, struct alias_info *info)
+int fsmonitor__get_alias(const char *path UNUSED,
+			 struct alias_info *info UNUSED)
 {
 	return 0;
 }
 
 /*
  * No-op for now.
  */
-char *fsmonitor__resolve_alias(const char *path,
-	const struct alias_info *info)
+char *fsmonitor__resolve_alias(const char *path UNUSED,
+			       const struct alias_info *info UNUSED)
 {
 	return NULL;
 }
diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index 88575aa54c..153918cf76 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -20,7 +20,7 @@ int fsmonitor_ipc__is_supported(void)
 	return 0;
 }
 
-const char *fsmonitor_ipc__get_path(struct repository *r)
+const char *fsmonitor_ipc__get_path(struct repository *r UNUSED)
 {
 	return NULL;
 }
@@ -30,14 +30,14 @@ enum ipc_active_state fsmonitor_ipc__get_state(void)
 	return IPC_STATE__OTHER_ERROR;
 }
 
-int fsmonitor_ipc__send_query(const char *since_token,
-			      struct strbuf *answer)
+int fsmonitor_ipc__send_query(const char *since_token UNUSED,
+			      struct strbuf *answer UNUSED)
 {
 	return -1;
 }
 
-int fsmonitor_ipc__send_command(const char *command,
-				struct strbuf *answer)
+int fsmonitor_ipc__send_command(const char *command UNUSED,
+				struct strbuf *answer UNUSED)
 {
 	return -1;
 }
-- 
2.42.0.671.g43fbf3903a

[PATCH 6/8] fsmonitor/darwin: mark unused parameters in system callback

[Jeff King]

We pass fsevent_callback() to the system FSEventStreamCreate() function
as a callback. So we must match the expected function signature, even
though we don't care about all of the parameters. Mark the unused ones
to satisfy -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 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 36c7e13281..11b56d3ef1 100644
--- a/compat/fsmonitor/fsm-listen-darwin.c
+++ b/compat/fsmonitor/fsm-listen-darwin.c
@@ -191,12 +191,12 @@ static void my_add_path(struct fsmonitor_batch *batch, const char *path)
 }
 
 
-static void fsevent_callback(ConstFSEventStreamRef streamRef,
+static void fsevent_callback(ConstFSEventStreamRef streamRef UNUSED,
 			     void *ctx,
 			     size_t num_of_events,
 			     void *event_paths,
 			     const FSEventStreamEventFlags event_flags[],
-			     const FSEventStreamEventId event_ids[])
+			     const FSEventStreamEventId event_ids[] UNUSED)
 {
 	struct fsmonitor_daemon_state *state = ctx;
 	struct fsm_listen_data *data = state->listen_data;
-- 
2.42.0.671.g43fbf3903a

[PATCH 7/8] fsmonitor: mark unused hashmap callback parameters

[Jeff King]

Like many hashmap comparison functions, our cookies_cmp() does not look
at its extra void data parameter. This should have been annotated in
02c3c59e62 (hashmap: mark unused callback parameters, 2022-08-19), but
this new case was added around the same time (plus fsmonitor is not
built at all on Linux, so it is easy to miss there).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fsmonitor--daemon.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 7c4130c981..ce511c3ed6 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -129,8 +129,9 @@ struct fsmonitor_cookie_item {
 	enum fsmonitor_cookie_item_result result;
 };
 
-static int cookies_cmp(const void *data, const struct hashmap_entry *he1,
-		     const struct hashmap_entry *he2, const void *keydata)
+static int cookies_cmp(const void *data UNUSED,
+		       const struct hashmap_entry *he1,
+		       const struct hashmap_entry *he2, const void *keydata)
 {
 	const struct fsmonitor_cookie_item *a =
 		container_of(he1, const struct fsmonitor_cookie_item, entry);
-- 
2.42.0.671.g43fbf3903a

[PATCH 8/8] run-command: mark unused parameters in start_bg_wait callbacks

[Jeff King]

The start_bg_command() function takes a callback to tell when the
background-ed process is "ready". The callback receives the
child_process struct as well as an extra void pointer. But curiously,
neither of the two users of this interface look at either parameter!

This makes some sense. The only non-test user of the API is fsmonitor,
which uses fsmonitor_ipc__get_state() to connect to a single global
fsmonitor daemon (i.e., the one we just started!).

So we could just drop these parameters entirely. But it seems like a
pretty reasonable interface for the "wait" callback to have access to
the details of the spawned process, and to have room for passing extra
data through a void pointer. So let's leave these in place but mark the
unused ones so that -Wunused-parameter does not complain.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fsmonitor--daemon.c | 3 ++-
 t/helper/test-simple-ipc.c  | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index ce511c3ed6..5d01db5c02 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1443,7 +1443,8 @@ static int try_to_run_foreground_daemon(int detach_console MAYBE_UNUSED)
 
 static start_bg_wait_cb bg_wait_cb;
 
-static int bg_wait_cb(const struct child_process *cp, void *cb_data)
+static int bg_wait_cb(const struct child_process *cp UNUSED,
+		      void *cb_data UNUSED)
 {
 	enum ipc_active_state s = fsmonitor_ipc__get_state();
 
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 3d1436da59..941ae7e3bc 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -278,7 +278,8 @@ static int daemon__run_server(void)
 
 static start_bg_wait_cb bg_wait_cb;
 
-static int bg_wait_cb(const struct child_process *cp, void *cb_data)
+static int bg_wait_cb(const struct child_process *cp UNUSED,
+		      void *cb_data UNUSED)
 {
 	int s = ipc_get_active_state(cl_args.path);
 
-- 
2.42.0.671.g43fbf3903a

Re: [PATCH 0/8] fsmonitor unused parameter cleanups

[Jeff Hostetler]

On 9/18/23 6:29 PM, Jeff King wrote:
> Here are a few cleanups of the fsmonitor code to remove or annotate
> unused parameters (working towards my goal of making us compile clean
> with -Wunused-parameter). I think they should all be pretty
> non-controversial, but I'm cc-ing folks active in the area in case patch
> 2 steps on the toes of any unpublished works in progress.
> 
>    [1/8]: fsmonitor: prefer repo_git_path() to git_pathdup()
>    [2/8]: fsmonitor/win32: drop unused parameters
>    [3/8]: fsmonitor: mark some maybe-unused parameters
>    [4/8]: fsmonitor/win32: mark unused parameter in fsm_os__incompatible()
>    [5/8]: fsmonitor: mark unused parameters in stub functions
>    [6/8]: fsmonitor/darwin: mark unused parameters in system callback
>    [7/8]: fsmonitor: mark unused hashmap callback parameters
>    [8/8]: run-command: mark unused parameters in start_bg_wait callbacks
> 
>   builtin/fsmonitor--daemon.c             | 10 ++++++----
>   compat/fsmonitor/fsm-health-darwin.c    |  8 ++++----
>   compat/fsmonitor/fsm-ipc-win32.c        |  2 +-
>   compat/fsmonitor/fsm-listen-darwin.c    |  4 ++--
>   compat/fsmonitor/fsm-listen-win32.c     | 24 ++++++++++--------------
>   compat/fsmonitor/fsm-path-utils-win32.c |  7 ++++---
>   compat/fsmonitor/fsm-settings-win32.c   |  2 +-
>   fsmonitor-ipc.c                         | 10 +++++-----
>   fsmonitor-settings.c                    |  3 ++-
>   t/helper/test-simple-ipc.c              |  3 ++-
>   10 files changed, 37 insertions(+), 36 deletions(-)
> 
> -Peff

LGTM

Thanks,
Jeff

Re: [PATCH 0/8] fsmonitor unused parameter cleanups

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> On 9/18/23 6:29 PM, Jeff King wrote:
>> Here are a few cleanups of the fsmonitor code to remove or annotate
>> unused parameters (working towards my goal of making us compile clean
> ...
> LGTM
>
> Thanks,
> Jeff

Thanks, both.  Let's merge it down.