{"thread":{"id":"41983","subject":"[PATCH 0/3] index-helper: fix UNIX_PATH_MAX redefinition error on cygwin","startedAt":"2016-04-10T22:57:31Z","lastAt":"2016-04-12T01:46:16Z","messageCount":4,"participants":["Ramsay Jones","Jeff King","David Turner"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"283113","messageId":"570ADA5B.5030408@ramsayjones.plus.com","threadId":"41983","inReplyTo":null,"subject":"[PATCH 0/3] index-helper: fix UNIX_PATH_MAX redefinition error on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-04-10T22:57:31Z","receivedAt":"2016-04-10T22:57:31Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nHi David, Duy,\n\nIf you need to re-roll your 'dt/index-helper' branch, could you please\nconsider squashing these patches into the relevant patch (equivalent to\ncommit 12909da4 (\"index-helper: new daemon for caching index and related\nstuff\", 06-04-2016)).\n\nThe above commit causes the cygwin build to throw a warning (many times)\nabout the redefinition of the UNIX_PATH_MAX macro, viz:\n\n      CC credential-store.o\n  In file included from cache.h:4:0,\n                   from credential-store.c:1:\n  git-compat-util.h:1060:0: warning: \"UNIX_PATH_MAX\" redefined\n   #define UNIX_PATH_MAX 92\n   ^\n  In file included from git-compat-util.h:212:0,\n                   from cache.h:4,\n                   from credential-store.c:1:\n  /usr/include/sys/un.h:18:0: note: this is the location of the previous definition\n   #define UNIX_PATH_MAX   108\n   ^\n\nWhich is quite easy to confirm:\n\n  $ find /usr/include -iname '*.h' | xargs grep -n UNIX_PATH_MAX\n  /usr/include/sys/un.h:18:#define UNIX_PATH_MAX   108\n  /usr/include/sys/un.h:22:  char\t         sun_path[UNIX_PATH_MAX]; /* 108 bytes of socket address     */\n\n  $ gcc -dD -E git-compat-util.h 2>/dev/null | grep -n 'un\\.h'\n  8716:# 1 \"/usr/include/sys/un.h\" 1 3 4\n  8717:# 12 \"/usr/include/sys/un.h\" 3 4\n\n  $ gcc -dD -E git-compat-util.h 2>/dev/null | grep -n 'UNIX_PATH_MAX'\n  8724:#define UNIX_PATH_MAX 108\n  32675:#define UNIX_PATH_MAX 92\n\n  $ \n\nSo, a simple fix would look like:\n\n  diff --git a/git-compat-util.h b/git-compat-util.h\n  index 0e35c13..6d65132 100644\n  --- a/git-compat-util.h\n  +++ b/git-compat-util.h\n  @@ -1043,7 +1043,7 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n   #define getc_unlocked(fh) getc(fh)\n   #endif\n \n  -#ifdef __linux__\n  +#if defined(__linux__) || defined(__CYGWIN__)\n   #define UNIX_PATH_MAX 108\n   #elif defined(__APPLE__) || defined(BSD)\n   #define UNIX_PATH_MAX 104\n\n... since you are allowed to redefine a macro to the same token sequence\n(modulo some strange whitespace rules).\n\nHowever, assuming the <sys/un.h> header is defining the UNIX_PATH_MAX\nmacro, why even try to #define it again. So, maybe put an\n\n  #ifndef UNIX_PATH_MAX\n  ...\n  #endif\n\naround the entire block. But then, why on earth are you doing this at all?\n\nLet's look on linux:\n\n  $ find /usr/include -iname '*.h' | xargs grep -n UNIX_PATH_MAX\n  /usr/include/valgrind/vki/vki-linux.h:754:#define VKI_UNIX_PATH_MAX\t108\n  /usr/include/valgrind/vki/vki-linux.h:758:\tchar sun_path[VKI_UNIX_PATH_MAX];\t/* pathname */\n  /usr/include/linux/un.h:6:#define UNIX_PATH_MAX\t108\n  /usr/include/linux/un.h:10:\tchar sun_path[UNIX_PATH_MAX];\t/* pathname */\n\n  $ gcc -dD -E git-compat-util.h | grep -n 'un\\.h'\n  14119:# 1 \"/usr/include/x86_64-linux-gnu/sys/un.h\" 1 3 4\n  14120:# 19 \"/usr/include/x86_64-linux-gnu/sys/un.h\" 3 4\n\n  $ gcc -dD -E git-compat-util.h | grep -n 'UNIX_PATH_MAX'\n  36788:#define UNIX_PATH_MAX 108\n\n  $ \n\nAh, interesting, ... despite having a header that does indeed define the\nUNIX_PATH_MAX macro, the compiler is not using that header, but another\none which doesn't. Well, blow me down. ;-)\n\nSo, the approach taken by patch #1 is to forget about UNIX_PATH_MAX and\nsimply use sizeof(address.sun_path) instead!\n\nThe second and third patches are simply 'mild suggestions' for other\nminor issues I noticed while looking into these warnings.\n\nATB,\nRamsay Jones\n\nRamsay Jones (3):\n  index-helper: fix UNIX_PATH_MAX redefinition error on cygwin\n  index-helper: convert strncpy to memcpy\n  index-helper: take extra care with readlink\n\n git-compat-util.h | 17 -----------------\n index-helper.c    |  4 ++--\n read-cache.c      | 15 +++++++++++++--\n 3 files changed, 15 insertions(+), 21 deletions(-)\n\n-- \n2.8.0\n"},{"id":"283137","messageId":"20160411133343.GA7492@sigill.intra.peff.net","threadId":"41983","inReplyTo":"570ADA5B.5030408@ramsayjones.plus.com","subject":"Re: [PATCH 0/3] index-helper: fix UNIX_PATH_MAX redefinition error on cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-11T13:33:44Z","receivedAt":"2016-04-11T13:33:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 10, 2016 at 11:57:31PM +0100, Ramsay Jones wrote:\n\n> So, the approach taken by patch #1 is to forget about UNIX_PATH_MAX and\n> simply use sizeof(address.sun_path) instead!\n\nThat's what the existing code in unix-socket.c does. Which makes me\nwonder why the index-helper code is not simply calling that.\n\n-Peff\n"},{"id":"283156","messageId":"1460410162.5540.20.camel@twopensource.com","threadId":"41983","inReplyTo":"20160411133343.GA7492@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] index-helper: fix UNIX_PATH_MAX redefinition error on cygwin","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-11T21:29:22Z","receivedAt":"2016-04-11T21:29:22Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2016-04-11 at 09:33 -0400, Jeff King wrote:\n> On Sun, Apr 10, 2016 at 11:57:31PM +0100, Ramsay Jones wrote:\n> \n> > So, the approach taken by patch #1 is to forget about UNIX_PATH_MAX\n> > and\n> > simply use sizeof(address.sun_path) instead!\n> \n> That's what the existing code in unix-socket.c does. Which makes me\n> wonder why the index-helper code is not simply calling that.\n\nBecause I didn't notice it at the time.  Duy pointed it out on another\ncomment on this series; I'll fix it.\n"},{"id":"283163","messageId":"20160412014616.GA9604@sigill.intra.peff.net","threadId":"41983","inReplyTo":"1460410162.5540.20.camel@twopensource.com","subject":"Re: [PATCH 0/3] index-helper: fix UNIX_PATH_MAX redefinition error on cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-12T01:46:16Z","receivedAt":"2016-04-12T01:46:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 11, 2016 at 05:29:22PM -0400, David Turner wrote:\n\n> On Mon, 2016-04-11 at 09:33 -0400, Jeff King wrote:\n> > On Sun, Apr 10, 2016 at 11:57:31PM +0100, Ramsay Jones wrote:\n> > \n> > > So, the approach taken by patch #1 is to forget about UNIX_PATH_MAX\n> > > and\n> > > simply use sizeof(address.sun_path) instead!\n> > \n> > That's what the existing code in unix-socket.c does. Which makes me\n> > wonder why the index-helper code is not simply calling that.\n> \n> Because I didn't notice it at the time.  Duy pointed it out on another\n> comment on this series; I'll fix it.\n\nThanks. I _think_ it should do everything you need already, but I'm\nhappy to review any tweaks you need to make to it.\n\n-Peff\n"}]}