50a6c8ef - xmalloc_array

I haven't digged further,
but trying to verify t9115 under Windows gave this:


compat/mingw.c: In function 'mingw_spawnve_fd':
compat/mingw.c:1072:10: warning: implicit declaration of function 
'xmalloc_array' [-Wimplicit-function-declaration]
   wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
           ^
compat/mingw.c:1072:8: warning: assignment makes pointer from integer without a 
cast [-Wint-conversion]
   wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
         ^

--- and later ---

libgit.a(mingw.o): In function `mingw_spawnve_fd':
..... compat/mingw.c:1072: undefined reference to `xmalloc_array'
collect2.exe: error: ld returned 1 exit status
Makefile:2014: recipe for target 'git-credential-store.exe' failed
make: *** [git-credential-store.exe] Error 1

On Mon, Feb 29, 2016 at 07:30:02AM +0100, Torsten Bögershausen wrote:

I haven't digged further,
but trying to verify t9115 under Windows gave this:

compat/mingw.c: In function 'mingw_spawnve_fd':
compat/mingw.c:1072:10: warning: implicit declaration of function
'xmalloc_array' [-Wimplicit-function-declaration]
  wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
          ^

Yikes.

-- >8 --
Subject: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e

Commit 50a6c8e (use st_add and st_mult for allocation size
computation, 2016-02-22) fixed up many xmalloc call-sites
including ones in compat/mingw.c.

But I screwed up one of them, which was half-converted to
ALLOC_ARRAY, using a very early prototype of the function.
And I never caught it because I don't build on Windows.

Signed-off-by: Jeff King <redacted>
---
I think this means "master" is broken for mingw builds.

Sorry, Windows people, for breaking your build. I'm happy to hold back
such repo-wide cleanups from the mingw code in the future, since I can't
actually compile them. But the flipside is that if I _do_ improve
things, you don't get the benefit until somebody manually ports it over.

 compat/mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index cfedcf9..54c82ec 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -1069,7 +1069,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
 			free(quoted);
 	}
 
-	wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
+	ALLOC_ARRAY(wargs, st_add(st_mult(2, args.len), 1));
 	xutftowcs(wargs, args.buf, 2 * args.len + 1);
 	strbuf_release(&args);
 

-- 
2.8.0.rc0.276.gddf4100

Thanks for the fast-patch.

I manually copied the patch, But there are more probles, it seems.

$ git diff

diff --git a/compat/mingw.c b/compat/mingw.c
index cfedcf9..b1163f2 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -1069,7 +1069,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char 

**argv, char **deltaen
                         free(quoted);
         }

