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

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 9, 2020, 23:16 UTC
Message-ID: <20200209231602.GC4530@syl.local>
In-Reply-To: <20200208090704.26506-5-mirucam@gmail.com>

On Sat, Feb 08, 2020 at 10:06:55AM +0100, Miriam Rubio wrote:

Show 6 quoted lines

> 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.

Show 41 quoted lines

> `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).

Show 7 quoted lines

> +
>  /**
>   * 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