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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 13, 2012, 17:48 UTC
Message-ID: <7vmx1yel9d.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120813170221.GB6418@book.hvoigt.net>

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 23 quoted lines

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

Does having the "stat()" help on Windows in any way? Does it ever return an executable bit by itself?

If not, Windows compat/ implementation may want to skip issuing a useless stat() and write it as

	if (has_extension(".exe"))
        	return 1;
	if (contents_begins_with("MZ") || contents_begins_with("#!"))
        	return 1;
	return 0;

without ever talking about stat() which is POSIXism compat/ implementation for Windows does not have to worry about.

And that was the reason I suggested making the whole implementation of path_is_executable() overridable by compat/ layer.

But if having "stat()" helps on Windows, then your counterproposal is good enough for me.

Thanks.