-       wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
+       wargs = ALLOC_ARRAY(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
         xutftowcs(wargs, args.buf, 2 * args.len + 1);
         strbuf_release(&args);


tb@torbogwm MINGW64 ~/Users/tb/projects/git/tb ((2273582...))
$ make
     CC compat/mingw.o
In file included from compat/mingw.c:1:0:
compat/mingw.c: In function 'mingw_spawnve_fd':
compat/../git-compat-util.h:769:60: error: invalid type argument of unary '*' 
(have 'size_t {aka long long unsigned int}')
  #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
                                                             ^
compat/mingw.c:1072:10: note: in expansion of macro 'ALLOC_ARRAY'
   wargs = ALLOC_ARRAY(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
           ^
Makefile:1948: recipe for target 'compat/mingw.o' failed
make: *** [compat/mingw.o] Error 1

On Mon, Feb 29, 2016 at 10:56:22AM +0100, Torsten Bögershausen wrote:

Argh. Let me write "git commit -a" on the inside of my brown paper bag,
so that I actually send out the fix sitting in my working tree, not the
half-finished thing I ran "git add" on.

-- >8 --
Subject: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e

Commit 50a6c8e (use st_add and st_mult for allocation size
computation, 2016-02-22) fixed up many xmalloc call-sites
including ones in compat/mingw.c.

But I screwed up one of them, which was half-converted to
ALLOC_ARRAY, using a very early prototype of the function.
And I never caught it because I don't build on Windows.

Signed-off-by: Jeff King <redacted>
---
 compat/mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index cfedcf9..54c82ec 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -1069,7 +1069,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
 			free(quoted);
 	}
 
-	wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
+	ALLOC_ARRAY(wargs, st_add(st_mult(2, args.len), 1));
 	xutftowcs(wargs, args.buf, 2 * args.len + 1);
 	strbuf_release(&args);
 

-- 
2.8.0.rc0.276.gddf4100

That compiles OK, thanks.


Sorry for high-jacking this thread, but while compiling under CYGWIN,
found one warning:

    LINK git-credential-store.exe
     CC daemon.o
daemon.c: In function ‘drop_privileges’:
daemon.c:1136:15: warning: implicit declaration of function ‘initgroups’ 
[-Wimplicit-function-declaration]
   if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
                ^

t9115 doesn't pass,  NTFS doesn't like the non-UTF filenames, it seams.
Probably the same problem as under Mac OS /HFS+-
-----------------------

And MINGW is not happy for other reasons:

builtin/rev-parse.c: In function 'cmd_rev_parse':
builtin/rev-parse.c:775:12: warning: implicit declaration of function 
'realpath' [-Wimplicit-function-declaration]
        if (!realpath(gitdir, absolute_path))
             ^
     CC builtin/revert.o


     CC builtin/write-tree.o
     LINK git.exe
builtin/rev-parse.o: In function `cmd_rev_parse':
C:\Users\tb\projects\git\tb/builtin/rev-parse.c:775: undefined reference to 
`realpath'
collect2.exe: error: ld returned 1 exit status
Makefile:1725: recipe for target 'git.exe' failed
make: *** [git.exe] Error 1

On Mon, Feb 29, 2016 at 11:40:59AM +0100, Torsten Bögershausen wrote:

That compiles OK, thanks.

Sorry for high-jacking this thread, but while compiling under CYGWIN,
found one warning:

   LINK git-credential-store.exe
    CC daemon.o
daemon.c: In function ‘drop_privileges’:
daemon.c:1136:15: warning: implicit declaration of function ‘initgroups’
[-Wimplicit-function-declaration]
  if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||

Interesting that it doesn't later complain in the link step. :)

You should probably be compiling with the NO_INITGROUPS knob on that
platform.

t9115 doesn't pass,  NTFS doesn't like the non-UTF filenames, it seams.
Probably the same problem as under Mac OS /HFS+-
-----------------------

No comment from me on that one.

And MINGW is not happy for other reasons:

builtin/rev-parse.c: In function 'cmd_rev_parse':
builtin/rev-parse.c:775:12: warning: implicit declaration of function
'realpath' [-Wimplicit-function-declaration]
       if (!realpath(gitdir, absolute_path))
            ^

I guess you're building "pu"; that is only in sg/completion-updates. I
don't know if our custom real_path() would suffice there. You might want
to ping the author. The patch is:

  http://article.gmane.org/gmane.comp.version-control.git/287462

-Peff

On 29/02/16 10:40, Torsten Bögershausen wrote:

That compiles OK, thanks.


Sorry for high-jacking this thread, but while compiling under CYGWIN,
found one warning:

   LINK git-credential-store.exe
    CC daemon.o
daemon.c: In function ‘drop_privileges’:
daemon.c:1136:15: warning: implicit declaration of function ‘initgroups’ [-Wimplicit-function-declaration]
  if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||

Yeah, this has been there for a while - except it depends on which version
of the header files you have. (Some may not see the warning).

I have 'fixed' this twice before, then updated my installation and
a change to the system headers broke it again! (The headers are
currently 'broken'). So, I got tired of fixing it up and have left
it a while - you never know a new update may fix it! ;-)

[I personally don't use the git daemon on cygwin, so I don't know
if this a problem in practice.]

ATB,
Ramsay Jones

Hi Peff,

On Mon, 29 Feb 2016, Jeff King wrote:

I think this means "master" is broken for mingw builds.

Sorry, Windows people, for breaking your build. I'm happy to hold back
such repo-wide cleanups from the mingw code in the future, since I can't
actually compile them. But the flipside is that if I _do_ improve
things, you don't get the benefit until somebody manually ports it over.

No, I do not think that you need to hold back cleanups. We will catch such
issues before long, anyway.

Thanks for all your hard work!
Dscho

Hi,

quoted

And MINGW is not happy for other reasons:

builtin/rev-parse.c: In function 'cmd_rev_parse':
builtin/rev-parse.c:775:12: warning: implicit declaration of function
'realpath' [-Wimplicit-function-declaration]
       if (!realpath(gitdir, absolute_path))
            ^

I guess you're building "pu"; that is only in sg/completion-updates. I
don't know if our custom real_path() would suffice there. You might want
to ping the author. The patch is:

  http://article.gmane.org/gmane.comp.version-control.git/287462

