Re: [PATCH v3 04/13] run-command: make `exists_in_PATH()` non-static

[Taylor Blau <me@ttaylorr.com>]

On Sat, Feb 08, 2020 at 10:06:55AM +0100, Miriam Rubio wrote:
> 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.
> The function will be used in bisect_visualize() in a later
> commit.

There may be some odd line-wrapping going on here.

> `exists_in_PATH()` and `locate_in_PATH()` functions don't
> depend on file-local variables.
>
> 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 | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index f5e1149f9b..4df975178d 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 592d9dc035..7c8e206d97 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -172,6 +172,17 @@ void child_process_clear(struct child_process *);
>
>  int is_executable(const char *name);
>
> +/**
> + * Returns if a $PATH given by parameter is found or not (it is NULL). This
> + * function uses locate_in_PATH() function that emulates the path search that
> + * execvp would perform. Memory used to store the resultant path is freed by
> + * the function.
> + *
> + * The caller should ensure that $PATH contains no directory
> + * separators.

This line-wrapping caught my eye: it looks like 'separators' would fit
on the line before, or at least that this paragraph and the one above it
are wrapped at two different widths.

I'm not sure that it really matters, since it looks like the wrapping in
this file isn't entirely consistent, but I figured that I'd point it out
nonetheless.

> + */
> +int exists_in_PATH(const char *);

Have you considered naming this argument? It's somewhat clear from the
documentation, but I don't see a reason not to name it (especially since
other declarations in this file *do* name their parameters). What about:

  int exists_in_PATH(const char *file);

To match its definition in 'run-command.c'? (Admittedly, if I had
written this I'd probably call it 'entry', but they should at least be
consistent).

> +
>  /**
>   * Start a sub-process. Takes a pointer to a `struct child_process`
>   * that specifies the details and returns pipe FDs (if requested).
> --
> 2.25.0
>

Thanks,
Taylor