Hi,

Ramsay Jones wrote:
 
> Signed-off-by: Ramsay Jones <ram...@ramsay1.demon.co.uk>

Let me try to understand this.

Before v1.8.1.1~7^2~2 (Update cygwin.c for new mingw-64 win32 api
headers, 2012-11-11), compat/cygwin.c did

        #define CYGWIN_C
        #define WIN32_LEAN_AND_MEAN
        #include "../git-compat-util.h"
        #include "win32.h"

Afterward, on modern Cygwin it changed to

        #define CYGWIN_C
        #define WIN32_LEAN_AND_MEAN
        #include <sys/stat.h>
        #include <sys/errno.h>
        #include "win32.h"
        #include "../git-compat-util.h"

compat/win32.h does

        #ifndef WIN32         /* Not defined by Cygwin */
        #include <windows.h>
        #endif

and then defines some inline functions relying on the win32 api.

git-compat-util.h defines various feature request macros and includes
many system headers, so that simple swap of lines was a huge change.
It's not obvious to me what part was important for making this work
with modern Cygwin win32api.  Mark or others, do you remember what
went wrong with the original "git-compat-util.h and then
compat/win32.h" order (e.g., did it break the build?  what was the
compiler message?) when upgrading win32api?

Unfortunately this was a breaking change: systems with the *old*
version of win32api would only compile with the old order of header
inclusions.  The new ordering produced the following symptom:

        In file included from compat/../git-compat-util.h:90,
                         from compat/cygwin.c:9:
        
/usr/lib/gcc/i686-pc-cygwin/3.4.4/../../../../include/w32api/winsock2.h:103:2: 
        warning: #warning "fd_set and associated macros have been defined in 
sys/types.      
        This may cause runtime problems with W32 sockets"
        In file included from /usr/include/sys/socket.h:16,
                         from compat/../git-compat-util.h:131,
                         from compat/cygwin.c:9:
        /usr/include/cygwin/socket.h:29: error: redefinition of `struct 
sockaddr'
 [...]

Alex Riesen (cc-ed) noticed that removing the following lines from
git-compat-util.h alleviated this new symptom:

        #ifdef WIN32 /* Both MinGW and MSVC */
        # if defined (_MSC_VER)
        #  define _WIN32_WINNT 0x0502
        # endif
        #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
        #include <winsock2.h>
        #include <windows.h>
        #endif

After all, on Cygwin there is no reason to include winsock or the
win32api from git-compat-util.h.  Presumably one of <sys/stat.h>,
<sys/errno.h>, and <windows.h> causes the WIN32 macro to be defined on
these systems with old win32api, and chaos ensues.

All in all, it wasn't a bad thing, since it revealed that the WIN32
macro doesn't mean what most of the git codebase assumes it means
(hence patch 1/2, which fixes that).

The reordering made in v1.8.1.1~7^2~2 still seems like voodoo to me,
but at least it works.  This patch applies that same order for
everyone.  Systems that would previously use the "I have old win32api
and don't need that reordering" codepath don't need to be
special-cased any more, since *their* particular brand of trouble is
avoided by being careful about how to use the WIN32 macro.

The upshot:

 - No change on modern setups.  To uninformed people like me I feel
   like there is still something subtle going on that is not well
   understood, but hey, this patch doesn't break it. :)

 - Tested to still work on setups that previously needed
   CYGWIN_V15_WIN32API.  Yay!

 - This drops an #ifdef, which means less code that is never tested
   to keep up to date.

With or without a few words of explanation in the commit message to
save some time for the next confused person looking this over,

Reviewed-by: Jonathan Nieder <jrnie...@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to