[PATCH v3] run-command: trigger PATH lookup properly on Cygwin

From: András Kucsma via GitGitGadget <gitgitgadget@gmail.com>
Date: Mar 27, 2020, 00:36 UTC
Message-ID: <pull.587.v3.git.1585269403947.gitgitgadget@gmail.com>
In-Reply-To: <pull.587.v2.git.1585143910604.gitgitgadget@gmail.com>

From: Andras Kucsma <r0maikx02b@gmail.com>

On Cygwin, the codepath for POSIX-like systems is taken in run-command.c::start_command(). The prepare_cmd() helper function is called to decide if the command needs to be looked up in the PATH. The logic there is to do the PATH-lookup if and only if it does not have any slash '/' in it. If this test passes we end up attempting to run the command by appending the string after each colon-separated component of PATH.

The Cygwin environment supports both Windows and POSIX style paths, so both forwardslahes '/' and back slashes '\' can be used as directory separators for any external program the user supplies.

Examples for path strings which are being incorrectly searched for in the PATH instead of being executed as is:

- "C:\Program Files\some-program.exe"
- "a\b\c.exe"

To handle these, the PATH lookup detection logic in prepare_cmd() is taught to know about this Cygwin quirk, by introducing has_dir_sep(path) helper function to abstract away the difference between true POSIX and Cygwin systems.

Signed-off-by: Andras Kucsma <r0maikx02b@gmail.com>
---
    run-command: trigger PATH lookup properly on Cygwin
    
    On Cygwin, the codepath for POSIX-like systems is taken in
    run-command.c::start_command(). The prepare_cmd() helper function is
    called to decide if the command needs to be looked up in the PATH. The
    logic there is to do the PATH-lookup if and only if it does not have any
    slash '/' in it. If this test passes we end up attempting to run the
    command by appending the string after each colon-separated component of
    PATH.
    
    The Cygwin environment supports both Windows and POSIX style paths, so
    both forwardslahes '/' and back slashes '' can be used as directory
    separators for any external program the user supplies.
    
    Examples for path strings which are being incorrectly searched for in
    the PATH instead of being executed as is:
    
     * "C:\Program Files\some-program.exe"
     * "a\b\c.exe"
    
    To handle these, the PATH lookup detection logic in prepare_cmd() is
    taught to know about this Cygwin quirk, by introducing has_dir_sep(path)
    helper function to abstract away the difference between true POSIX and
    Cygwin systems.
    
    Signed-off-by: Andras Kucsma r0maikx02b@gmail.com [r0maikx02b@gmail.com]
    
    Changes since v1:
    
     * Avoid scanning the whole path for a directory separator even if one
       is found earlier as suggested by Junio C Hamano. Changes since v2:
     * Rephrased the commit message based on Junio's suggestion.

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

Range-diff vs v2:

 1:  947931ac568 ! 1:  fd3cbd51635 Fix dir sep handling of GIT_ASKPASS on Windows
     @@ -1,16 +1,30 @@
      Author: Andras Kucsma <r0maikx02b@gmail.com>
      
     -    Fix dir sep handling of GIT_ASKPASS on Windows
     +    run-command: trigger PATH lookup properly on Cygwin
      
     -    On Windows with git installed through cygwin, GIT_ASKPASS failed to run
     -    for relative and absolute paths containing only backslashes as directory
     -    separators.
     +    On Cygwin, the codepath for POSIX-like systems is taken in
     +    run-command.c::start_command(). The prepare_cmd() helper
     +    function is called to decide if the command needs to be looked
     +    up in the PATH. The logic there is to do the PATH-lookup if
     +    and only if it does not have any slash '/' in it. If this test
     +    passes we end up attempting to run the command by appending the
     +    string after each colon-separated component of PATH.
      
     -    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 Cygwin environment supports both Windows and POSIX style
     +    paths, so both forwardslahes '/' and back slashes '\' can be
     +    used as directory separators for any external program the user
     +    supplies.
      
     -    The fix is to look for OS specific directory separators, not just
     -    forward slashes.
     +    Examples for path strings which are being incorrectly searched
     +    for in the PATH instead of being executed as is:
     +
     +    - "C:\Program Files\some-program.exe"
     +    - "a\b\c.exe"
     +
     +    To handle these, the PATH lookup detection logic in prepare_cmd()
     +    is taught to know about this Cygwin quirk, by introducing
     +    has_dir_sep(path) helper function to abstract away the difference
     +    between true POSIX and Cygwin systems.
      
          Signed-off-by: Andras Kucsma <r0maikx02b@gmail.com>

 compat/win32/path-utils.h | 11 +++++++++++
 git-compat-util.h         |  8 ++++++++
 run-command.c             | 10 +++++-----
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h
index f2e70872cd2..18eff7899e9 100644
--- a/compat/win32/path-utils.h
+++ b/compat/win32/path-utils.h
@@ -20,6 +20,17 @@ static inline char *win32_find_last_dir_sep(const char *path)
 	return ret;
 }
 #define find_last_dir_sep win32_find_last_dir_sep
+static inline int win32_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 NULL),
+	 * there is no separator in the path---otherwise there is a separator.
+	 */
+	size_t len = strcspn(path, "/\\");
+	return !!path[len];
+}
+#define has_dir_sep(path) win32_has_dir_sep(path)
 int win32_offset_1st_component(const char *path);
 #define offset_1st_component win32_offset_1st_component
 
diff --git a/git-compat-util.h b/git-compat-util.h
index aed0b5d4f90..8ba576e81e3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -389,6 +389,14 @@ static inline char *git_find_last_dir_sep(const char *path)
 #define find_last_dir_sep git_find_last_dir_sep
 #endif
 
+#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
+
 #ifndef query_user_email
 #define query_user_email() NULL
 #endif
diff --git a/run-command.c b/run-command.c
index f5e1149f9b3..0f41af3b550 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 (!has_dir_sep(out->argv[1])) {
 		char *program = locate_in_PATH(out->argv[1]);
 		if (program) {
 			free((char *)out->argv[1]);

base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925

-- 
gitgitgadget