Re: [PATCH] help: correct behavior for is_executable on Windows

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> My preference is to remove "static int is_executable()" function
> from help.c, have an...
> ... I wouldn't mind seeing the implementation of posix_is_executable()
> in help.c, which will be dead-code on Windows and Cygwin, if that
> makes linking and Makefile easier.

An outline of such a change might look like this.

As usual, I am not great at names, so you may want to rename the
shared one.  Also I do not know what headers Cygwin and Windows
specific bits would need to include, so you would have to include
more than just "git-compat-util.h" in the compat/*.c files this
patch adds.

It would make sense to move the "does it end with .exe" check from
the caller to the shared function, but because that does not match
the concept of the name I came up with (peek-contents), I left it
out of the function. If there is a good name to express what such a
combined "ends with .exe, or inspected contents look like an
executable", it would make sense to rename it as such and move
".exe" check to it.

Thanks.

 Makefile                     |  6 ++++--
 compat/cygwin.c              | 16 ++++++++++++++++
 compat/win32/is_executable.c | 14 ++++++++++++++
 compat/win32/peek_contents.c | 16 ++++++++++++++++
 git-compat-util.h            |  9 +++++++++
 help.c                       | 32 +-------------------------------
 is_executable.c              | 12 ++++++++++++
 7 files changed, 72 insertions(+), 33 deletions(-)

diff --git a/Makefile b/Makefile
index 6b0c961..2bcb9f4 100644
--- a/Makefile
+++ b/Makefile
@@ -734,6 +734,7 @@ LIB_OBJS += hash.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
 LIB_OBJS += ident.o
+LIB_OBJS += is_executable.o
 LIB_OBJS += kwset.o
 LIB_OBJS += levenshtein.o
 LIB_OBJS += list-objects.o
@@ -1073,7 +1074,7 @@ ifeq ($(uname_O),Cygwin)
 	# Try commenting this out if you suspect MMAP is more efficient
 	NO_MMAP = YesPlease
 	X = .exe
-	COMPAT_OBJS += compat/cygwin.o
+	COMPAT_OBJS += compat/cygwin.o compat/win32/peek_contents.o
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
 endif
@@ -1257,7 +1258,8 @@ ifeq ($(uname_S),Windows)
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
 	COMPAT_OBJS = compat/msvc.o compat/winansi.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
-		compat/win32/poll.o compat/win32/dirent.o
+		compat/win32/poll.o compat/win32/dirent.o \
+		compat/win32/is_executable.o compat/win32/peek_contents.o
 	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib
diff --git a/compat/cygwin.c b/compat/cygwin.c
index dfe9b30..5eb948a 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -140,3 +140,19 @@ static int cygwin_lstat_stub(const char *file_name, struct stat *buf)
 stat_fn_t cygwin_stat_fn = cygwin_stat_stub;
 stat_fn_t cygwin_lstat_fn = cygwin_lstat_stub;
 
+extern int win32_peek_contents_executable(const char *);
+
+int cygwin_is_path_executable(const char *name)
+{
+	struct stat st;
+
+	/* stat, not lstat */
+	if (stat(name, &st) || !S_ISREG(st.st_mode))
+		return 0;
+	if (st.st_mode & S_IXUSR)
+		return 1;
+
+	if (has_extension(name, ".exe"))
+		return 1;
+	return win32_peek_contents_executable(name);
+}
diff --git a/compat/win32/is_executable.c b/compat/win32/is_executable.c
new file mode 100644
index 0000000..f7907b3
--- /dev/null
+++ b/compat/win32/is_executable.c
@@ -0,0 +1,14 @@
+#include "git-compat-util.h"
+
+int win32_path_is_executable(const char *name)
+{
+	/*
+	 * Do whatever you would do on win32 to make sure
+	 * "name" exists here
+	 */
+	if (!file_exists(name))
+		return 0;
+	if (has_extension(name, ".exe"))
+		return 1;
+	return win32_peek_contents_executable(name);
+}
diff --git a/compat/win32/peek_contents.c b/compat/win32/peek_contents.c
new file mode 100644
index 0000000..5e425aa
--- /dev/null
+++ b/compat/win32/peek_contents.c
@@ -0,0 +1,16 @@
+#include "git-compat-util.h"
+
+int win32_peek_contents_executable(const char *name)
+{
+	char buf[2];
+	int n, fd;
+
+	fd = open(name, O_RDONLY);
+	if (fd < 0)
+		return 0;
+	n = read(fd, buf, 2);
+	close(fd);
+	return (n == 2 &&
+		/* DOS executables start with "MZ" */
+		(!memcmp(buf, "#!", 2) || !memcmp(buf, "MZ", 2)));
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 35b095e..e8f8cb1 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -607,4 +607,13 @@ int remove_or_warn(unsigned int mode, const char *path);
 /* Get the passwd entry for the UID of the current process. */
 struct passwd *xgetpwuid_self(void);
 
+/* Does the given path name an executable command? */
+#if defined(__CYGWIN__)
+#define is_path_executable(p) cygwin_is_path_executable(p)
+#elif defined(WIN32)
+#define is_path_executable(p) win32_is_path_executable(p)
+#endif
+
+extern int is_path_executable(const char *);
+
 #endif
diff --git a/help.c b/help.c
index 2a42ec6..672d9d3 100644
--- a/help.c
+++ b/help.c
@@ -98,36 +98,6 @@ static void pretty_print_string_list(struct cmdnames *cmds,
 	string_list_clear(&list, 0);
 }
 
-static int is_executable(const char *name)
-{
-	struct stat st;
-
-	if (stat(name, &st) || /* stat, not lstat */
-	    !S_ISREG(st.st_mode))
-		return 0;
-
-#if defined(WIN32) || defined(__CYGWIN__)
-#if defined(__CYGWIN__)
-if ((st.st_mode & S_IXUSR) == 0)
-#endif
-{	/* cannot trust the executable bit, peek into the file instead */
-	char buf[3] = { 0 };
-	int n;
-	int fd = open(name, O_RDONLY);
-	st.st_mode &= ~S_IXUSR;
-	if (fd >= 0) {
-		n = read(fd, buf, 2);
-		if (n == 2)
-			/* DOS executables start with "MZ" */
-			if (!strcmp(buf, "#!") || !strcmp(buf, "MZ"))
-				st.st_mode |= S_IXUSR;
-		close(fd);
-	}
-}
-#endif
-	return st.st_mode & S_IXUSR;
-}
-
 static void list_commands_in_dir(struct cmdnames *cmds,
 					 const char *path,
 					 const char *prefix)
@@ -155,7 +125,7 @@ static void list_commands_in_dir(struct cmdnames *cmds,
 
 		strbuf_setlen(&buf, len);
 		strbuf_addstr(&buf, de->d_name);
-		if (!is_executable(buf.buf))
+		if (!is_path_executable(buf.buf))
 			continue;
 
 		entlen = strlen(de->d_name) - prefix_len;
diff --git a/is_executable.c b/is_executable.c
new file mode 100644
index 0000000..9dacd2a
--- /dev/null
+++ b/is_executable.c
@@ -0,0 +1,12 @@
+#include "git-compat-util.h"
+
+int is_path_executable(const char *name)
+{
+	struct stat st;
+
+	/* stat, not lstat */
+	if (stat(name, &st) || !S_ISREG(st.st_mode))
+		return 0;
+
+	return st.st_mode & S_IXUSR;
+}