On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:
> + 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@xxxxxxxxxxxxxxxxxxx/
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");
}
/*
