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

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Aug 13, 2012, 17:02 UTC
Message-ID: <20120813170221.GB6418@book.hvoigt.net>
In-Reply-To: <7vd32whgvl.fsf@alter.siamese.dyndns.org>

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

Show 45 quoted lines

> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> >  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