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

On Sat, Aug 11, 2012 at 09:30:06PM -0700, Junio C Hamano wrote:

Heiko Voigt [off-list ref] writes:

quoted

 help.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index 662349d..b41fa21 100644
--- a/help.c
+++ b/help.c

@@ -103,10 +103,19 @@ static int is_executable(const char *name)
 		return 0;
 
 #if defined(WIN32) || defined(__CYGWIN__)
+	/* On Windows we cannot use the executable bit. The executable
+	 * state is determined by extension only. We do this first
+	 * because with virus scanners opening an executeable for
+	 * reading is potentially expensive.
+	 */
+	if (has_extension(name, ".exe"))
+		return S_IXUSR;
+
 #if defined(__CYGWIN__)
 if ((st.st_mode & S_IXUSR) == 0)
 #endif
-{	/* cannot trust the executable bit, peek into the file instead */
+{	/* now that we know it does not have an executable extension,
+	   peek into the file instead */
 	char buf[3] = { 0 };
 	int n;
 	int fd = open(name, O_RDONLY);

@@ -114,8 +123,8 @@ if ((st.st_mode & S_IXUSR) == 0)
 	if (fd >= 0) {
 		n = read(fd, buf, 2);
 		if (n == 2)
-			/* DOS executables start with "MZ" */
-			if (!strcmp(buf, "#!") || !strcmp(buf, "MZ"))
+			/* look for a she-bang */
+			if (!strcmp(buf, "#!"))
 				st.st_mode |= S_IXUSR;
 		close(fd);
 	}

Would it make sense to move this to compat/win32/, compat/cygwin.c,
and compat/posix.c, each exporting is_executable(const char *path),
so that we do not have to suffer the #ifdef mess?

Yes that makes sense. But that means I need to test the code on multiple
platforms. To ease the merge in msysgit (the patch is already applied there)
I would suggest to post a follow up patch which would split up the function
into the platform specific parts.

Since the code for cygwin and windows in general is almost the same I would
extract one function for them where I leave in one ifdef for cygwin.

E.g. like this:


	static int is_executable(const char *name)
	{
	        struct stat st;

	        if (stat(name, &st) || /* stat, not lstat */
	            !S_ISREG(st.st_mode))
	                return 0;

		fill_platform_stat(name, &st);

	        return st.st_mode & S_IXUSR;
	}

which I could then define to a no op on posix. That way we avoid code
duplication in the platform specific functions.

What do you think?

Cheers Heiko