patchhelp: correct behavior for is_executable on Windows

The previous implementation said that the filesystem information on Windows is not reliable to determine whether a file is executable. To find gather this information it was peeking into the first two bytes of a file to see whether it looks executable. Apart from the fact that on Windows executables are usually defined as such by their extension it lead to slow opening of help file in some situations.

When you have virus scanner running calling open on an executable file is a potentially expensive operation. See the following measurements (in seconds) for example.

With virus scanner running (coldcache):

$ ./a.exe /libexec/git-core/ before open (git-add.exe): 0.000000 after open (git-add.exe): 0.412873 before open (git-annotate.exe): 0.000175 after open (git-annotate.exe): 0.397925 before open (git-apply.exe): 0.000243 after open (git-apply.exe): 0.399996 before open (git-archive.exe): 0.000147 after open (git-archive.exe): 0.397783 before open (git-bisect--helper.exe): 0.000160 after open (git-bisect--helper.exe): 0.397700 before open (git-blame.exe): 0.000160 after open (git-blame.exe): 0.399136 ...

With virus scanner running (hotcache):

$ ./a.exe /libexec/git-core/ before open (git-add.exe): 0.000000 after open (git-add.exe): 0.000325 before open (git-annotate.exe): 0.000229 after open (git-annotate.exe): 0.000177 before open (git-apply.exe): 0.000167 after open (git-apply.exe): 0.000150 before open (git-archive.exe): 0.000154 after open (git-archive.exe): 0.000156 before open (git-bisect--helper.exe): 0.000132 after open (git-bisect--helper.exe): 0.000180 before open (git-blame.exe): 0.000718 after open (git-blame.exe): 0.000724 ...

This test did just list the given directory and open() each file in it.

With this patch I get:

$ time git help git Launching default browser to display HTML ...

real 0m8.723s user 0m0.000s sys 0m0.000s

and without

$ time git help git Launching default browser to display HTML ...

real 1m37.734s user 0m0.000s sys 0m0.031s

both tests with cold cache and giving the machine some time to settle down after restart.

Signed-off-by: Heiko Voigt <heiko.voigt@mahr.de>
---
 help.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Heiko Voigt <hvoigt@hvoigt.net> writes:

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?

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

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

Heiko Voigt <hvoigt@hvoigt.net> writes:

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.

Hi Junio,

On Mon, Aug 13, 2012 at 10:48:14AM -0700, Junio C Hamano wrote:

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.

Here is a draft of a patch I would follow up with:

Subject: [PATCH RFC] help: extract platform dependent part of is_executable in
 extra module

To remove various ifdefs required for the special handling of executables on windows we create a new module 'executable' in compat which allows a user to correctly fill the S_IXUSR flag of struct stat on Windows.

Since this is valid for all variants of windows (mingw, msvc, cygwin) we create a new module to avoid code duplication. compat/msvc.h includes mingw.h so we do not add an extra include there. By default we define correct_executeable_stat() to a no op on all other platforms. This is guarded by a NO_EXECUTABLE_STAT which when defined by a compat header requires and implementation of this function.

NOTE: I did not test this. I first would like to discuss whether the
overall structure this approach is taking is ok.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 Makefile                  |  8 +++++---
 compat/cygwin.h           |  2 ++
 compat/mingw.h            |  2 ++
 compat/win32/executable.c | 33 +++++++++++++++++++++++++++++++++
 compat/win32/executable.h |  9 +++++++++
 git-compat-util.h         |  4 ++++
 help.c                    | 30 ++----------------------------
 7 files changed, 57 insertions(+), 31 deletions(-)
 create mode 100644 compat/win32/executable.c
 create mode 100644 compat/win32/executable.h

Heiko Voigt <hvoigt@hvoigt.net> writes:

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?

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

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?

Junio C Hamano <gitster@pobox.com> writes:

Sorry; sent without sufficient proofreading.  

Obviously we need such an implementation (per platform) somewhere in the system.

What I meant to ask was "Why should we need a function whose implementation has to be platform-specific in this help.c file". The last part of the sentence was missing.

Hi Junio,

On Wed, Aug 15, 2012 at 10:53:55AM -0700, Junio C Hamano wrote:

No I am not opening the file anyway. Only when it does not have a ".exe" postfix. That at least was the intention of my previous patch in this thread.

My first patch in this thread (which lead to this extraction) is about avoiding the open because it is possibly very costly on our git binaries in case a virus scanner is running.

The optimized (and correct) check does only look at the given filename if it contains a ".exe" postfix. A directory (although unlikely) could also have such a suffix. Should we then consider that directory to be a git command? AFAICS there is no such check in the earlier codepath.

A stat is much cheaper since it does not open the file. For scripts the open is much cheaper than for our binaries because they are much smaller. For clarification: All the git builtin binaries basically contain the same code. Even though they are hardlinked, the system (or the scanner) does still consider them like individual files and executes a scan for each of them. That at least seems to be the case on a number of systems.

Agreed, I was not sure about the name anyway. is_executable.o sounds better.

I simply think filling in the correct information into the data structure we use on all platforms is the most transparent approach. We could also call this function

	correct_executable_stat_if_needed_by_platform()

but I considered that name to be too long. As explained above: To be fully correct the stat call is still needed to make sure we are not looking at a directory. list_commands_in_dir() from help.c is iterating over all entries in a given directory so if we skip the stat we are not sure whether we look at something that is not a regular file. Even though its unlikely that there will be a directory named something.exe in the git executable directory I would still like to keep the check in there.

When first looking at this I also thought about extending the windows stat replacement to do this whole logic. Then we could completely remove this platform specific thing. But I decided against that because when the file does not have a ".exe" postfix we would have an open call in all stats and to my knowledge that is very expensive on Windows.

I do not know why you are against filling that information into "struct stat". I could rephrase the above into something like this

	if (platform_is_executeable(name))
		return 1;

but its not that easy to replace to a no op by a macro anymore.

Cheers Heiko

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.

Junio C Hamano <gitster@pobox.com> writes:

An outline of such a change might look like this.

As usual, I am not great at names, so you may want to rename the shared one. Also I do not know what headers Cygwin and Windows specific bits would need to include, so you would have to include more than just "git-compat-util.h" in the compat/*.c files this patch adds.

It would make sense to move the "does it end with .exe" check from the caller to the shared function, but because that does not match the concept of the name I came up with (peek-contents), I left it out of the function. If there is a good name to express what such a combined "ends with .exe, or inspected contents look like an executable", it would make sense to rename it as such and move ".exe" check to it.

Thanks.

 Makefile                     |  6 ++++--
 compat/cygwin.c              | 16 ++++++++++++++++
 compat/win32/is_executable.c | 14 ++++++++++++++
 compat/win32/peek_contents.c | 16 ++++++++++++++++
 git-compat-util.h            |  9 +++++++++
 help.c                       | 32 +-------------------------------
 is_executable.c              | 12 ++++++++++++
 7 files changed, 72 insertions(+), 33 deletions(-)

Hi Junio,

On Wed, Aug 15, 2012 at 07:02:31PM -0700, Junio C Hamano wrote:

Thanks for the outline and the clarification, I will use it to cook up something.

Cheers Heiko