Merge #9516 from erw7/improve-executable-on-windows

Improve executable() and exepath() on windows

4 files changed, 126 insertions, 67 deletions

diff --git a/src/nvim/os/fs.c b/src/nvim/os/fs.c
index 27db675c52..8d9de1253e 100644
--- a/src/nvim/os/fs.c
+++ b/src/nvim/os/fs.c
@@ -226,13 +226,13 @@ int os_exepath(char *buffer, size_t *size)
   return uv_exepath(buffer, size);
 }
 
-/// Checks if the given path represents an executable file.
+/// Checks if the file `name` is executable.
 ///
-/// @param[in]  name     Name of the executable.
-/// @param[out] abspath  Path of the executable, if found and not `NULL`.
-/// @param[in] use_path  If 'false', only check if "name" is executable
+/// @param[in]  name     Filename to check.
+/// @param[out] abspath  Returns resolved executable path, if not NULL.
+/// @param[in] use_path  Also search $PATH.
 ///
-/// @return `true` if `name` is executable and
+/// @return true if `name` is executable and
 ///   - can be found in $PATH,
 ///   - is relative to current dir or
 ///   - is absolute.
@@ -242,40 +242,36 @@ bool os_can_exe(const char_u *name, char_u **abspath, bool use_path)
   FUNC_ATTR_NONNULL_ARG(1)
 {
   bool no_path = !use_path || path_is_absolute(name);
-#ifndef WIN32
   // If the filename is "qualified" (relative or absolute) do not check $PATH.
+#ifdef WIN32
+  no_path |= (name[0] == '.'
+              && ((name[1] == '/' || name[1] == '\\')
+                  || (name[1] == '.' && (name[2] == '/' || name[2] == '\\'))));
+#else
   no_path |= (name[0] == '.'
               && (name[1] == '/' || (name[1] == '.' && name[2] == '/')));
 #endif
 
   if (no_path) {
 #ifdef WIN32
-    const char *pathext = os_getenv("PATHEXT");
-    if (!pathext) {
-      pathext = ".com;.exe;.bat;.cmd";
-    }
-    bool ok = is_executable((char *)name) || is_executable_ext((char *)name,
-                                                               pathext);
+    if (is_executable_ext((char *)name, abspath)) {
 #else
     // Must have path separator, cannot execute files in the current directory.
-    const bool ok = ((const char_u *)gettail_dir((const char *)name) != name
-                     && is_executable((char *)name));
+    if ((const char_u *)gettail_dir((const char *)name) != name
+        && is_executable((char *)name, abspath)) {
 #endif
-    if (ok) {
-      if (abspath != NULL) {
-        *abspath = save_abs_path(name);
-      }
       return true;
+    } else {
+      return false;
     }
-    return false;
   }
 
   return is_executable_in_path(name, abspath);
 }
 
 /// Returns true if `name` is an executable file.
-static bool is_executable(const char *name)
-  FUNC_ATTR_NONNULL_ALL
+static bool is_executable(const char *name, char_u **abspath)
+  FUNC_ATTR_NONNULL_ARG(1)
 {
   int32_t mode = os_getperm((const char *)name);
 
@@ -286,40 +282,58 @@ static bool is_executable(const char *name)
 #ifdef WIN32
   // Windows does not have exec bit; just check if the file exists and is not
   // a directory.
-  return (S_ISREG(mode));
+  const bool ok = S_ISREG(mode);
 #else
   int r = -1;
   if (S_ISREG(mode)) {
     RUN_UV_FS_FUNC(r, uv_fs_access, name, X_OK, NULL);
   }
-  return (r == 0);
+  const bool ok = (r == 0);
 #endif
+  if (ok && abspath != NULL) {
+    *abspath = save_abs_path((char_u *)name);
+  }
+  return ok;
 }
 
 #ifdef WIN32
-/// Appends file extensions from `pathext` to `name` and returns true if any
-/// such combination is executable.
-static bool is_executable_ext(char *name, const char *pathext)
-  FUNC_ATTR_NONNULL_ALL
+/// Checks if file `name` is executable under any of these conditions:
+/// - extension is in $PATHEXT and `name` is executable
+/// - result of any $PATHEXT extension appended to `name` is executable
+static bool is_executable_ext(char *name, char_u **abspath)
+  FUNC_ATTR_NONNULL_ARG(1)
 {
+  const bool is_unix_shell = strstr((char *)path_tail(p_sh), "sh") != NULL;
+  char *nameext = strrchr(name, '.');
+  size_t nameext_len = nameext ? strlen(nameext) : 0;
   xstrlcpy(os_buf, name, sizeof(os_buf));
   char *buf_end = xstrchrnul(os_buf, '\0');
+  const char *pathext = os_getenv("PATHEXT");
+  if (!pathext) {
+    pathext = ".com;.exe;.bat;.cmd";
+  }
   for (const char *ext = pathext; *ext; ext++) {
-    // Skip the extension if there is no suffix after a '.'.
+    // If $PATHEXT itself contains dot:
     if (ext[0] == '.' && (ext[1] == '\0' || ext[1] == ENV_SEPCHAR)) {
+      if (is_executable(name, abspath)) {
+        return true;
+      }
+      // Skip it.
       ext++;
       continue;
     }
 
     const char *ext_end = xstrchrnul(ext, ENV_SEPCHAR);
-    STRLCPY(buf_end, ext, ext_end - ext + 1);
-
-    if (is_executable(os_buf)) {
-      return true;
-    }
-
-    if (*ext_end != ENV_SEPCHAR) {
-      break;
+    size_t ext_len = (size_t)(ext_end - ext);
+    if (ext_len != 0) {
+      STRLCPY(buf_end, ext, ext_len + 1);
+      bool in_pathext = nameext_len == ext_len
+        && 0 == mb_strnicmp((char_u *)nameext, (char_u *)ext, ext_len);
+
+      if (((in_pathext || is_unix_shell) && is_executable(name, abspath))
+          || is_executable(os_buf, abspath)) {
+        return true;
+      }
     }
     ext = ext_end;
   }
@@ -327,10 +341,10 @@ static bool is_executable_ext(char *name, const char *pathext)
 }
 #endif
 
-/// Checks if a file is inside the `$PATH` and is executable.
+/// Checks if a file is in `$PATH` and is executable.
 ///
-/// @param[in]  name The name of the executable.
-/// @param[out] abspath  Path of the executable, if found and not `NULL`.
+/// @param[in]  name  Filename to check.
+/// @param[out] abspath  Returns resolved executable path, if not NULL.
 ///
 /// @return `true` if `name` is an executable inside `$PATH`.
 static bool is_executable_in_path(const char_u *name, char_u **abspath)
@@ -351,15 +365,6 @@ static bool is_executable_in_path(const char_u *name, char_u **abspath)
 #endif
 
   size_t buf_len = STRLEN(name) + strlen(path) + 2;
-
-#ifdef WIN32
-  const char *pathext = os_getenv("PATHEXT");
-  if (!pathext) {
-    pathext = ".com;.exe;.bat;.cmd";
-  }
-  buf_len += strlen(pathext);
-#endif
-
   char *buf = xmalloc(buf_len);
 
   // Walk through all entries in $PATH to check if "name" exists there and
@@ -374,15 +379,10 @@ static bool is_executable_in_path(const char_u *name, char_u **abspath)
     append_path(buf, (char *)name, buf_len);
 
 #ifdef WIN32
-    bool ok = is_executable(buf) || is_executable_ext(buf, pathext);
+    if (is_executable_ext(buf, abspath)) {
 #else
-    bool ok = is_executable(buf);
+    if (is_executable(buf, abspath)) {
 #endif
-    if (ok) {
-      if (abspath != NULL) {  // Caller asked for a copy of the path.
-        *abspath = save_abs_path((char_u *)buf);
-      }
-
       rv = true;
       goto end;
     }
diff --git a/src/nvim/testdir/test_functions.vim b/src/nvim/testdir/test_functions.vim
index 13e2dcf804..46c2d0f4cd 100644
--- a/src/nvim/testdir/test_functions.vim
+++ b/src/nvim/testdir/test_functions.vim
@@ -881,8 +881,8 @@ func Test_Executable()
     call assert_equal(1, executable('notepad'))
     call assert_equal(1, executable('notepad.exe'))
     call assert_equal(0, executable('notepad.exe.exe'))
-    call assert_equal(1, executable('shell32.dll'))
-    call assert_equal(1, executable('win.ini'))
+    call assert_equal(0, executable('shell32.dll'))
+    call assert_equal(0, executable('win.ini'))
   elseif has('unix')
     call assert_equal(1, executable('cat'))
     call assert_equal(0, executable('nodogshere'))
diff --git a/test/functional/eval/executable_spec.lua b/test/functional/eval/executable_spec.lua
index c931b47221..6a95128a4d 100644
--- a/test/functional/eval/executable_spec.lua
+++ b/test/functional/eval/executable_spec.lua
@@ -1,6 +1,7 @@
 local helpers = require('test.functional.helpers')(after_each)
-local eq, clear, call, iswin, write_file =
-  helpers.eq, helpers.clear, helpers.call, helpers.iswin, helpers.write_file
+local eq, clear, call, iswin, write_file, command =
+  helpers.eq, helpers.clear, helpers.call, helpers.iswin, helpers.write_file,
+  helpers.command
 
 describe('executable()', function()
   before_each(clear)
@@ -48,18 +49,17 @@ describe('executable()', function()
     end)
 
     it('not set', function()
-      local expected = iswin() and 1 or 0
-      eq(expected, call('executable', 'Xtest_not_executable'))
-      eq(expected, call('executable', './Xtest_not_executable'))
+      eq(0, call('executable', 'Xtest_not_executable'))
+      eq(0, call('executable', './Xtest_not_executable'))
     end)
 
     it('set, unqualified and not in $PATH', function()
-      local expected = iswin() and 1 or 0
-      eq(expected, call('executable', 'Xtest_executable'))
+      eq(0, call('executable', 'Xtest_executable'))
     end)
 
     it('set, qualified as a path', function()
-      eq(1, call('executable', './Xtest_executable'))
+      local expected = iswin() and 0 or 1
+      eq(expected, call('executable', './Xtest_executable'))
     end)
   end)
 end)
@@ -136,16 +136,48 @@ describe('executable() (Windows)', function()
     eq(1, call('executable', '.\\test_executable_zzz'))
   end)
 
-  it('returns 1 for any existing filename', function()
+  it("with weird $PATHEXT", function()
+    clear({env={PATHEXT=';'}})
+    eq(0, call('executable', '.\\test_executable_zzz'))
+    clear({env={PATHEXT=';;;.zzz;;'}})
+    eq(1, call('executable', '.\\test_executable_zzz'))
+  end)
+
+  it("unqualified filename, Unix-style 'shell'", function()
     clear({env={PATHEXT=''}})
+    command('set shell=sh')
     for _,ext in ipairs(exts) do
       eq(1, call('executable', 'test_executable_'..ext..'.'..ext))
     end
     eq(1, call('executable', 'test_executable_zzz.zzz'))
   end)
 
-  it('returns 1 for any existing path (backslashes)', function()
+  it("relative path, Unix-style 'shell' (backslashes)", function()
     clear({env={PATHEXT=''}})
+    command('set shell=bash.exe')
+    for _,ext in ipairs(exts) do
+      eq(1, call('executable', '.\\test_executable_'..ext..'.'..ext))
+      eq(1, call('executable', './test_executable_'..ext..'.'..ext))
+    end
+    eq(1, call('executable', '.\\test_executable_zzz.zzz'))
+    eq(1, call('executable', './test_executable_zzz.zzz'))
+  end)
+
+  it('unqualified filename, $PATHEXT contains dot', function()
+    clear({env={PATHEXT='.;.zzz'}})
+    for _,ext in ipairs(exts) do
+      eq(1, call('executable', 'test_executable_'..ext..'.'..ext))
+    end
+    eq(1, call('executable', 'test_executable_zzz.zzz'))
+    clear({env={PATHEXT='.zzz;.'}})
+    for _,ext in ipairs(exts) do
+      eq(1, call('executable', 'test_executable_'..ext..'.'..ext))
+    end
+    eq(1, call('executable', 'test_executable_zzz.zzz'))
+  end)
+
+  it('relative path, $PATHEXT contains dot (backslashes)', function()
+    clear({env={PATHEXT='.;.zzz'}})
     for _,ext in ipairs(exts) do
       eq(1, call('executable', '.\\test_executable_'..ext..'.'..ext))
       eq(1, call('executable', './test_executable_'..ext..'.'..ext))
@@ -153,4 +185,17 @@ describe('executable() (Windows)', function()
     eq(1, call('executable', '.\\test_executable_zzz.zzz'))
     eq(1, call('executable', './test_executable_zzz.zzz'))
   end)
+
+  it('ignores case of extension', function()
+    clear({env={PATHEXT='.ZZZ'}})
+    eq(1, call('executable', 'test_executable_zzz.zzz'))
+  end)
+
+  it('relative path does not search $PATH', function()
+    clear({env={PATHEXT=''}})
+    eq(0, call('executable', './System32/notepad.exe'))
+    eq(0, call('executable', '.\\System32\\notepad.exe'))
+    eq(0, call('executable', '../notepad.exe'))
+    eq(0, call('executable', '..\\notepad.exe'))
+  end)
 end)
diff --git a/test/functional/eval/exepath_spec.lua b/test/functional/eval/exepath_spec.lua
new file mode 100644
index 0000000000..10a11aeacc
--- /dev/null
+++ b/test/functional/eval/exepath_spec.lua
@@ -0,0 +1,14 @@
+local helpers = require('test.functional.helpers')(after_each)
+local eq, clear, call, iswin =
+  helpers.eq, helpers.clear, helpers.call, helpers.iswin
+
+describe('exepath() (Windows)', function()
+  if not iswin() then return end  -- N/A for Unix.
+
+  it('append extension if omitted', function()
+    local filename = 'cmd'
+    local pathext = '.exe'
+    clear({env={PATHEXT=pathext}})
+    eq(call('exepath', filename..pathext), call('exepath', filename))
+  end)
+end)