{"thread":{"id":"27640","subject":"[PATCH 1/3] help.c: Fix detection of custom merge strategy on cygwin","startedAt":"2011-06-16T20:22:16Z","lastAt":"2011-06-18T22:46:16Z","messageCount":4,"participants":["Ramsay Jones","Junio C Hamano","Mark Levedahl"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"170159","messageId":"4DFA65F8.5010806@ramsay1.demon.co.uk","threadId":"27640","inReplyTo":null,"subject":"[PATCH 1/3] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-06-16T20:22:16Z","receivedAt":"2011-06-16T20:22:16Z","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 \n---\n help.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 7654f1b..e925ca1 100644\n--- a/help.c\n+++ b/help.c\n@@ -127,7 +127,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.5\n"},{"id":"170170","messageId":"7v1uyth0t9.fsf@alter.siamese.dyndns.org","threadId":"27640","inReplyTo":"4DFA65F8.5010806@ramsay1.demon.co.uk","subject":"Re: [PATCH 1/3] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-16T22:24:34Z","receivedAt":"2011-06-16T22:24:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\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\nThis looks somewhat ugly.\n\nI guess we could make the inner #if/#endif slightly more readable by\nletting the compiler do more work, like this:\n\n\t#if defined(WIN32) || defined(__CYGWIN__)\n if (!defined(__CYGWIN__) || !(st.st_mode & S_IXUSR)) {\n \t...\n\t}\n #endif\n\nI dunno.\n"},{"id":"170212","messageId":"4DFCD8E1.3060102@ramsay1.demon.co.uk","threadId":"27640","inReplyTo":"7v1uyth0t9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-06-18T16:57:05Z","receivedAt":"2011-06-18T16:57:05Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Junio C Hamano wrote:\n> Ramsay Jones writes:\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> This looks somewhat ugly.\n\n;-) Yeah, Johannes Sixt had a similar complaint last time ...\n\n> I guess we could make the inner #if/#endif slightly more readable by\n> letting the compiler do more work, like this:\n> \n> \t#if defined(WIN32) || defined(__CYGWIN__)\n> if (!defined(__CYGWIN__) || !(st.st_mode & S_IXUSR)) {\n ^^^^^^^^^\nThis would have to be something like:\n\n #if defined(__CYGWIN__)\n #define IS_CYGWIN 1\n #else\n #define IS_CYGWIN 0\n #endif\n if (!IS_CYGWIN || !(st.st_mode & S_IXUSR)) {\n\nno?\n> \t...\n> \t}\n> #endif\n> \n> I dunno.\n\nThe first version of this patch looked like:\n\n-#ifdef WIN32\n+#if defined(WIN32) || defined(__CYGWIN__)\n+if ((st.st_mode & S_IXUSR) == 0)\n {\t/* cannot trust the executable bit, peek into the file instead */\n\nThe idea being that the WIN32 builds (ie MinGW and MSVC) would never set\nthe executable bit, so this should only be false on cygwin when *not*\nusing the WIN32 l/stat implementation. So it should be safe ...\n\nHowever, I got cold feet (and being lazy didn't want to test on MinGW and\nMSVC) and decided on the more conservative approach.\n\nAnyway, if you don't mind executing the \"WIN32 code block\" unnecessarily\non cygwin (I don't think it would be too expensive) then we could simply\nreduce the patch to:\n\n-#ifdef WIN32\n+#if defined(WIN32) || defined(__CYGWIN__)\n {\t/* cannot trust the executable bit, peek into the file instead */\n\n(I've simply typed the above in my MUA, so not tested, obviously!)\n\nThis is exactly what Johannes proposed last year. :)\n\nATB,\nRamsay Jones\n"},{"id":"170223","messageId":"4DFD2AB8.2050907@gmail.com","threadId":"27640","inReplyTo":"4DFCD8E1.3060102@ramsay1.demon.co.uk","subject":"Re: [PATCH 1/3] help.c: Fix detection of custom merge strategy on cygwin","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2011-06-18T22:46:16Z","receivedAt":"2011-06-18T22:46:16Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 06/18/2011 12:57 PM, Ramsay Jones wrote:\n>\n> Anyway, if you don't mind executing the \"WIN32 code block\" unnecessarily\n> on cygwin (I don't think it would be too expensive) then we could simply\n> reduce the patch to:\n>\n> -#ifdef WIN32\n> +#if defined(WIN32) || defined(__CYGWIN__)\n> {\t/* cannot trust the executable bit, peek into the file instead */\n>\n> (I've simply typed the above in my MUA, so not tested, obviously!)\n>\n> This is exactly what Johannes proposed last year. :)\n>\n> ATB,\n> Ramsay Jones\n>\n\nPlease, no. Cygwin's git is already slow enough, and adding yet another \npeek into a file *after* cygwin already did that is just making it worse.\n\n(Muttering to self: Cygwin's goal is to replicate Linux / Posix behavior \nunder Windows, why did git ever adopt code to make cygwin more like \nWindows rather than just treating cygwin as another Posix'y environment? \nThose who want more speed really should just be using the native win32 \nport and deal with the lack of file modes, crlf issues, etc.)\n\nMark\n"}]}