[PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin

[PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin

[Ramsay Jones]

Test t7606-merge-custom.sh fails on cygwin when git-merge fails
with an "Could not find merge strategy 'theirs'" error, despite
the test correctly preparing an (executable) git-merge-theirs
script.

The cause of the failure is the mis-detection of the executable
status of the script, by the is_executable() function, while the
load_command_list() function is searching the path for additional
merge strategy programs.

Note that the l/stat() "functions" on cygwin are somewhat
schizophrenic (see commits adbc0b6, 7faee6b and 7974843), and
their behaviour depends on the timing of various git setup and
config function calls. In particular, until the "git_dir" has
been set (have_git_dir() returns true), the real cygwin (POSIX
emulating) l/stat() functions are called. Once "git_dir" has
been set, the "native Win32 API" implementations of l/stat()
may, or may not, be called depending on the setting of the
core.filemode and core.ignorecygwinfstricks config variables.

We also note that, since commit c869753, core.filemode is forced
to false, even on NTFS, by git-init and git-clone. A user (or a
test) can, of course, reset core.filemode to true explicitly if
the filesystem supports it (and he doesn't use any problematic
windows software). The test-suite currently runs all tests on
cygwin with core.filemode set to false.

Given the above, we see that the built-in merge strategies are
correctly detected as executable, since they are checked for
before "git_dir" is set, whereas all custom merge strategies are
not, since they are checked for after "git_dir" is set.

In order to fix the mis-detection problem, we change the code in
is_executable() to re-use the conditional WIN32 code section,
which actually looks at the content of the file to determine if
the file is executable. On cygwin we also make the additional
code conditional on the executable bit of the file mode returned
by the initial stat() call. (only the real cygwin function would
set the executable bit in the file mode.)

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 help.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/help.c b/help.c
index 7f4928e..eabadc9 100644
--- a/help.c
+++ b/help.c
@@ -126,7 +126,10 @@ static int is_executable(const char *name)
 	    !S_ISREG(st.st_mode))
 		return 0;
 
-#ifdef WIN32
+#if defined(WIN32) || defined(__CYGWIN__)
+#if defined(__CYGWIN__)
+if ((st.st_mode & S_IXUSR) == 0)
+#endif
 {	/* cannot trust the executable bit, peek into the file instead */
 	char buf[3] = { 0 };
 	int n;
-- 
1.7.3

Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin

[Erik Faye-Lund]

On Tue, Dec 14, 2010 at 7:29 PM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
>
> Test t7606-merge-custom.sh fails on cygwin when git-merge fails
> with an "Could not find merge strategy 'theirs'" error, despite
> the test correctly preparing an (executable) git-merge-theirs
> script.
>
> The cause of the failure is the mis-detection of the executable
> status of the script, by the is_executable() function, while the
> load_command_list() function is searching the path for additional
> merge strategy programs.
>
> Note that the l/stat() "functions" on cygwin are somewhat
> schizophrenic (see commits adbc0b6, 7faee6b and 7974843), and
> their behaviour depends on the timing of various git setup and
> config function calls. In particular, until the "git_dir" has
> been set (have_git_dir() returns true), the real cygwin (POSIX
> emulating) l/stat() functions are called. Once "git_dir" has
> been set, the "native Win32 API" implementations of l/stat()
> may, or may not, be called depending on the setting of the
> core.filemode and core.ignorecygwinfstricks config variables.
>
> We also note that, since commit c869753, core.filemode is forced
> to false, even on NTFS, by git-init and git-clone. A user (or a
> test) can, of course, reset core.filemode to true explicitly if
> the filesystem supports it (and he doesn't use any problematic
> windows software). The test-suite currently runs all tests on
> cygwin with core.filemode set to false.
>
> Given the above, we see that the built-in merge strategies are
> correctly detected as executable, since they are checked for
> before "git_dir" is set, whereas all custom merge strategies are
> not, since they are checked for after "git_dir" is set.
>
> In order to fix the mis-detection problem, we change the code in
> is_executable() to re-use the conditional WIN32 code section,
> which actually looks at the content of the file to determine if
> the file is executable. On cygwin we also make the additional
> code conditional on the executable bit of the file mode returned
> by the initial stat() call. (only the real cygwin function would
> set the executable bit in the file mode.)
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  help.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/help.c b/help.c
> index 7f4928e..eabadc9 100644
> --- a/help.c
> +++ b/help.c
> @@ -126,7 +126,10 @@ static int is_executable(const char *name)
>            !S_ISREG(st.st_mode))
>                return 0;
>
> -#ifdef WIN32
> +#if defined(WIN32) || defined(__CYGWIN__)
> +#if defined(__CYGWIN__)
> +if ((st.st_mode & S_IXUSR) == 0)
> +#endif

Perhaps the first check should simply be changed to check for _WIN32
instead of WIN32? IIRC _WIN32 is set on Cygwin, but I could be
mistaken...

Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin

[Johannes Sixt]

On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:
> @@ -126,7 +126,10 @@ static int is_executable(const char *name)
>  	    !S_ISREG(st.st_mode))
>  		return 0;
>
> -#ifdef WIN32
> +#if defined(WIN32) || defined(__CYGWIN__)
> +#if defined(__CYGWIN__)
> +if ((st.st_mode & S_IXUSR) == 0)
> +#endif
>  {	/* cannot trust the executable bit, peek into the file instead */
>  	char buf[3] = { 0 };
>  	int n;

Do you gain a lot by this extra condition? Wouldn't

-#ifdef WIN32
+#if defined(WIN32) || defined(__CYGWIN__)

be sufficient?

-- Hannes

Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin

[Ramsay Jones]

Erik Faye-Lund wrote:
>> diff --git a/help.c b/help.c
>> index 7f4928e..eabadc9 100644
>> --- a/help.c
>> +++ b/help.c
>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)
>>            !S_ISREG(st.st_mode))
>>                return 0;
>>
>> -#ifdef WIN32
>> +#if defined(WIN32) || defined(__CYGWIN__)
>> +#if defined(__CYGWIN__)
>> +if ((st.st_mode & S_IXUSR) == 0)
>> +#endif
> 
> Perhaps the first check should simply be changed to check for _WIN32
> instead of WIN32? IIRC _WIN32 is set on Cygwin, but I could be
> mistaken...

