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

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

Heiko Voigt <hvoigt@hvoigt.net> writes:

> I do not know why you are against filling that information into "struct
> stat".

Because it is *WRONG*.  Isn't it a good enough reason?

If the issue you are trying to solve were """stat emulation on
Windows and Cygwin does not give the correct x-bit (and the user
sometimes has to work it around with "update-index --chmod"), and by
using other clues the emulation can be improved to give a better
result""", I agree that it would be a good solution to have the
"""Does it exist as a regular file and ends with ".exe"?  Otherwise
does it start with "MZ" or "#!"?""" heuristics in a helper function
correct_executable_stat(), and to have the implementation of stat
emulation on these two platforms call that shared helper function.

But look at the caller again.  The problem the caller wants this
function to solve is not "I want you to stat this file."  It has a
pathname, and only wants to know if it is an executable file.  It
does not care about who owns it, what time it was last touched,
etc., but calling the incomplete stat emulation on Windows will try
to come up with an answer, and the is_executable() function discards
most of it.  

In other words, you are solving a wrong problem with that approach.

"Run stat() and look at S_IXUSR" happens to be an implementation
detail that is valid only on POSIX systems for a function to answer
the question: "Is this an executable file?", and in that specific
implementation, the answer to that question appears in the S_IXUSR
bit of st.st_mode.  That does not mean the "struct stat" is the best
container for the answer to that question on other platforms.  So
why structure your abstraction around the inappropriate data
structure?  Between the function (is_executable) and its callers,
there is only one bit that needs to be passed.

My preference is to remove "static int is_executable()" function
from help.c, have an "extern int is_executable(const char *)" that
has platform-specific implementation in compat/ layer, and call it
from help.c::list_commands_in_dir() without any #ifdef.  In
git-compat-util.h, have something like:

	#ifdef __CYGWIN__
	#define is_executable(path) cygwin_is_executable(path)
	#else
        # ifdef WIN32
        # define is_executable(path) win32_is_executable(path)
	# endif
	#endif

        #ifndef is_exectutable
	#define is_executable(path) posix_is_executable(path)
	#endif

        extern int is_executable(const char *);

I wouldn't mind seeing the implementation of posix_is_executable()
in help.c, which will be dead-code on Windows and Cygwin, if that
makes linking and Makefile easier.