Improve searching PATH on Windows for completions

This was extracted from https://github.com/elves/elvish/pull/1326. It
augments that change by honoring $E:PATHEXT and some other
minor improvements.  This matches the behavior of the Go stdlib
os/exec.LookPath() function that is used to decide if what has been typed
(in a non-completion context) represents an external command.

pkg/cli/lscolors/feature.go

@@ -2,6 +2,8 @@ package lscolors
 
 import (
 	"os"
+
+	"src.elv.sh/pkg/fsutil"
 )
 
 type feature int
@@ -34,11 +36,10 @@ const (
 	featureRegular
 )
 
-// Weirdly, permission masks for group and other are missing on platforms other
-// than linux, darwin and netbsd. So we replicate some of them here.
 const (
-	worldWritable = 02   // Writable by other
-	executable    = 0111 // Executable
+	// Some platforms, such as Windows, have simulated UNIX style permission masks.
+	// On Windows the only two permission masks are 0o666 (RW) and 0o444 (RO).
+	worldWritable = 0o002
 )
 
 func determineFeature(fname string, mh bool) (feature, error) {
@@ -105,7 +106,7 @@ func determineFeature(fname string, mh bool) (feature, error) {
 		return featureSetuid, nil
 	case is(m, os.ModeSetgid):
 		return featureSetgid, nil
-	case m&executable != 0:
+	case fsutil.IsExecutable(stat):
 		return featureExecutable, nil
 	}
 

pkg/edit/complete/generators.go

@@ -128,7 +128,7 @@ func generateFileNames(seed string, onlyExecutable bool) ([]RawItem, error) {
 	// Make candidates out of elements that match the file component.
 	for _, file := range files {
 		name := file.Name()
-		info, err := file.Info()
+		stat, err := file.Info()
 		if err != nil {
 			continue
 		}
@@ -139,7 +139,7 @@ func generateFileNames(seed string, onlyExecutable bool) ([]RawItem, error) {
 		}
 		// Only accept searchable directories and executable files if
 		// executableOnly is true.
-		if onlyExecutable && (info.Mode()&0111) == 0 {
+		if onlyExecutable && (stat.Mode()&0111) == 0 {
 			continue
 		}
 
@@ -149,13 +149,12 @@ func generateFileNames(seed string, onlyExecutable bool) ([]RawItem, error) {
 		// Will be set to an empty space for non-directories
 		suffix := " "
 
-		if info.IsDir() {
+		if stat.IsDir() {
 			full += pathSeparator
 			suffix = ""
-		} else if info.Mode()&os.ModeSymlink != 0 {
+		} else if stat.Mode()&os.ModeSymlink != 0 {
 			stat, err := os.Stat(full)
-			if err == nil && stat.IsDir() {
-				// Symlink to directory.
+			if err == nil && stat.IsDir() { // symlink to directory
 				full += pathSeparator
 				suffix = ""
 			}

pkg/edit/highlight.go

@@ -100,7 +100,7 @@ func hasFn(ns *eval.Ns, name string) bool {
 
 func isDirOrExecutable(fname string) bool {
 	stat, err := os.Stat(fname)
-	return err == nil && (stat.IsDir() || stat.Mode()&0111 != 0)
+	return err == nil && (stat.IsDir() || fsutil.IsExecutable(stat))
 }
 
 func hasExternalCommand(cmd string) bool {

pkg/fsutil/fsutil_unix.go

@@ -0,0 +1,11 @@
+//go:build !windows && !plan9
+// +build !windows,!plan9
+
+package fsutil
+
+import "os"
+
+// IsExecutable returns true if the stat object refers to an executable file on UNIX platforms.
+func IsExecutable(stat os.FileInfo) bool {
+	return !stat.IsDir() && (stat.Mode()&0o111 != 0)
+}

pkg/fsutil/fsutil_windows.go

@@ -0,0 +1,44 @@
+package fsutil
+
+import (
+	"os"
+	"path"
+	"path/filepath"
+	"strings"
+
+	"src.elv.sh/pkg/env"
+)
+
+// isExecutable determines whether pathStr refers to an executable file in PATH. It honors E:PATHEXT
+// but defaults to extensions ".com", ".exe", ".bat", ".cmd" if that env var isn't set.
+func isExecutable(pathStr string) bool {
+	var validExts []string
+	if pathext := os.Getenv(env.PATHEXT); pathext != "" {
+		for _, e := range strings.Split(strings.ToLower(pathext), string(filepath.ListSeparator)) {
+			if e == "" {
+				continue
+			}
+			if e[0] != '.' {
+				e = "." + e
+			}
+			validExts = append(validExts, e)
+		}
+	} else {
+		validExts = []string{".com", ".exe", ".bat", ".cmd"}
+	}
+
+	ext := strings.ToLower(path.Ext(pathStr))
+	for _, valid := range validExts {
+		if ext == valid {
+			return true
+		}
+	}
+	return false
+}
+
+// IsExecutable returns true if the stat object refers to a valid executable on Windows.
+// Note that on Windows file permissions are not checked but we do validate the file is not a
+// directory and has a recognized extension.
+func IsExecutable(stat os.FileInfo) bool {
+	return !stat.IsDir() && isExecutable(stat.Name())
+}

pkg/fsutil/search.go

@@ -15,38 +15,27 @@ func DontSearch(exe string) bool {
 		strings.ContainsRune(exe, '/')
 }
 
-// IsExecutable determines whether path refers to an executable file.
-func IsExecutable(path string) bool {
-	fi, err := os.Stat(path)
-	if err != nil {
-		return false
-	}
-	fm := fi.Mode()
-	return !fm.IsDir() && (fm&0111 != 0)
-}
-
-// EachExternal calls f for each name that can resolve to an external command.
-//
-// BUG: EachExternal may generate the same command multiple command it it
-// appears in multiple directories in PATH.
+// EachExternal calls f for each executable file found while scanning the directories of $E:PATH.
 //
-// BUG: EachExternal doesn't work on Windows since it relies on the execution
-// permission bit, which doesn't exist on Windows.
+// NOTE: EachExternal may generate the same command multiple times; once for each time it appears in
+// $E:PATH. That is, no deduplication of the files found by scanning $E:PATH is performed.
 func EachExternal(f func(string)) {
 	for _, dir := range searchPaths() {
 		files, err := os.ReadDir(dir)
 		if err != nil {
+			// In practice this rarely happens. There isn't much we can reasonably do when it does
+			// happen other than silently ignore the invalid directory.
 			continue
 		}
 		for _, file := range files {
-			info, err := file.Info()
-			if err == nil && !info.IsDir() && (info.Mode()&0111 != 0) {
-				f(file.Name())
+			stat, err := file.Info()
+			if err == nil && IsExecutable(stat) {
+				f(stat.Name())
 			}
 		}
 	}
 }
 
 func searchPaths() []string {
-	return strings.Split(os.Getenv(env.PATH), ":")
+	return strings.Split(os.Getenv(env.PATH), string(filepath.ListSeparator))
 }

pkg/fsutil/search_unix_test.go

@@ -11,9 +11,6 @@ import (
 	"src.elv.sh/pkg/testutil"
 )
 
-// TODO: When EachExternal is modified to work on Windows either fold this
-// test into external_cmd_test.go or create an external_cmd_windows_test.go
-// that performs an equivalent test on Windows.
 func TestEachExternal(t *testing.T) {
 	binPath := testutil.InTempDir(t)
 

pkg/fsutil/search_windows_test.go

@@ -0,0 +1,34 @@
+package fsutil
+
+import (
+	"reflect"
+	"sort"
+	"testing"
+
+	"src.elv.sh/pkg/testutil"
+)
+
+func TestEachExternal(t *testing.T) {
+	binPath := testutil.InTempDir(t)
+
+	testutil.Setenv(t, "PATH", "Z:\\foo;"+binPath+";Z:\\bar")
+
+	testutil.ApplyDir(testutil.Dir{
+		"dir":      testutil.Dir{},
+		"file.txt": "",
+		"prog.bat": testutil.File{Perm: 0o666, Content: ""},
+		"prog.cmd": testutil.File{Perm: 0o755, Content: ""},
+		"prog.txt": testutil.File{Perm: 0o755, Content: ""},
+		"PROG.EXE": "", // validate that explicit file perms don't matter
+	})
+
+	wantCmds := []string{"prog.bat", "prog.cmd", "PROG.EXE"}
+	gotCmds := []string{}
+	EachExternal(func(cmd string) { gotCmds = append(gotCmds, cmd) })
+
+	sort.Strings(wantCmds)
+	sort.Strings(gotCmds)
+	if !reflect.DeepEqual(wantCmds, gotCmds) {
+		t.Errorf("EachExternal want %q got %q", wantCmds, gotCmds)
+	}
+}

tools/run-race.sh

@@ -1,15 +1,14 @@
 #!/bin/sh
 # Prints "-race" if running on a platform that supports the race detector.
-# This should be kept in sync of the official list here:
-# https://golang.org/doc/articles/race_detector#Supported_Systems
+# This should be kept in sync with the official list here:
+# https://golang.org/doc/articles/race_detector#Requirements
 if test `go env CGO_ENABLED` = 1; then
   if echo `go env GOOS GOARCH` |
      egrep -qx '((linux|darwin|freebsd|netbsd) amd64|(linux|darwin) arm64|linux ppc64le)'; then
     printf %s -race
   elif echo `go env GOOS GOARCH` | egrep -qx 'windows amd64'; then
-    # Race detector on windows amd64 requires gcc:
-    # https://github.com/golang/go/issues/27089
-    if which gcc > /dev/null; then
+    # Race detector on Windows AMD64 requires gcc: https://github.com/golang/go/issues/27089
+    if which gcc > /dev/null 2>&1; then
       printf %s -race
     fi
   fi