{"thread":{"id":"44987","subject":"[PATCH] help: correct behavior for is_executable on Windows","startedAt":"2017-01-27T13:50:26Z","lastAt":"2017-01-30T17:03:19Z","messageCount":5,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"310406","messageId":"c1c6ccae4e60608259809914e8ff3d3d5e1ead5a.1485524999.git.johannes.schindelin@gmx.de","threadId":"44987","inReplyTo":null,"subject":"[PATCH] help: correct behavior for is_executable on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-27T13:50:09Z","receivedAt":"2017-01-27T13:50:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Heiko Voigt \n\nThe previous implementation said that the filesystem information on\nWindows is not reliable to determine whether a file is executable. To\ngather this information it was peeking into the first two bytes of a\nfile to see whether it looks executable.\n\nApart from the fact that on Windows executables are defined as such by\ntheir extension (and Git has special code to support \"executing\" scripts\nwhen they have a she-bang line) it leads to serious performance\nproblems: not only do we have to open many files now, it gets even\nslower when a virus scanner is running.\n\nSee the following measurements (in seconds) as an example, where we\nexecute a simple program that simply lists the directory contents and\ncalls open() on every listed file:\n\nWith virus scanner running (coldcache):\n\n$ ./a.exe /libexec/git-core/\nbefore open (git-add.exe): 0.000000\nafter open (git-add.exe): 0.412873\nbefore open (git-annotate.exe): 0.000175\nafter open (git-annotate.exe): 0.397925\nbefore open (git-apply.exe): 0.000243\nafter open (git-apply.exe): 0.399996\nbefore open (git-archive.exe): 0.000147\nafter open (git-archive.exe): 0.397783\nbefore open (git-bisect--helper.exe): 0.000160\nafter open (git-bisect--helper.exe): 0.397700\nbefore open (git-blame.exe): 0.000160\nafter open (git-blame.exe): 0.399136\n...\n\nWith virus scanner running (hotcache):\n\n$ ./a.exe /libexec/git-core/\nbefore open (git-add.exe): 0.000000\nafter open (git-add.exe): 0.000325\nbefore open (git-annotate.exe): 0.000229\nafter open (git-annotate.exe): 0.000177\nbefore open (git-apply.exe): 0.000167\nafter open (git-apply.exe): 0.000150\nbefore open (git-archive.exe): 0.000154\nafter open (git-archive.exe): 0.000156\nbefore open (git-bisect--helper.exe): 0.000132\nafter open (git-bisect--helper.exe): 0.000180\nbefore open (git-blame.exe): 0.000718\nafter open (git-blame.exe): 0.000724\n...\n\nWith this patch I get:\n\n$ time git help git\nLaunching default browser to display HTML ...\n\nreal 0m8.723s\nuser 0m0.000s\nsys 0m0.000s\n\nand without\n\n$ time git help git\nLaunching default browser to display HTML ...\n\nreal 1m37.734s\nuser 0m0.000s\nsys 0m0.031s\n\nboth tests with cold cache and giving the machine some time to settle\ndown after restart.\n\nSigned-off-by: Heiko Voigt \nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/help-is-exe-v1\nFetch-It-Via: git fetch https://github.com/dscho/git help-is-exe-v1\n\n help.c | 21 ++++++++++++++++++---\n 1 file changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 53e2a67e00..bc6cd19cf3 100644\n--- a/help.c\n+++ b/help.c\n@@ -105,7 +105,22 @@ static int is_executable(const char *name)\n \t\treturn 0;\n \n #if defined(GIT_WINDOWS_NATIVE)\n-{\t/* cannot trust the executable bit, peek into the file instead */\n+\t/*\n+\t * On Windows there is no executable bit. The file extension\n+\t * indicates whether it can be run as an executable, and Git\n+\t * has special-handling to detect scripts and launch them\n+\t * through the indicated script interpreter. We test for the\n+\t * file extension first because virus scanners may make\n+\t * it quite expensive to open many files.\n+\t */\n+\tif (ends_with(name, \".exe\"))\n+\t\treturn S_IXUSR;\n+\n+{\n+\t/*\n+\t * Now that we know it does not have an executable extension,\n+\t * peek into the file instead.\n+\t */\n \tchar buf[3] = { 0 };\n \tint n;\n \tint fd = open(name, O_RDONLY);\n@@ -113,8 +128,8 @@ static int is_executable(const char *name)\n \tif (fd >= 0) {\n \t\tn = read(fd, buf, 2);\n \t\tif (n == 2)\n-\t\t\t/* DOS executables start with \"MZ\" */\n-\t\t\tif (!strcmp(buf, \"#!\") || !strcmp(buf, \"MZ\"))\n+\t\t\t/* look for a she-bang */\n+\t\t\tif (!strcmp(buf, \"#!\"))\n \t\t\t\tst.st_mode |= S_IXUSR;\n \t\tclose(fd);\n \t}\n\nbase-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe\n-- \n2.11.1.windows.prerelease.2.9.g3014b57\n"},{"id":"310430","messageId":"xmqqd1f8z6lt.fsf@gitster.mtv.corp.google.com","threadId":"44987","inReplyTo":"c1c6ccae4e60608259809914e8ff3d3d5e1ead5a.1485524999.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] help: correct behavior for is_executable on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-27T18:43:26Z","receivedAt":"2017-01-27T18:45:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> From: Heiko Voigt \n>\n> The previous implementation said that the filesystem information on\n> Windows is not reliable to determine whether a file is executable. To\n> gather this information it was peeking into the first two bytes of a\n> file to see whether it looks executable.\n>\n> Apart from the fact that on Windows executables are defined as such by\n> their extension (and Git has special code to support \"executing\" scripts\n> when they have a she-bang line) it leads to serious performance\n> problems: not only do we have to open many files now, it gets even\n> slower when a virus scanner is running.\n\nHeiko, around here (before going into the details of how severe the\nproblem is and how wonderful the result applying of this change is)\nis the best place to summarize the solution. E.g.\n\n\tBecause the definition of \"executable\" on Windows is based\n\ton the file extension, update the function to declare that a\n\tfile with \".exe\" extension without opening and reading the\n\tearly bytes from it. This avoids serious performance issues.\n\nI paraphrased the rest only so that the description of the solution\n(i.e. \"instead of opening and peeking, we trust .exe suffix\") fits\nwell in the surrounding text; the important part is to say what the\nchange does clearly.\n\nI agree with the reasoning and the execution of the patch, except\nthat \n\n - \"correct behaviour\" in the title makes it appear that this is a\n correctness thing, but this is primarily a performance fix.\n\n - It is a bit strange that \"MZ\" is dropped in the same patch\n without any mention.\n\nThe latter may be \"correctness\" fix, in that earlier we treated any\nfile that happens to begin with MZ as an executable, regardless of\nthe filename, which may have misidentified a non-executable file as\nan executable. If that is what is going on, it deserves to be\ndescribed in the log message.\n\n> diff --git a/help.c b/help.c\n> index 53e2a67e00..bc6cd19cf3 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -105,7 +105,22 @@ static int is_executable(const char *name)\n> \t\treturn 0;\n> \n> #if defined(GIT_WINDOWS_NATIVE)\n> -{\t/* cannot trust the executable bit, peek into the file instead */\n> +\t/*\n> +\t * On Windows there is no executable bit. The file extension\n> +\t * indicates whether it can be run as an executable, and Git\n> +\t * has special-handling to detect scripts and launch them\n> +\t * through the indicated script interpreter. We test for the\n> +\t * file extension first because virus scanners may make\n> +\t * it quite expensive to open many files.\n> +\t */\n> +\tif (ends_with(name, \".exe\"))\n> +\t\treturn S_IXUSR;\n> +\n> +{\n> +\t/*\n> +\t * Now that we know it does not have an executable extension,\n> +\t * peek into the file instead.\n> +\t */\n> \tchar buf[3] = { 0 };\n> \tint n;\n> \tint fd = open(name, O_RDONLY);\n> @@ -113,8 +128,8 @@ static int is_executable(const char *name)\n> \tif (fd >= 0) {\n> \t\tn = read(fd, buf, 2);\n> \t\tif (n == 2)\n> -\t\t\t/* DOS executables start with \"MZ\" */\n> -\t\t\tif (!strcmp(buf, \"#!\") || !strcmp(buf, \"MZ\"))\n> +\t\t\t/* look for a she-bang */\n> +\t\t\tif (!strcmp(buf, \"#!\"))\n> \t\t\t\tst.st_mode |= S_IXUSR;\n> \t\tclose(fd);\n> \t}\n>\n> base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe\n"},{"id":"310522","messageId":"4b93fe44ff9020ed80e4fd93a24a6ffa647e7678.1485780050.git.johannes.schindelin@gmx.de","threadId":"44987","inReplyTo":"c1c6ccae4e60608259809914e8ff3d3d5e1ead5a.1485524999.git.johannes.schindelin@gmx.de","subject":"[PATCH v2] help: improve is_executable() on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-30T12:40:58Z","receivedAt":"2017-01-30T12:41:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Heiko Voigt \n\nOn Windows, executables need to have the file extension `.exe`, or they\nare not executables. Hence, to support scripts, Git for Windows also\nlooks for a she-bang line by opening the file in question, and executing\nit via the specified script interpreter.\n\nTo figure out whether files in the `PATH` are executable, `git help` has\ncode that imitates this behavior. With one exception: it *always* opens\nthe files and looks for a she-bang line *or* an `MZ` tell-tale\n(nevermind that files with the magic `MZ` but without file extension\n`.exe` would still not be executable).\n\nOpening this many files leads to performance problems that are even more\nserious when a virus scanner is running. Therefore, let's change the\ncode to look for the file extension `.exe` early, and avoid opening the\nfile altogether if we already know that it is executable.\n\nSee the following measurements (in seconds) as an example, where we\nexecute a simple program that simply lists the directory contents and\ncalls open() on every listed file:\n\nWith virus scanner running (coldcache):\n\n$ ./a.exe /libexec/git-core/\nbefore open (git-add.exe): 0.000000\nafter open (git-add.exe): 0.412873\nbefore open (git-annotate.exe): 0.000175\nafter open (git-annotate.exe): 0.397925\nbefore open (git-apply.exe): 0.000243\nafter open (git-apply.exe): 0.399996\nbefore open (git-archive.exe): 0.000147\nafter open (git-archive.exe): 0.397783\nbefore open (git-bisect--helper.exe): 0.000160\nafter open (git-bisect--helper.exe): 0.397700\nbefore open (git-blame.exe): 0.000160\nafter open (git-blame.exe): 0.399136\n...\n\nWith virus scanner running (hotcache):\n\n$ ./a.exe /libexec/git-core/\nbefore open (git-add.exe): 0.000000\nafter open (git-add.exe): 0.000325\nbefore open (git-annotate.exe): 0.000229\nafter open (git-annotate.exe): 0.000177\nbefore open (git-apply.exe): 0.000167\nafter open (git-apply.exe): 0.000150\nbefore open (git-archive.exe): 0.000154\nafter open (git-archive.exe): 0.000156\nbefore open (git-bisect--helper.exe): 0.000132\nafter open (git-bisect--helper.exe): 0.000180\nbefore open (git-blame.exe): 0.000718\nafter open (git-blame.exe): 0.000724\n...\n\nWith this patch I get:\n\n$ time git help git\nLaunching default browser to display HTML ...\n\nreal 0m8.723s\nuser 0m0.000s\nsys 0m0.000s\n\nand without\n\n$ time git help git\nLaunching default browser to display HTML ...\n\nreal 1m37.734s\nuser 0m0.000s\nsys 0m0.031s\n\nboth tests with cold cache and giving the machine some time to settle\ndown after restart.\n\n[jes: adjusted the commit message]\n\nSigned-off-by: Heiko Voigt \nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/help-is-exe-v2\nFetch-It-Via: git fetch https://github.com/dscho/git help-is-exe-v2\n\n help.c | 21 ++++++++++++++++++---\n 1 file changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 53e2a67e00..bc6cd19cf3 100644\n--- a/help.c\n+++ b/help.c\n@@ -105,7 +105,22 @@ static int is_executable(const char *name)\n \t\treturn 0;\n \n #if defined(GIT_WINDOWS_NATIVE)\n-{\t/* cannot trust the executable bit, peek into the file instead */\n+\t/*\n+\t * On Windows there is no executable bit. The file extension\n+\t * indicates whether it can be run as an executable, and Git\n+\t * has special-handling to detect scripts and launch them\n+\t * through the indicated script interpreter. We test for the\n+\t * file extension first because virus scanners may make\n+\t * it quite expensive to open many files.\n+\t */\n+\tif (ends_with(name, \".exe\"))\n+\t\treturn S_IXUSR;\n+\n+{\n+\t/*\n+\t * Now that we know it does not have an executable extension,\n+\t * peek into the file instead.\n+\t */\n \tchar buf[3] = { 0 };\n \tint n;\n \tint fd = open(name, O_RDONLY);\n@@ -113,8 +128,8 @@ static int is_executable(const char *name)\n \tif (fd >= 0) {\n \t\tn = read(fd, buf, 2);\n \t\tif (n == 2)\n-\t\t\t/* DOS executables start with \"MZ\" */\n-\t\t\tif (!strcmp(buf, \"#!\") || !strcmp(buf, \"MZ\"))\n+\t\t\t/* look for a she-bang */\n+\t\t\tif (!strcmp(buf, \"#!\"))\n \t\t\t\tst.st_mode |= S_IXUSR;\n \t\tclose(fd);\n \t}\n\nbase-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe\n-- \n2.11.1.windows.prerelease.2.9.g3014b57\n"},{"id":"310524","messageId":"alpine.DEB.2.20.1701301341020.3469@virtualbox","threadId":"44987","inReplyTo":"xmqqd1f8z6lt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] help: correct behavior for is_executable on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-30T12:44:08Z","receivedAt":"2017-01-30T13:07:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 27 Jan 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > From: Heiko Voigt \n> >\n> > The previous implementation said that the filesystem information on\n> > Windows is not reliable to determine whether a file is executable. To\n> > gather this information it was peeking into the first two bytes of a\n> > file to see whether it looks executable.\n> >\n> > Apart from the fact that on Windows executables are defined as such by\n> > their extension (and Git has special code to support \"executing\"\n> > scripts when they have a she-bang line) it leads to serious\n> > performance problems: not only do we have to open many files now, it\n> > gets even slower when a virus scanner is running.\n> \n> Heiko, around here (before going into the details of how severe the\n> problem is and how wonderful the result applying of this change is) is\n> the best place to summarize the solution. E.g.\n> \n> \tBecause the definition of \"executable\" on Windows is based\n> \ton the file extension, update the function to declare that a\n> \tfile with \".exe\" extension without opening and reading the\n> \tearly bytes from it. This avoids serious performance issues.\n> \n> I paraphrased the rest only so that the description of the solution\n> (i.e. \"instead of opening and peeking, we trust .exe suffix\") fits well\n> in the surrounding text; the important part is to say what the change\n> does clearly.\n\nI adjusted the commit message. It was tweaked a little differently from\nwhat you suggested, as I preferred to condense the information a bit more.\n\n> I agree with the reasoning and the execution of the patch, except\n> that \n> \n> - \"correct behaviour\" in the title makes it appear that this is a\n> correctness thing, but this is primarily a performance fix.\n\nPrimarily. But not only. The magic `MZ` without the file extension `.exe`\nis pretty useless, as the file could not be executed, still.\n\nTo avoid further turnaround, though, I also edited the contentious\n\"correct\" to read \"improve\" now.\n\n> - It is a bit strange that \"MZ\" is dropped in the same patch\n> without any mention.\n\nI fixed that in the commit message.\n\nCiao,\nJohannes\n"},{"id":"310536","messageId":"xmqqmve8wkds.fsf@gitster.mtv.corp.google.com","threadId":"44987","inReplyTo":"4b93fe44ff9020ed80e4fd93a24a6ffa647e7678.1485780050.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2] help: improve is_executable() on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-30T17:03:11Z","receivedAt":"2017-01-30T17:03:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> From: Heiko Voigt \n>\n> On Windows, executables need to have the file extension `.exe`, or they\n> are not executables. Hence, to support scripts, Git for Windows also\n> looks for a she-bang line by opening the file in question, and executing\n> it via the specified script interpreter.\n>\n> To figure out whether files in the `PATH` are executable, `git help` has\n> code that imitates this behavior. With one exception: it *always* opens\n> the files and looks for a she-bang line *or* an `MZ` tell-tale\n> (nevermind that files with the magic `MZ` but without file extension\n> `.exe` would still not be executable).\n>\n> Opening this many files leads to performance problems that are even more\n> serious when a virus scanner is running. Therefore, let's change the\n> code to look for the file extension `.exe` early, and avoid opening the\n> file altogether if we already know that it is executable.\n\nMuch more readable than the initial round. Will queue; thanks.\n"}]}