Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH

From: Jeff King <peff@peff.net>
Date: Jan 7, 2020, 11:08 UTC
Message-ID: <20200107110850.GB1073219@coredump.intra.peff.net>
In-Reply-To: <20200107020425.GG92456@google.com>

On Mon, Jan 06, 2020 at 06:04:25PM -0800, Jonathan Nieder wrote:

Show 20 quoted lines

> -- >8 --
> Subject: run-command: let caller pass in buffer to locate_in_PATH
> 
> Instead of returning a buffer that the caller is responsible for
> freeing, use a strbuf output parameter to record the path to the
> searched-for program.
> 
> This makes ownership a little easier to reason about, since the owning
> code declares the buffer.  It's a good habit to follow because it
> allows buffer reuse when calling such a function in a loop.
> 
> It also allows the caller exists_in_PATH that does not care about the
> path to the command to be slightly simplified, by allowing a NULL
> output parameter that means that locate_in_PATH should take care of
> allocating and freeing its temporary buffer.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  run-command.c | 51 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 29 insertions(+), 22 deletions(-)

I dunno. Now the rules inside locate_in_PATH() are more complicated, and we have an unusual boolean return from the function.

I admit I don't overly care either way, as there are literally two callers (and not likely to be more). So I'd probably just leave it alone, but I'm not opposed to the patch if people think it's a good idea.

-Peff