No, neither WIN32 or _WIN32 will be defined here (and they should not be).
It's actually quite tricky, particularly when #including <windows.h>, viz:

    $ cat -n test.c
     1	#include <stdio.h>
     2	
     3	#ifdef IW
     4	# include <windows.h>
     5	#endif
     6	
     7	int main(int argc, char *argv[])
     8	{
     9	#ifdef WIN32
    10		printf("WIN32 ");
    11	#endif
    12	#ifdef _WIN32
    13		printf("_WIN32 ");
    14	#endif
    15	#ifdef __CYGWIN__
    16		printf("__CYGWIN__ ");
    17	#endif
    18	#ifdef __MINGW32__
    19		printf("__MINGW32__ ");
    20	#endif
    21		printf("\n");
    22		return 0;
    23	}

    $ gcc -o test test.c
    $ ./test
    __CYGWIN__ 

    $ gcc -o test -DIW test.c
    $ ./test
    WIN32 _WIN32 __CYGWIN__ 

    $ gcc -o test -mno-cygwin test.c
    $ ./test
    WIN32 _WIN32 __MINGW32__ 

ATB,
Ramsay Jones

Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin

[Ramsay Jones]

Johannes Sixt wrote:
> On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:
>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)
>>  	    !S_ISREG(st.st_mode))
>>  		return 0;
>>
>> -#ifdef WIN32
>> +#if defined(WIN32) || defined(__CYGWIN__)
>> +#if defined(__CYGWIN__)
>> +if ((st.st_mode & S_IXUSR) == 0)
>> +#endif
>>  {	/* cannot trust the executable bit, peek into the file instead */
>>  	char buf[3] = { 0 };
>>  	int n;
> 
> Do you gain a lot by this extra condition? Wouldn't
> 
> -#ifdef WIN32
> +#if defined(WIN32) || defined(__CYGWIN__)
> 
> be sufficient?

Yes, that would be sufficient. No, I probably don't gain a great deal
(but I have *not* timed it), since the number of files that are tested
by is_executable() is fairly low anyway since they are already filtered
by a filename prefix (eg. git-merge-).

However, if the executable bit is set, then executing the WIN32 code
block is wasted effort (we already know the answer), so why bother?

ATB,
Ramsay Jones

Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin

[Erik Faye-Lund]

On Thu, Dec 16, 2010 at 10:03 PM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
> Erik Faye-Lund wrote:
>>> diff --git a/help.c b/help.c
>>> index 7f4928e..eabadc9 100644
>>> --- a/help.c
>>> +++ b/help.c
>>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)
>>>            !S_ISREG(st.st_mode))
>>>                return 0;
>>>
>>> -#ifdef WIN32
>>> +#if defined(WIN32) || defined(__CYGWIN__)
>>> +#if defined(__CYGWIN__)
>>> +if ((st.st_mode & S_IXUSR) == 0)
>>> +#endif
>>
>> Perhaps the first check should simply be changed to check for _WIN32
>> instead of WIN32? IIRC _WIN32 is set on Cygwin, but I could be
>> mistaken...
>
> No, neither WIN32 or _WIN32 will be defined here (and they should not be).
> It's actually quite tricky, particularly when #including <windows.h>, viz:
>
>    $ cat -n test.c
>     1  #include <stdio.h>
>     2
>     3  #ifdef IW
>     4  # include <windows.h>
>     5  #endif
>     6
>     7  int main(int argc, char *argv[])
>     8  {
>     9  #ifdef WIN32
>    10          printf("WIN32 ");
>    11  #endif
>    12  #ifdef _WIN32
>    13          printf("_WIN32 ");
>    14  #endif
>    15  #ifdef __CYGWIN__
>    16          printf("__CYGWIN__ ");
>    17  #endif
>    18  #ifdef __MINGW32__
>    19          printf("__MINGW32__ ");
>    20  #endif
>    21          printf("\n");
>    22          return 0;
>    23  }
>
>    $ gcc -o test test.c
>    $ ./test
>    __CYGWIN__
>
>    $ gcc -o test -DIW test.c
>    $ ./test
>    WIN32 _WIN32 __CYGWIN__
>
>    $ gcc -o test -mno-cygwin test.c
>    $ ./test
>    WIN32 _WIN32 __MINGW32__
>

