Hi Junio,

On Mon, Aug 13, 2012 at 10:48:14AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvo...@hvoigt.net> writes:
> > What do you think?
> 
> Does having the "stat()" help on Windows in any way?  Does it ever
> return an executable bit by itself?

No, AFAIK it does not return anything about executability. But I think
the stat is still necessary to verify that the file exists and is a
regular file.

Here is a draft of a patch I would follow up with:

Subject: [PATCH RFC] help: extract platform dependent part of is_executable in
 extra module

To remove various ifdefs required for the special handling of
executables on windows we create a new module 'executable' in compat
which allows a user to correctly fill the S_IXUSR flag of struct stat on
Windows.

Since this is valid for all variants of windows (mingw, msvc, cygwin) we
create a new module to avoid code duplication. compat/msvc.h includes
mingw.h so we do not add an extra include there. By default we define
correct_executeable_stat() to a no op on all other platforms. This is
guarded by a NO_EXECUTABLE_STAT which when defined by a compat header
requires and implementation of this function.

NOTE: I did not test this. I first would like to discuss whether the
overall structure this approach is taking is ok.

Signed-off-by: Heiko Voigt <hvo...@hvoigt.net>
---
 Makefile                  |  8 +++++---
 compat/cygwin.h           |  2 ++
 compat/mingw.h            |  2 ++
 compat/win32/executable.c | 33 +++++++++++++++++++++++++++++++++
 compat/win32/executable.h |  9 +++++++++
 git-compat-util.h         |  4 ++++
 help.c                    | 30 ++----------------------------
 7 files changed, 57 insertions(+), 31 deletions(-)
 create mode 100644 compat/win32/executable.c
 create mode 100644 compat/win32/executable.h

diff --git a/Makefile b/Makefile
index 6b0c961..cea3559 100644
--- a/Makefile
+++ b/Makefile
@@ -1073,7 +1073,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/executable.o
        UNRELIABLE_FSTAT = UnfortunatelyYes
        SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
 endif
@@ -1257,7 +1257,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/executable.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
@@ -1347,7 +1348,8 @@ ifneq (,$(findstring MINGW,$(uname_S)))
        COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
        COMPAT_OBJS += compat/mingw.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/executable.o
        EXTLIBS += -lws2_32
        PTHREAD_LIBS =
        X = .exe
diff --git a/compat/cygwin.h b/compat/cygwin.h
index a3229f5..e3bbd4d 100644
--- a/compat/cygwin.h
+++ b/compat/cygwin.h
@@ -1,6 +1,8 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 
+#include "win32/executable.h"
+
 typedef int (*stat_fn_t)(const char*, struct stat*);
 extern stat_fn_t cygwin_stat_fn;
 extern stat_fn_t cygwin_lstat_fn;
diff --git a/compat/mingw.h b/compat/mingw.h
index 61a6521..73c4f3d 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -1,6 +1,8 @@
 #include <winsock2.h>
 #include <ws2tcpip.h>
 
+#include "win32/executable.h"
+
 /*
  * things that are not available in header files
  */
diff --git a/compat/win32/executable.c b/compat/win32/executable.c
new file mode 100644
index 0000000..326ddb1
--- /dev/null
+++ b/compat/win32/executable.c
@@ -0,0 +1,33 @@
+#include "executeable.h"
+
+void correct_executable_stat(const char *filename, struct stat *st)
+{
+       char buf[3] = { 0 };
+       int n, fd;
+
+       /* 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"))
+               st->st_mode |= S_IXUSR;
+
+       if (st.st_mode & S_IXUSR)
+               return;
+
+       /* now that we know it does not have an executable extension,
+          peek into the file instead */
+       fd = open(name, O_RDONLY);
+       if (fd < 0)
+               return;
+
+       n = read(fd, buf, 2);
+       if (n == 2) {
+               /* look for a she-bang */
+               if (!strcmp(buf, "#!"))
+                       st.st_mode |= S_IXUSR;
+       }
+
+       close(fd);
+}
diff --git a/compat/win32/executable.h b/compat/win32/executable.h
new file mode 100644
index 0000000..e4f0c5c
--- /dev/null
+++ b/compat/win32/executable.h
@@ -0,0 +1,9 @@
+#ifndef COMPAT_WIN32_EXECUTEABLE
+#define COMPAT_WIN32_EXECUTEABLE
+
+#include "../../git-compat-util.h"
+
+#define NO_EXECUTABLE_STAT
+extern void correct_executable_stat(const char *filename, struct stat *st);
+
+#endif /* COMPAT_WIN32_EXECUTEABLE */
diff --git a/git-compat-util.h b/git-compat-util.h
index 35b095e..1b723af 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -607,4 +607,8 @@ 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);
 
+#ifndef NO_EXECUTABLE_STAT
+#define correct_executable_stat(name, stat) ;
+#endif
+
 #endif
diff --git a/help.c b/help.c
index ebc2c42..d9fae3c 100644
--- a/help.c
+++ b/help.c
@@ -106,34 +106,8 @@ static int is_executable(const char *name)
            !S_ISREG(st.st_mode))
                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
-{      /* 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);
-       st.st_mode &= ~S_IXUSR;
-       if (fd >= 0) {
-               n = read(fd, buf, 2);
-               if (n == 2)
-                       /* look for a she-bang */
-                       if (!strcmp(buf, "#!"))
-                               st.st_mode |= S_IXUSR;
-               close(fd);
-       }
-}
-#endif
+       correct_executable_stat(name, &st);
+
        return st.st_mode & S_IXUSR;
 }
 
-- 
1.7.12.rc2.11.g5d52328

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to