{"thread":{"id":"26060","subject":"[PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin","startedAt":"2010-12-14T18:29:26Z","lastAt":"2010-12-18T20:41:29Z","messageCount":8,"participants":["Ramsay Jones","Erik Faye-Lund","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":14},"messages":[{"id":"158072","messageId":"4D07B786.2060602@ramsay1.demon.co.uk","threadId":"26060","inReplyTo":null,"subject":"[PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-12-14T18:29:26Z","receivedAt":"2010-12-14T18:29:26Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nTest t7606-merge-custom.sh fails on cygwin when git-merge fails\nwith an \"Could not find merge strategy 'theirs'\" error, despite\nthe test correctly preparing an (executable) git-merge-theirs\nscript.\n\nThe cause of the failure is the mis-detection of the executable\nstatus of the script, by the is_executable() function, while the\nload_command_list() function is searching the path for additional\nmerge strategy programs.\n\nNote that the l/stat() \"functions\" on cygwin are somewhat\nschizophrenic (see commits adbc0b6, 7faee6b and 7974843), and\ntheir behaviour depends on the timing of various git setup and\nconfig function calls. In particular, until the \"git_dir\" has\nbeen set (have_git_dir() returns true), the real cygwin (POSIX\nemulating) l/stat() functions are called. Once \"git_dir\" has\nbeen set, the \"native Win32 API\" implementations of l/stat()\nmay, or may not, be called depending on the setting of the\ncore.filemode and core.ignorecygwinfstricks config variables.\n\nWe also note that, since commit c869753, core.filemode is forced\nto false, even on NTFS, by git-init and git-clone. A user (or a\ntest) can, of course, reset core.filemode to true explicitly if\nthe filesystem supports it (and he doesn't use any problematic\nwindows software). The test-suite currently runs all tests on\ncygwin with core.filemode set to false.\n\nGiven the above, we see that the built-in merge strategies are\ncorrectly detected as executable, since they are checked for\nbefore \"git_dir\" is set, whereas all custom merge strategies are\nnot, since they are checked for after \"git_dir\" is set.\n\nIn order to fix the mis-detection problem, we change the code in\nis_executable() to re-use the conditional WIN32 code section,\nwhich actually looks at the content of the file to determine if\nthe file is executable. On cygwin we also make the additional\ncode conditional on the executable bit of the file mode returned\nby the initial stat() call. (only the real cygwin function would\nset the executable bit in the file mode.)\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n help.c |    5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 7f4928e..eabadc9 100644\n--- a/help.c\n+++ b/help.c\n@@ -126,7 +126,10 @@ static int is_executable(const char *name)\n \t    !S_ISREG(st.st_mode))\n \t\treturn 0;\n \n-#ifdef WIN32\n+#if defined(WIN32) || defined(__CYGWIN__)\n+#if defined(__CYGWIN__)\n+if ((st.st_mode & S_IXUSR) == 0)\n+#endif\n {\t/* cannot trust the executable bit, peek into the file instead */\n \tchar buf[3] = { 0 };\n \tint n;\n-- \n1.7.3\n"},{"id":"158088","messageId":"AANLkTimt3w9GVCXa_n1_HXivyRmnRyUhhdSArrHOT6fs@mail.gmail.com","threadId":"26060","inReplyTo":"4D07B786.2060602@ramsay1.demon.co.uk","subject":"Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-12-14T20:07:28Z","receivedAt":"2010-12-14T20:07:28Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Dec 14, 2010 at 7:29 PM, Ramsay Jones\n<ramsay@ramsay1.demon.co.uk> wrote:\n>\n> Test t7606-merge-custom.sh fails on cygwin when git-merge fails\n> with an \"Could not find merge strategy 'theirs'\" error, despite\n> the test correctly preparing an (executable) git-merge-theirs\n> script.\n>\n> The cause of the failure is the mis-detection of the executable\n> status of the script, by the is_executable() function, while the\n> load_command_list() function is searching the path for additional\n> merge strategy programs.\n>\n> Note that the l/stat() \"functions\" on cygwin are somewhat\n> schizophrenic (see commits adbc0b6, 7faee6b and 7974843), and\n> their behaviour depends on the timing of various git setup and\n> config function calls. In particular, until the \"git_dir\" has\n> been set (have_git_dir() returns true), the real cygwin (POSIX\n> emulating) l/stat() functions are called. Once \"git_dir\" has\n> been set, the \"native Win32 API\" implementations of l/stat()\n> may, or may not, be called depending on the setting of the\n> core.filemode and core.ignorecygwinfstricks config variables.\n>\n> We also note that, since commit c869753, core.filemode is forced\n> to false, even on NTFS, by git-init and git-clone. A user (or a\n> test) can, of course, reset core.filemode to true explicitly if\n> the filesystem supports it (and he doesn't use any problematic\n> windows software). The test-suite currently runs all tests on\n> cygwin with core.filemode set to false.\n>\n> Given the above, we see that the built-in merge strategies are\n> correctly detected as executable, since they are checked for\n> before \"git_dir\" is set, whereas all custom merge strategies are\n> not, since they are checked for after \"git_dir\" is set.\n>\n> In order to fix the mis-detection problem, we change the code in\n> is_executable() to re-use the conditional WIN32 code section,\n> which actually looks at the content of the file to determine if\n> the file is executable. On cygwin we also make the additional\n> code conditional on the executable bit of the file mode returned\n> by the initial stat() call. (only the real cygwin function would\n> set the executable bit in the file mode.)\n>\n> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> ---\n>  help.c |    5 ++++-\n>  1 files changed, 4 insertions(+), 1 deletions(-)\n>\n> diff --git a/help.c b/help.c\n> index 7f4928e..eabadc9 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -126,7 +126,10 @@ static int is_executable(const char *name)\n>            !S_ISREG(st.st_mode))\n>                return 0;\n>\n> -#ifdef WIN32\n> +#if defined(WIN32) || defined(__CYGWIN__)\n> +#if defined(__CYGWIN__)\n> +if ((st.st_mode & S_IXUSR) == 0)\n> +#endif\n\nPerhaps the first check should simply be changed to check for _WIN32\ninstead of WIN32? IIRC _WIN32 is set on Cygwin, but I could be\nmistaken...\n"},{"id":"158089","messageId":"201012142138.37679.j6t@kdbg.org","threadId":"26060","inReplyTo":"4D07B786.2060602@ramsay1.demon.co.uk","subject":"Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-12-14T20:38:37Z","receivedAt":"2010-12-14T20:38:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:\n> @@ -126,7 +126,10 @@ static int is_executable(const char *name)\n>  \t    !S_ISREG(st.st_mode))\n>  \t\treturn 0;\n>\n> -#ifdef WIN32\n> +#if defined(WIN32) || defined(__CYGWIN__)\n> +#if defined(__CYGWIN__)\n> +if ((st.st_mode & S_IXUSR) == 0)\n> +#endif\n>  {\t/* cannot trust the executable bit, peek into the file instead */\n>  \tchar buf[3] = { 0 };\n>  \tint n;\n\nDo you gain a lot by this extra condition? Wouldn't\n\n-#ifdef WIN32\n+#if defined(WIN32) || defined(__CYGWIN__)\n\nbe sufficient?\n\n-- Hannes\n"},{"id":"158234","messageId":"4D0A7E87.3020203@ramsay1.demon.co.uk","threadId":"26060","inReplyTo":"AANLkTimt3w9GVCXa_n1_HXivyRmnRyUhhdSArrHOT6fs@mail.gmail.com","subject":"Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-12-16T21:03:03Z","receivedAt":"2010-12-16T21:03:03Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Erik Faye-Lund wrote:\n>> diff --git a/help.c b/help.c\n>> index 7f4928e..eabadc9 100644\n>> --- a/help.c\n>> +++ b/help.c\n>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)\n>>            !S_ISREG(st.st_mode))\n>>                return 0;\n>>\n>> -#ifdef WIN32\n>> +#if defined(WIN32) || defined(__CYGWIN__)\n>> +#if defined(__CYGWIN__)\n>> +if ((st.st_mode & S_IXUSR) == 0)\n>> +#endif\n> \n> Perhaps the first check should simply be changed to check for _WIN32\n> instead of WIN32? IIRC _WIN32 is set on Cygwin, but I could be\n> mistaken...\n\nNo, neither WIN32 or _WIN32 will be defined here (and they should not be).\nIt's actually quite tricky, particularly when #including <windows.h>, viz:\n\n    $ cat -n test.c\n     1\t#include <stdio.h>\n     2\t\n     3\t#ifdef IW\n     4\t# include <windows.h>\n     5\t#endif\n     6\t\n     7\tint main(int argc, char *argv[])\n     8\t{\n     9\t#ifdef WIN32\n    10\t\tprintf(\"WIN32 \");\n    11\t#endif\n    12\t#ifdef _WIN32\n    13\t\tprintf(\"_WIN32 \");\n    14\t#endif\n    15\t#ifdef __CYGWIN__\n    16\t\tprintf(\"__CYGWIN__ \");\n    17\t#endif\n    18\t#ifdef __MINGW32__\n    19\t\tprintf(\"__MINGW32__ \");\n    20\t#endif\n    21\t\tprintf(\"\\n\");\n    22\t\treturn 0;\n    23\t}\n\n    $ gcc -o test test.c\n    $ ./test\n    __CYGWIN__ \n\n    $ gcc -o test -DIW test.c\n    $ ./test\n    WIN32 _WIN32 __CYGWIN__ \n\n    $ gcc -o test -mno-cygwin test.c\n    $ ./test\n    WIN32 _WIN32 __MINGW32__ \n\nATB,\nRamsay Jones\n"},{"id":"158237","messageId":"4D0A80C5.4020003@ramsay1.demon.co.uk","threadId":"26060","inReplyTo":"201012142138.37679.j6t@kdbg.org","subject":"Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-12-16T21:12:37Z","receivedAt":"2010-12-16T21:12:37Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Johannes Sixt wrote:\n> On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:\n>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)\n>>  \t    !S_ISREG(st.st_mode))\n>>  \t\treturn 0;\n>>\n>> -#ifdef WIN32\n>> +#if defined(WIN32) || defined(__CYGWIN__)\n>> +#if defined(__CYGWIN__)\n>> +if ((st.st_mode & S_IXUSR) == 0)\n>> +#endif\n>>  {\t/* cannot trust the executable bit, peek into the file instead */\n>>  \tchar buf[3] = { 0 };\n>>  \tint n;\n> \n> Do you gain a lot by this extra condition? Wouldn't\n> \n> -#ifdef WIN32\n> +#if defined(WIN32) || defined(__CYGWIN__)\n> \n> be sufficient?\n\nYes, that would be sufficient. No, I probably don't gain a great deal\n(but I have *not* timed it), since the number of files that are tested\nby is_executable() is fairly low anyway since they are already filtered\nby a filename prefix (eg. git-merge-).\n\nHowever, if the executable bit is set, then executing the WIN32 code\nblock is wasted effort (we already know the answer), so why bother?\n\nATB,\nRamsay Jones\n"},{"id":"158240","messageId":"AANLkTi=9rSEK=7JyS6w50Hr=mrVttmjeqnBrbYsSR=6K@mail.gmail.com","threadId":"26060","inReplyTo":"4D0A7E87.3020203@ramsay1.demon.co.uk","subject":"Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-12-16T23:06:07Z","receivedAt":"2010-12-16T23:06:07Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Dec 16, 2010 at 10:03 PM, Ramsay Jones\n<ramsay@ramsay1.demon.co.uk> wrote:\n> Erik Faye-Lund wrote:\n>>> diff --git a/help.c b/help.c\n>>> index 7f4928e..eabadc9 100644\n>>> --- a/help.c\n>>> +++ b/help.c\n>>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)\n>>>            !S_ISREG(st.st_mode))\n>>>                return 0;\n>>>\n>>> -#ifdef WIN32\n>>> +#if defined(WIN32) || defined(__CYGWIN__)\n>>> +#if defined(__CYGWIN__)\n>>> +if ((st.st_mode & S_IXUSR) == 0)\n>>> +#endif\n>>\n>> Perhaps the first check should simply be changed to check for _WIN32\n>> instead of WIN32? IIRC _WIN32 is set on Cygwin, but I could be\n>> mistaken...\n>\n> No, neither WIN32 or _WIN32 will be defined here (and they should not be).\n> It's actually quite tricky, particularly when #including <windows.h>, viz:\n>\n>    $ cat -n test.c\n>     1  #include <stdio.h>\n>     2\n>     3  #ifdef IW\n>     4  # include <windows.h>\n>     5  #endif\n>     6\n>     7  int main(int argc, char *argv[])\n>     8  {\n>     9  #ifdef WIN32\n>    10          printf(\"WIN32 \");\n>    11  #endif\n>    12  #ifdef _WIN32\n>    13          printf(\"_WIN32 \");\n>    14  #endif\n>    15  #ifdef __CYGWIN__\n>    16          printf(\"__CYGWIN__ \");\n>    17  #endif\n>    18  #ifdef __MINGW32__\n>    19          printf(\"__MINGW32__ \");\n>    20  #endif\n>    21          printf(\"\\n\");\n>    22          return 0;\n>    23  }\n>\n>    $ gcc -o test test.c\n>    $ ./test\n>    __CYGWIN__\n>\n>    $ gcc -o test -DIW test.c\n>    $ ./test\n>    WIN32 _WIN32 __CYGWIN__\n>\n>    $ gcc -o test -mno-cygwin test.c\n>    $ ./test\n>    WIN32 _WIN32 __MINGW32__\n>\n\nHmm, I thought _WIN32 was always defined when targeting Windows, and\nthat WIN32 was defined when windows.h was included (or usually in the\npreprocessor flags when compiling GUI programs) - that's what MSVC\ndoes anyway. MinGW seems to always define both _WIN32 and WIN32, but\nas you've shown Cygwin doesn't define either until windows.h is\nincluded.\n\nSo sorry for leading you onto the wrong path, a check for __CYGWIN__\nseems to be necessary indeed.\n"},{"id":"158296","messageId":"201012172246.45038.j6t@kdbg.org","threadId":"26060","inReplyTo":"4D0A80C5.4020003@ramsay1.demon.co.uk","subject":"Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-12-17T21:46:44Z","receivedAt":"2010-12-17T21:46:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 16. Dezember 2010, Ramsay Jones wrote:\n> Johannes Sixt wrote:\n> > On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:\n> >> @@ -126,7 +126,10 @@ static int is_executable(const char *name)\n> >>  \t    !S_ISREG(st.st_mode))\n> >>  \t\treturn 0;\n> >>\n> >> -#ifdef WIN32\n> >> +#if defined(WIN32) || defined(__CYGWIN__)\n> >> +#if defined(__CYGWIN__)\n> >> +if ((st.st_mode & S_IXUSR) == 0)\n> >> +#endif\n> >>  {\t/* cannot trust the executable bit, peek into the file instead */\n> >>  \tchar buf[3] = { 0 };\n> >>  \tint n;\n> >\n> > Do you gain a lot by this extra condition? Wouldn't\n> >\n> > -#ifdef WIN32\n> > +#if defined(WIN32) || defined(__CYGWIN__)\n> >\n> > be sufficient?\n>\n> Yes, that would be sufficient. No, I probably don't gain a great deal\n> (but I have *not* timed it), since the number of files that are tested\n> by is_executable() is fairly low anyway since they are already filtered\n> by a filename prefix (eg. git-merge-).\n>\n> However, if the executable bit is set, then executing the WIN32 code\n> block is wasted effort (we already know the answer), so why bother?\n\nIt would have made to code a bit easier to read with one less #if defined(), \nbut it's not a big deal.\n\n-- Hannes\n"},{"id":"158342","messageId":"4D0D1C79.4070203@ramsay1.demon.co.uk","threadId":"26060","inReplyTo":"201012172246.45038.j6t@kdbg.org","subject":"Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-12-18T20:41:29Z","receivedAt":"2010-12-18T20:41:29Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Johannes Sixt wrote:\n> On Donnerstag, 16. Dezember 2010, Ramsay Jones wrote:\n>> Johannes Sixt wrote:\n>>> On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:\n>>>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)\n>>>>  \t    !S_ISREG(st.st_mode))\n>>>>  \t\treturn 0;\n>>>>\n>>>> -#ifdef WIN32\n>>>> +#if defined(WIN32) || defined(__CYGWIN__)\n>>>> +#if defined(__CYGWIN__)\n>>>> +if ((st.st_mode & S_IXUSR) == 0)\n>>>> +#endif\n>>>>  {\t/* cannot trust the executable bit, peek into the file instead */\n>>>>  \tchar buf[3] = { 0 };\n>>>>  \tint n;\n>>> Do you gain a lot by this extra condition? Wouldn't\n>>>\n>>> -#ifdef WIN32\n>>> +#if defined(WIN32) || defined(__CYGWIN__)\n>>>\n>>> be sufficient?\n>> Yes, that would be sufficient. No, I probably don't gain a great deal\n>> (but I have *not* timed it), since the number of files that are tested\n>> by is_executable() is fairly low anyway since they are already filtered\n>> by a filename prefix (eg. git-merge-).\n>>\n>> However, if the executable bit is set, then executing the WIN32 code\n>> block is wasted effort (we already know the answer), so why bother?\n> \n> It would have made to code a bit easier to read with one less #if defined(), \n> but it's not a big deal.\n\nYep, I did consider this:\n\n-#idef WIN32\n+#if defined(WIN32) || defined(__CYGWIN__)\n+if ((st.st_mode & S_IXUSR) == 0)\n\nbut chickened out! ;-) (No, I didn't even test it)\nI decided to be conservative here ...\n\nATB,\nRamsay Jones\n"}]}