threads / patch / 41983

patch, 3 partsindex-helper: fix UNIX_PATH_MAX redefinition error on cygwin

Subject: [PATCH 0/3] index-helper: fix UNIX_PATH_MAX redefinition error on cygwin

## tl;dr

4 messages between Apr 10, 2016 and Apr 12, 2016. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Ramsay Jones· Apr 10, 2016, 22:57 UTC · lore
Hi David, Duy,

If you need to re-roll your 'dt/index-helper' branch, could you please consider squashing these patches into the relevant patch (equivalent to commit 12909da4 ("index-helper: new daemon for caching index and related stuff", 06-04-2016)).

The above commit causes the cygwin build to throw a warning (many times) about the redefinition of the UNIX_PATH_MAX macro, viz:

      CC credential-store.o
  In file included from cache.h:4:0,
                   from credential-store.c:1:
  git-compat-util.h:1060:0: warning: "UNIX_PATH_MAX" redefined
   #define UNIX_PATH_MAX 92
   ^
  In file included from git-compat-util.h:212:0,
                   from cache.h:4,
                   from credential-store.c:1:
  /usr/include/sys/un.h:18:0: note: this is the location of the previous definition
   #define UNIX_PATH_MAX   108
   ^
Which is quite easy to confirm:
  $ find /usr/include -iname '*.h' | xargs grep -n UNIX_PATH_MAX
  /usr/include/sys/un.h:18:#define UNIX_PATH_MAX   108
  /usr/include/sys/un.h:22:  char	         sun_path[UNIX_PATH_MAX]; /* 108 bytes of socket address     */
  $ gcc -dD -E git-compat-util.h 2>/dev/null | grep -n 'un\.h'
  8716:# 1 "/usr/include/sys/un.h" 1 3 4
  8717:# 12 "/usr/include/sys/un.h" 3 4
  $ gcc -dD -E git-compat-util.h 2>/dev/null | grep -n 'UNIX_PATH_MAX'
  8724:#define UNIX_PATH_MAX 108
  32675:#define UNIX_PATH_MAX 92
  $ 
So, a simple fix would look like:
  diff --git a/git-compat-util.h b/git-compat-util.h
  index 0e35c13..6d65132 100644
  --- a/git-compat-util.h
  +++ b/git-compat-util.h
  @@ -1043,7 +1043,7 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
   #define getc_unlocked(fh) getc(fh)
   #endif
 
  -#ifdef __linux__
  +#if defined(__linux__) || defined(__CYGWIN__)
   #define UNIX_PATH_MAX 108
   #elif defined(__APPLE__) || defined(BSD)
   #define UNIX_PATH_MAX 104

... since you are allowed to redefine a macro to the same token sequence (modulo some strange whitespace rules).

However, assuming the <sys/un.h> header is defining the UNIX_PATH_MAX macro, why even try to #define it again. So, maybe put an

  #ifndef UNIX_PATH_MAX
  ...
  #endif
around the entire block. But then, why on earth are you doing this at all?
Let's look on linux:
  $ find /usr/include -iname '*.h' | xargs grep -n UNIX_PATH_MAX
  /usr/include/valgrind/vki/vki-linux.h:754:#define VKI_UNIX_PATH_MAX	108
  /usr/include/valgrind/vki/vki-linux.h:758:	char sun_path[VKI_UNIX_PATH_MAX];	/* pathname */
  /usr/include/linux/un.h:6:#define UNIX_PATH_MAX	108
  /usr/include/linux/un.h:10:	char sun_path[UNIX_PATH_MAX];	/* pathname */
  $ gcc -dD -E git-compat-util.h | grep -n 'un\.h'
  14119:# 1 "/usr/include/x86_64-linux-gnu/sys/un.h" 1 3 4
  14120:# 19 "/usr/include/x86_64-linux-gnu/sys/un.h" 3 4
  $ gcc -dD -E git-compat-util.h | grep -n 'UNIX_PATH_MAX'
  36788:#define UNIX_PATH_MAX 108
  $ 

Ah, interesting, ... despite having a header that does indeed define the UNIX_PATH_MAX macro, the compiler is not using that header, but another one which doesn't. Well, blow me down. ;-)

So, the approach taken by patch #1 is to forget about UNIX_PATH_MAX and simply use sizeof(address.sun_path) instead!

The second and third patches are simply 'mild suggestions' for other minor issues I noticed while looking into these warnings.

ATB, Ramsay Jones

Ramsay Jones (3):
  index-helper: fix UNIX_PATH_MAX redefinition error on cygwin
  index-helper: convert strncpy to memcpy
  index-helper: take extra care with readlink
 git-compat-util.h | 17 -----------------
 index-helper.c    |  4 ++--
 read-cache.c      | 15 +++++++++++++--
 3 files changed, 15 insertions(+), 21 deletions(-)
-- 
2.8.0
Jeff King· Apr 11, 2016, 13:33 UTC · re: Ramsay Jones · lore

Re: [PATCH 0/3] index-helper: fix UNIX_PATH_MAX redefinition error on cygwin

On Sun, Apr 10, 2016 at 11:57:31PM +0100, Ramsay Jones wrote:
> So, the approach taken by patch #1 is to forget about UNIX_PATH_MAX and
> simply use sizeof(address.sun_path) instead!

That's what the existing code in unix-socket.c does. Which makes me wonder why the index-helper code is not simply calling that.

-Peff
David Turner· Apr 11, 2016, 21:29 UTC · re: Jeff King · lore

Re: [PATCH 0/3] index-helper: fix UNIX_PATH_MAX redefinition error on cygwin

On Mon, 2016-04-11 at 09:33 -0400, Jeff King wrote:
Show 8 quoted lines
> On Sun, Apr 10, 2016 at 11:57:31PM +0100, Ramsay Jones wrote:
> 
> > So, the approach taken by patch #1 is to forget about UNIX_PATH_MAX
> > and
> > simply use sizeof(address.sun_path) instead!
> 
> That's what the existing code in unix-socket.c does. Which makes me
> wonder why the index-helper code is not simply calling that.

Because I didn't notice it at the time. Duy pointed it out on another comment on this series; I'll fix it.

Jeff King· Apr 12, 2016, 01:46 UTC · re: David Turner · lore

Re: [PATCH 0/3] index-helper: fix UNIX_PATH_MAX redefinition error on cygwin

On Mon, Apr 11, 2016 at 05:29:22PM -0400, David Turner wrote:
Show 12 quoted lines
> On Mon, 2016-04-11 at 09:33 -0400, Jeff King wrote:
> > On Sun, Apr 10, 2016 at 11:57:31PM +0100, Ramsay Jones wrote:
> > 
> > > So, the approach taken by patch #1 is to forget about UNIX_PATH_MAX
> > > and
> > > simply use sizeof(address.sun_path) instead!
> > 
> > That's what the existing code in unix-socket.c does. Which makes me
> > wonder why the index-helper code is not simply calling that.
> 
> Because I didn't notice it at the time.  Duy pointed it out on another
> comment on this series; I'll fix it.

Thanks. I _think_ it should do everything you need already, but I'm happy to review any tweaks you need to make to it.

-Peff

← back to recent threads