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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 23, 2021, 18:37 UTC
Message-ID: <xmqqczozyw8i.fsf@gitster.g>
In-Reply-To: <87r1df497s.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 17 quoted lines

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

I have no comment on the "default" thing, but I agree that the preimage does look _unusual_ in our codebase. You cannot even declare a "variable" of that type with the typedef, i.e.

	start_bg_wait_cb an_instance_of_that;

If there is a good reason behind choosing the unusual "the type of the function is...", that is OK, but otherwise...