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

Hi Junio,

On Fri, 27 Jan 2017, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

From: Heiko Voigt <redacted>

The previous implementation said that the filesystem information on
Windows is not reliable to determine whether a file is executable. To
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 defined as such by
their extension (and Git has special code to support "executing"
scripts when they have a she-bang line) it leads to serious
performance problems: not only do we have to open many files now, it
gets even slower when a virus scanner is running.

Heiko, around here (before going into the details of how severe the
problem is and how wonderful the result applying of this change is) is
the best place to summarize the solution.  E.g.

	Because the definition of "executable" on Windows is based
	on the file extension, update the function to declare that a
	file with ".exe" extension without opening and reading the
	early bytes from it.  This avoids serious performance issues.

I paraphrased the rest only so that the description of the solution
(i.e. "instead of opening and peeking, we trust .exe suffix") fits well
in the surrounding text; the important part is to say what the change
does clearly.

I adjusted the commit message. It was tweaked a little differently from
what you suggested, as I preferred to condense the information a bit more.

I agree with the reasoning and the execution of the patch, except
that 

 - "correct behaviour" in the title makes it appear that this is a
   correctness thing, but this is primarily a performance fix.

Primarily. But not only. The magic `MZ` without the file extension `.exe`
is pretty useless, as the file could not be executed, still.

To avoid further turnaround, though, I also edited the contentious
"correct" to read "improve" now.

 - It is a bit strange that "MZ" is dropped in the same patch
   without any mention.

I fixed that in the commit message.

Ciao,
Johannes