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

Heiko Voigt <hvoigt@hvoigt.net> writes:

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.