Hmm, I thought _WIN32 was always defined when targeting Windows, and
that WIN32 was defined when windows.h was included (or usually in the
preprocessor flags when compiling GUI programs) - that's what MSVC
does anyway. MinGW seems to always define both _WIN32 and WIN32, but
as you've shown Cygwin doesn't define either until windows.h is
included.

So sorry for leading you onto the wrong path, a check for __CYGWIN__
seems to be necessary indeed.

Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin

[Johannes Sixt]

On Donnerstag, 16. Dezember 2010, Ramsay Jones wrote:
> Johannes Sixt wrote:
> > On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:
> >> @@ -126,7 +126,10 @@ static int is_executable(const char *name)
> >>  	    !S_ISREG(st.st_mode))
> >>  		return 0;
> >>
> >> -#ifdef WIN32
> >> +#if defined(WIN32) || defined(__CYGWIN__)
> >> +#if defined(__CYGWIN__)
> >> +if ((st.st_mode & S_IXUSR) == 0)
> >> +#endif
> >>  {	/* cannot trust the executable bit, peek into the file instead */
> >>  	char buf[3] = { 0 };
> >>  	int n;
> >
> > Do you gain a lot by this extra condition? Wouldn't
> >
> > -#ifdef WIN32
> > +#if defined(WIN32) || defined(__CYGWIN__)
> >
> > be sufficient?
>
> Yes, that would be sufficient. No, I probably don't gain a great deal
> (but I have *not* timed it), since the number of files that are tested
> by is_executable() is fairly low anyway since they are already filtered
> by a filename prefix (eg. git-merge-).
>
> However, if the executable bit is set, then executing the WIN32 code
> block is wasted effort (we already know the answer), so why bother?

It would have made to code a bit easier to read with one less #if defined(), 
but it's not a big deal.

-- Hannes

Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin

[Ramsay Jones]

Johannes Sixt wrote:
> On Donnerstag, 16. Dezember 2010, Ramsay Jones wrote:
>> Johannes Sixt wrote:
>>> On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:
>>>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)
>>>>  	    !S_ISREG(st.st_mode))
>>>>  		return 0;
>>>>
>>>> -#ifdef WIN32
>>>> +#if defined(WIN32) || defined(__CYGWIN__)
>>>> +#if defined(__CYGWIN__)
>>>> +if ((st.st_mode & S_IXUSR) == 0)
>>>> +#endif
>>>>  {	/* cannot trust the executable bit, peek into the file instead */
>>>>  	char buf[3] = { 0 };
>>>>  	int n;
>>> Do you gain a lot by this extra condition? Wouldn't
>>>
>>> -#ifdef WIN32
>>> +#if defined(WIN32) || defined(__CYGWIN__)
>>>
>>> be sufficient?
>> Yes, that would be sufficient. No, I probably don't gain a great deal
>> (but I have *not* timed it), since the number of files that are tested
>> by is_executable() is fairly low anyway since they are already filtered
>> by a filename prefix (eg. git-merge-).
>>
>> However, if the executable bit is set, then executing the WIN32 code
>> block is wasted effort (we already know the answer), so why bother?
> 
> It would have made to code a bit easier to read with one less #if defined(), 
> but it's not a big deal.

Yep, I did consider this:

-#idef WIN32
+#if defined(WIN32) || defined(__CYGWIN__)
+if ((st.st_mode & S_IXUSR) == 0)

but chickened out! ;-) (No, I didn't even test it)
I decided to be conservative here ...

ATB,
Ramsay Jones