threads / patch / 26060

patch, 14 partshelp.c: Fix detection of custom merge strategy on cygwin

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

## tl;dr

8 messages between Dec 14, 2010 and Dec 18, 2010. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Ramsay Jones· Dec 14, 2010, 18:29 UTC · lore

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(-)
Show changes to help.c +4 −1
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
Erik Faye-Lund· Dec 14, 2010, 20:07 UTC · re: Ramsay Jones · lore

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

On Tue, Dec 14, 2010 at 7:29 PM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote:

Show 59 quoted lines
>
> 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...

Ramsay Jones· Dec 16, 2010, 21:03 UTC · re: Erik Faye-Lund · lore

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

Erik Faye-Lund wrote:
Show 17 quoted lines
>> 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

Erik Faye-Lund· Dec 16, 2010, 23:06 UTC · re: Ramsay Jones · lore

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

On Thu, Dec 16, 2010 at 10:03 PM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote:

Show 59 quoted lines
> 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.

Johannes Sixt· Dec 14, 2010, 20:38 UTC · re: Ramsay Jones · lore

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

On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:
Show 12 quoted lines
> @@ -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
Ramsay Jones· Dec 16, 2010, 21:12 UTC · re: Johannes Sixt · lore

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

Johannes Sixt wrote:
Show 20 quoted lines
> 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

Johannes Sixt· Dec 17, 2010, 21:46 UTC · re: Ramsay Jones · lore

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

On Donnerstag, 16. Dezember 2010, Ramsay Jones wrote:
Show 29 quoted lines
> 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
Ramsay Jones· Dec 18, 2010, 20:41 UTC · re: Johannes Sixt · lore

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

Johannes Sixt wrote:
Show 31 quoted lines
> 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

← back to recent threads