Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 23, 2021, 15:03 UTC
Message-ID: <87r1df497s.fsf@evledraar.gmail.com>
In-Reply-To: <6b7a058284b93fab52458b12a6aede5e8aed6652.1632152179.git.gitgitgadget@gmail.com>

On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:

Show 23 quoted lines

> +	switch (sbgr) {
> +	case SBGR_READY:
> +		return 0;
>  
> -		else if (pid_seen == pid_child) {
> -			/*
> -			 * The new child daemon process shutdown while
> -			 * it was starting up, so it is not listening
> -			 * on the socket.
> -			 *
> -			 * Try to ping the socket in the odd chance
> -			 * that another daemon started (or was already
> -			 * running) while our child was starting.
> -			 *
> -			 * Again, we don't care who services the socket.
> -			 */
> -			s = ipc_get_active_state(cl_args.path);
> -			if (s == IPC_STATE__LISTENING)
> -				return 0;
> +	default:
> +	case SBGR_ERROR:
> +	case SBGR_CB_ERROR:
> +		return error("daemon failed to start");

There was a discussion on v1 about the "default" being redundant here and hiding future compiler checks, this is another "not sure what you thought of that" case (per [1]).

Interestingly in this case if I drop the "default" my local gcc uncharacteristically complains about a missing "return" in this function, but clang doesn't. I needed to add a BUG() to shut up the former. Maybe I'm wrong, but perhaps it's a sign of some deeper trouble. This is with gcc/clang 10.2.1-6/11.0.1-2.

1. https://lore.kernel.org/git/87v92r49mt.fsf@evledraar.gmail.com/

I played with the diff below on top of this, I can't remember if it was noted already, but the way you declare function ptrs and use them isn't the usual style:

-- >8 --

diff --git a/run-command.c b/run-command.c
index 76bbef9d96d..5c831545201 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1903,7 +1903,7 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
 }
 
 enum start_bg_result start_bg_command(struct child_process *cmd,
-				      start_bg_wait_cb *wait_cb,
+				      start_bg_wait_cb wait_cb,
 				      void *cb_data,
 				      unsigned int timeout_sec)
 {
diff --git a/run-command.h b/run-command.h
index 17b1b80c3d7..e8a637d1967 100644
--- a/run-command.h
+++ b/run-command.h
@@ -527,7 +527,7 @@ enum start_bg_result {
  * Returns 0 if child is "ready".
  * Returns -1 on any error and cause start_bg_command() to also error out.
  */
-typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);
+typedef int (*start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);
 
 /**
  * Start a command in the background.  Wait long enough for the child
@@ -549,7 +549,7 @@ typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);
  * any instance data that it might require.  This may be NULL.
  */
 enum start_bg_result start_bg_command(struct child_process *cmd,
-				      start_bg_wait_cb *wait_cb,
+				      start_bg_wait_cb wait_cb,
 				      void *cb_data,
 				      unsigned int timeout_sec);
 
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 28365ff85b6..82dc2906a2a 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -275,9 +275,7 @@ static int daemon__run_server(void)
 	return ret;
 }
 
-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, void *cb_data, int foo)
 {
 	int s = ipc_get_active_state(cl_args.path);
 
@@ -319,9 +317,8 @@ static int daemon__start_server(void)
 	switch (sbgr) {
 	case SBGR_READY:
 		return 0;
-
-	default:
 	case SBGR_ERROR:
+		return 0;
 	case SBGR_CB_ERROR:
 		return error("daemon failed to start");
 
@@ -331,6 +328,7 @@ static int daemon__start_server(void)
 	case SBGR_DIED:
 		return error("daemon terminated");
 	}
+	BUG("unreachable");
 }
 
 /*