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

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

Heiko Voigt <hvoigt@hvoigt.net> writes:

> On Mon, Aug 13, 2012 at 10:48:14AM -0700, Junio C Hamano wrote:
>> Heiko Voigt <hvoigt@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.

But if you are going to read it anyway, you can tell it from open()
and read() of the first 2 bytes failing, no?  That will still be an
implementation detail of platform specific "is_path_executable()".

So you are forcing Windows an extra and unnecessary stat() that only
is needed on Cygwin, no?

> @@ -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

Looks sensible, even though the filename does not tell what it does
about "executable".  is_executable.o might be a better name for them.

> 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);
> +

Yuck.

Why should we need even a single line of the implementation of a
function that tells if a given pathname contains an executable
command, which we know is platform specific?  

On Posix systems, the implementation will be "stat() and check
S_IXUSR".  On pure Windows, it will be "check .exe, or open it and
read the first two bytes". On Cygwin, it will also be "check .exe,
stat() and check S_IXUSR, or open it and read the first two bytes.

It is not like the caller of is_executable() needed to run stat for
other purposes on its own and we can optimize by either borrowing
the stat data the caller already collected for us, or returning the
stat data we collected for later use by the caller.  The use of stat
is entirely contained in the POSIX implementation of this function.

Why are you so dead-set to shoehorn the different semantics into
"struct stat" that we know is not an appropriate abstraction across
platforms?