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

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> -#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;
> 
> This looks somewhat ugly.

;-) Yeah, Johannes Sixt had a similar complaint last time ...

> I guess we could make the inner #if/#endif slightly more readable by
> letting the compiler do more work, like this:
> 
> 	#if defined(WIN32) || defined(__CYGWIN__)
>         if (!defined(__CYGWIN__) || !(st.st_mode & S_IXUSR)) {
              ^^^^^^^^^
This would have to be something like:

    #if defined(__CYGWIN__)
    #define IS_CYGWIN 1
    #else
    #define IS_CYGWIN 0
    #endif
    if (!IS_CYGWIN || !(st.st_mode & S_IXUSR)) {

no?
>         	...
> 	}
>         #endif
> 
> I dunno.

The first version of this patch looked like:

-#ifdef WIN32
+#if defined(WIN32) || defined(__CYGWIN__)
+if ((st.st_mode & S_IXUSR) == 0)
 {	/* cannot trust the executable bit, peek into the file instead */

The idea being that the WIN32 builds (ie MinGW and MSVC) would never set
the executable bit, so this should only be false on cygwin when *not*
using the WIN32 l/stat implementation. So it should be safe ...

However, I got cold feet (and being lazy didn't want to test on MinGW and
MSVC) and decided on the more conservative approach.

Anyway, if you don't mind executing the "WIN32 code block" unnecessarily
on cygwin (I don't think it would be too expensive) then we could simply
reduce the patch to:

-#ifdef WIN32
+#if defined(WIN32) || defined(__CYGWIN__)
 {	/* cannot trust the executable bit, peek into the file instead */

(I've simply typed the above in my MUA, so not tested, obviously!)

This is exactly what Johannes proposed last year. :)

ATB,
Ramsay Jones