Oh, I was not aware that there is a custom real_path() that is
preferred over the system realpath().  I don't see why our real_path()
would not suffice, it even makes the code a tad shorter.

I will include the patch below in the reroll.

Best,
Gábor


 ----  >8  ----
Subject: [PATCH] fixup! rev-parse: add '--absolute-git-dir' option

---
 builtin/rev-parse.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 90a4dd6032c0..d6d9a82c77c4 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c

@@ -762,10 +762,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					if (!gitdir && !prefix)
 						gitdir = ".git";
 					if (gitdir) {
-						char absolute_path[PATH_MAX];
-						if (!realpath(gitdir, absolute_path))
-							die_errno(_("unable to get absolute path"));
-						puts(absolute_path);
+						puts(real_path(gitdir));
 						continue;
 					}
 				}

-- 
2.7.2.410.g92cb358

On 29/02/16 12:32, Ramsay Jones wrote:

On 29/02/16 10:40, Torsten Bögershausen wrote:

quoted

That compiles OK, thanks.


Sorry for high-jacking this thread, but while compiling under CYGWIN,
found one warning:

   LINK git-credential-store.exe
    CC daemon.o
daemon.c: In function ‘drop_privileges’:
daemon.c:1136:15: warning: implicit declaration of function ‘initgroups’ [-Wimplicit-function-declaration]
  if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||

Yeah, this has been there for a while - except it depends on which version
of the header files you have. (Some may not see the warning).

I have 'fixed' this twice before, then updated my installation and
a change to the system headers broke it again! (The headers are
currently 'broken'). So, I got tired of fixing it up and have left
it a while - you never know a new update may fix it! ;-)

[I personally don't use the git daemon on cygwin, so I don't know
if this a problem in practice.]

BTW, I forgot to mention that I have had a patch in my local repo for
ages which addresses this issue. However, although this patch fixes
the problem for me, with the header files I currently have installed, it
would just as easily _introduce_ this warning for those that don't
currently see it! ;-)

I suppose that I could send a patch which sets NO_INITGROUPS in the
Makefile, until the system header files are fixed. What do you think?

The correct solution is to fix the <grp.h> header file. I have been
a bit reluctant to tackle that, because I'm not familiar with the
cygwin project development process. Since Newlib is an upstream
project to cygwin, should I go there first/instead?

Anyway, I had a quick squint at the header and I think it needs to
be changed something like the diff below. (I've also attached the
new grp.h file).

[Note: I didn't know what to do about _PATH_GROUP, setgrfile(),
group_from_gid() and setgroupent(), so I punted on those!]

Now, If only we knew someone who could try introducing such a fix
to the cygwin project ...

ATB,
Ramsay Jones
-- >8 --

diff --git a/usr/include/grp.h b/grp.h
index c3a5a67..f2f1902 100644
--- a/usr/include/grp.h
+++ b/grp.h

@@ -67,25 +67,27 @@ extern "C" {
 #ifndef __INSIDE_CYGWIN__
 struct group	*getgrgid (gid_t);
 struct group	*getgrnam (const char *);
+#if defined __POSIX_VISIBLE || defined __BSD_VISIBLE
 int		 getgrnam_r (const char *, struct group *,
 			char *, size_t, struct group **);
 int		 getgrgid_r (gid_t, struct group *,
 			char *, size_t, struct group **);
-#ifndef _POSIX_SOURCE
-struct group	*getgrent (void);
+#endif
+#if defined __BSD_VISIBLE || defined _XOPEN_SOURCE_EXTENDED
 void		 setgrent (void);
+#if __XSI_VISIBLE >= 700
+struct group	*getgrent (void);
 void		 endgrent (void);
+#endif
+#endif
+#if defined __BSD_VISIBLE
+int		 initgroups (const char *, gid_t);
+#endif
 #ifndef __CYGWIN__
-void		 setgrfile (const char *);
-#endif /* !__CYGWIN__ */
-#ifndef _XOPEN_SOURCE
-#ifndef __CYGWIN__
+void		 setgrfile (const char *); /* 4.3BSD-Reno & Minix */
 char		*group_from_gid (gid_t, int);
-int		 setgroupent (int);
+int		 setgroupent (int); /* 4.3BSD-Reno & Mac OS X */
 #endif /* !__CYGWIN__ */
-int		 initgroups (const char *, gid_t);
-#endif /* !_XOPEN_SOURCE */
-#endif /* !_POSIX_SOURCE */
 #endif /* !__INSIDE_CYGWIN__ */
 
 #ifdef __cplusplus

On 03/03/16 03:33, Ramsay Jones wrote:

On 29/02/16 12:32, Ramsay Jones wrote:

quoted

On 29/02/16 10:40, Torsten Bögershausen wrote:

quoted

That compiles OK, thanks.


Sorry for high-jacking this thread, but while compiling under CYGWIN,
found one warning:

   LINK git-credential-store.exe
    CC daemon.o
daemon.c: In function ‘drop_privileges’:
daemon.c:1136:15: warning: implicit declaration of function ‘initgroups’ [-Wimplicit-function-declaration]
  if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||

Yeah, this has been there for a while - except it depends on which version
of the header files you have. (Some may not see the warning).

I have 'fixed' this twice before, then updated my installation and
a change to the system headers broke it again! (The headers are
currently 'broken'). So, I got tired of fixing it up and have left
it a while - you never know a new update may fix it! ;-)

