Re: [PATCH v6 3/6] run-command: make `exists_in_PATH()` non-static

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 2, 2021, 22:19 UTC
Message-ID: <xmqqk0jyipt1.fsf@gitster.g>
In-Reply-To: <20210902090421.93113-4-mirucam@gmail.com>

Miriam Rubio <mirucam@gmail.com> writes:

> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Removes the `static` keyword from `exists_in_PATH()` function
> and declares the function in `run-command.h` file.

"Remove" and "declare", as if we are giving an order to somebody else to make these changes.

Show 35 quoted lines

> The function will be used in bisect_visualize() in a later
> commit.
>
> Mentored by: Christian Couder <chriscool@tuxfamily.org>
> Mentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>  run-command.c |  2 +-
>  run-command.h | 12 ++++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index f72e72cce7..390f46819f 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -210,7 +210,7 @@ static char *locate_in_PATH(const char *file)
>  	return NULL;
>  }
>  
> -static int exists_in_PATH(const char *file)
> +int exists_in_PATH(const char *file)
>  {
>  	char *r = locate_in_PATH(file);
>  	int found = r != NULL;
> diff --git a/run-command.h b/run-command.h
> index af1296769f..54d74b706f 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -182,6 +182,18 @@ void child_process_clear(struct child_process *);
>  
>  int is_executable(const char *name);
>  
> +/**
> + * Search if a $PATH for a command exists.  This emulates the path search that

The first sentence does not make sense to me. Isn't this for checking if a command exists in one of the directories on $PATH?

	Check if the command exists on $PATH.

may make more sense, especially since "search" may hint that the caller may be able to learn where it exists, which is not the case.

Show 5 quoted lines

> + * execvp would perform, without actually executing the command so it
> + * can be used before fork() to prepare to run a command using
> + * execve() or after execvp() to diagnose why it failed.
> + *
> + * The caller should ensure that file contains no directory separators.

Consistently use "command" instead of "file" and rename the parameter in the prototype below from "file" to "command".

Alternatively, you can rewrite the first paragraph above to make sure that it is clear to the readers that "command" it refers to is actually the "file" parameter the function takes. A rewrite of the first sentence I just rewrote above may become

	Check if an executable "file" exists on $PATH.

which does not look too bad, but "executing the file so it can ..." and "to run a file using..." smell a bit strange, and that is why I suggested to consistently use "command" instead.

> + *
> + * Returns 1 if it is found in $PATH or 0 if the command could not be found.
> + */
> +int exists_in_PATH(const char *file);

Thanks.