Fix dir sep handling of GIT_ASKPASS on Windows
diff mbox series

Message ID pull.587.git.1584997990694.gitgitgadget@gmail.com
State New
Headers show
Series
  • Fix dir sep handling of GIT_ASKPASS on Windows
Related show

Commit Message

Elijah Newren via GitGitGadget March 23, 2020, 9:13 p.m. UTC
From: Andras Kucsma <r0maikx02b@gmail.com>

On Windows with git installed through cygwin, GIT_ASKPASS failed to run
for relative and absolute paths containing only backslashes as directory
separators.

The reason was that git assumed that if there are no forward slashes in
the executable path, it has to search for the executable on the PATH.

The fix is to look for OS specific directory separators, not just
forward slashes.

Signed-off-by: Andras Kucsma <r0maikx02b@gmail.com>
---
    Fix dir sep handling of GIT_ASKPASS on Windows
    
    On Windows with git installed through cygwin, GIT_ASKPASS failed to run
    for relative and absolute paths containing only backslashes as directory
    separators.
    
    The reason was that git assumed that if there are no forward slashes in
    the executable path, it has to search for the executable on the PATH.
    
    The fix is to look for OS specific directory separators, not just
    forward slashes.
    
    Signed-off-by: Andras Kucsma r0maikx02b@gmail.com [r0maikx02b@gmail.com]
    
    CC: Torsten Bögershausen tboegi@web.de [tboegi@web.de]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-587%2Fr0mai%2Ffix-prepare_cmd-windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-587/r0mai/fix-prepare_cmd-windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/587

 run-command.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925

Comments

Junio C Hamano March 24, 2020, 8:51 p.m. UTC | #1
"András Kucsma via GitGitGadget"  <gitgitgadget@gmail.com> writes:

> From: Andras Kucsma <r0maikx02b@gmail.com>
>
> On Windows with git installed through cygwin, GIT_ASKPASS failed to run
> for relative and absolute paths containing only backslashes as directory
> separators.
>
> The reason was that git assumed that if there are no forward slashes in
> the executable path, it has to search for the executable on the PATH.

Also if I were reading the discussion correctly, there was a doubt
about locate_in_PATH() that may not work on Windows for at least two
reasons.  Is it OK to ignore these issues, and if so why?

I know if you have a full path, a broken locate_in_PATH() would be
skipped and won't cause an immediate issue, but this change to make
the code realize that "a\\b" is not asking to search in %PATH% feels
just a beginning of a fix, not the whole fix, at least to me.

> The fix is to look for OS specific directory separators, not just
> forward slashes.

Yes, but it is quite unfortunate that you would use a function that
has to scan the string to the end because it asks for the last one.

Perhaps introduce 

--------------------------------------------------
#ifndef has_dir_sep
static inline int git_has_dir_sep(const char *path)
{
	return !!strchr(path, '/');
}
#define has_dir_sep(path) git_has_dir_sep(path)
#endif
--------------------------------------------------

in <git-compat-util.h>, with a replacement definition in
<compat/win32/path-utils.h> that may read

--------------------------------------------------
#define has_dir_sep(path) win32_has_dir_sep(path)
static inline int has_dir_sep(const char *path)
{
        /* 
         * See how long the non-separator part of the given path is, and
         * if and only if it covers the whole path (i.e. path[len] is NUL),
         * there is no separator in the path---otherwise there is a separaptor.
         */
        size_t len = strcspn(path, "/\\");
        return !!path[len];
}
--------------------------------------------------

and use that instead?


> diff --git a/run-command.c b/run-command.c
> index f5e1149f9b3..9fcc12ebf9c 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -421,12 +421,12 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd)
>  	}
>  
>  	/*
> -	 * If there are no '/' characters in the command then perform a path
> -	 * lookup and use the resolved path as the command to exec.  If there
> -	 * are '/' characters, we have exec attempt to invoke the command
> -	 * directly.
> +	 * If there are no dir separator characters in the command then perform
> +	 * a path lookup and use the resolved path as the command to exec. If
> +	 * there are dir separator characters, we have exec attempt to invoke
> +	 * the command directly.
>  	 */
> -	if (!strchr(out->argv[1], '/')) {
> +	if (find_last_dir_sep(out->argv[1]) == NULL) {
>  		char *program = locate_in_PATH(out->argv[1]);
>  		if (program) {
>  			free((char *)out->argv[1]);
>
> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925

Patch
diff mbox series

diff --git a/run-command.c b/run-command.c
index f5e1149f9b3..9fcc12ebf9c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -421,12 +421,12 @@  static int prepare_cmd(struct argv_array *out, const struct child_process *cmd)
 	}
 
 	/*
-	 * If there are no '/' characters in the command then perform a path
-	 * lookup and use the resolved path as the command to exec.  If there
-	 * are '/' characters, we have exec attempt to invoke the command
-	 * directly.
+	 * If there are no dir separator characters in the command then perform
+	 * a path lookup and use the resolved path as the command to exec. If
+	 * there are dir separator characters, we have exec attempt to invoke
+	 * the command directly.
 	 */
-	if (!strchr(out->argv[1], '/')) {
+	if (find_last_dir_sep(out->argv[1]) == NULL) {
 		char *program = locate_in_PATH(out->argv[1]);
 		if (program) {
 			free((char *)out->argv[1]);