[I personally don't use the git daemon on cygwin, so I don't know
if this a problem in practice.]

BTW, I forgot to mention that I have had a patch in my local repo for
ages which addresses this issue. However, although this patch fixes
the problem for me, with the header files I currently have installed, it
would just as easily _introduce_ this warning for those that don't
currently see it! ;-)

I suppose that I could send a patch which sets NO_INITGROUPS in the
Makefile, until the system header files are fixed. What do you think?

The correct solution is to fix the <grp.h> header file. I have been
a bit reluctant to tackle that, because I'm not familiar with the
cygwin project development process. Since Newlib is an upstream
project to cygwin, should I go there first/instead?

Anyway, I had a quick squint at the header and I think it needs to
be changed something like the diff below. (I've also attached the
new grp.h file).

And, of course, I made a mess of it! It was only supposed to be a
'something like this' patch, but still ... :-D

[Note: I didn't know what to do about _PATH_GROUP, setgrfile(),
group_from_gid() and setgroupent(), so I punted on those!]

Also, the comments on setgrfile() and setgroupent() were just notes
to myself, which I didn't intend to send ...

So, more like the diff below.

ATB,
Ramsay Jones

-- >8 --

diff --git a/usr/include/grp.h b/grp.h
index c3a5a67..c04fd63 100644
--- a/usr/include/grp.h
+++ b/grp.h

@@ -67,25 +67,25 @@ extern "C" {
 #ifndef __INSIDE_CYGWIN__
 struct group	*getgrgid (gid_t);
 struct group	*getgrnam (const char *);
+#if defined __POSIX_VISIBLE || defined __BSD_VISIBLE
 int		 getgrnam_r (const char *, struct group *,
 			char *, size_t, struct group **);
 int		 getgrgid_r (gid_t, struct group *,
 			char *, size_t, struct group **);
-#ifndef _POSIX_SOURCE
-struct group	*getgrent (void);
+#endif
+#if defined __BSD_VISIBLE || defined __XSI_VISIBLE
 void		 setgrent (void);
+struct group	*getgrent (void);
 void		 endgrent (void);
+#endif
+#if defined __BSD_VISIBLE
+int		 initgroups (const char *, gid_t);
+#endif
 #ifndef __CYGWIN__
 void		 setgrfile (const char *);
-#endif /* !__CYGWIN__ */
-#ifndef _XOPEN_SOURCE
-#ifndef __CYGWIN__
 char		*group_from_gid (gid_t, int);
 int		 setgroupent (int);
 #endif /* !__CYGWIN__ */
-int		 initgroups (const char *, gid_t);
-#endif /* !_XOPEN_SOURCE */
-#endif /* !_POSIX_SOURCE */
 #endif /* !__INSIDE_CYGWIN__ */
 
 #ifdef __cplusplus

Eric Sunshine [off-list ref] writes:

  t1500: test_rev_parse: facilitate future test enhancements
  t1500: reduce dependence upon global state
  t1500: avoid changing working directory outside of tests
  t1500: avoid setting configuration options outside of tests
  t1500: avoid setting environment variables outside of tests
  t1500: be considerate to future potential tests

When you reroll sg/completion-updates series (87a213f^..2be685a, 21
patches), please pay attention to this series, as it changes the way
you would check the output from your "--absolute-git-dir" option.