Re: [PATCH 2/3] run-command: teach locate_in_PATH about Windows

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 3, 2023, 16:13 UTC
Message-ID: <xmqq7cqc5cjf.fsf@gitster.g>
In-Reply-To: <bf8b34aaef32a64b85f778ab219aeb41238f2bf2.1691058498.git.gitgitgadget@gmail.com>

"Matthias Aßhauer via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 27 quoted lines

> diff --git a/run-command.c b/run-command.c
> index 60c94198664..8f518e37e27 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -182,13 +182,10 @@ int is_executable(const char *name)
>   * Returns the path to the command, as found in $PATH or NULL if the
>   * command could not be found.  The caller inherits ownership of the memory
>   * used to store the resultant path.
> - *
> - * This should not be used on Windows, where the $PATH search rules
> - * are more complicated (e.g., a search for "foo" should find
> - * "foo.exe").
>   */
>  static char *locate_in_PATH(const char *file)
>  {
> +#ifndef GIT_WINDOWS_NATIVE
>  	const char *p = getenv("PATH");
>  	struct strbuf buf = STRBUF_INIT;
>  
> @@ -217,6 +214,9 @@ static char *locate_in_PATH(const char *file)
>  
>  	strbuf_release(&buf);
>  	return NULL;
> +#else
> +	return mingw_path_lookup(file,0);
> +#endif
>  }

It may be cleaner to make the above more like

	#ifndef locate_in_PATH
	static char *locate_in_PATH(const char *file)
	{
	    ... original implementation without any #ifdef ...
	}
	#endif

and redo the [1/3] patch so that it does not rename or otherwise touch path_lookup() in any way, and instead implements a mingw_locate_in_PATH() in terms of path_lookup() and make it public, declare it in <compat/mingw.h>, together with #define locate_in_PATH(), i.e. [1/3] will essentially become something like:

    (add to compat/mingw.c)
    char *mingw_locate_in_PATH(const char *file)
    {
	return path_lookup(file, 0);
    }

    (add to compat/mingw.h)
    extern char *mingw_locate_in_PATH(const char *);
    #define locate_in_PATH(file) mingw_locate_in_PATH(file)

That way, the second non-UNIXy system can add its own way to locate an executable in PATH without having to touch the main part of the system, right?