Hi, Ramsay Jones wrote: > Signed-off-by: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxx> 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 <jrnieder@xxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html