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

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Jun 18, 2011, 16:57 UTC
Message-ID: <4DFCD8E1.3060102@ramsay1.demon.co.uk>
In-Reply-To: <7v1uyth0t9.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 12 quoted lines

> 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 ...

Show 5 quoted lines

> 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?

Show 5 quoted lines

>         	...
> 	}
>         #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