50a6c8ef - xmalloc_array

50a6c8ef - xmalloc_array

[Torsten Bögershausen]

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

[PATCH] compat/mingw: brown paper bag fix for 50a6c8e

[Jeff King]

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 <peff@peff.net>
---
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

Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e

[Torsten Bögershausen]

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

Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e

[Jeff King]

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

> 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);

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 <peff@peff.net>
---
 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

Compiler warning under cygwin/mingw (was: fix for 50a6c8e)

[Torsten Bögershausen]

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

Re: Compiler warning under cygwin/mingw (was: fix for 50a6c8e)

[Jeff King]

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

Re: Compiler warning under cygwin/mingw

[Ramsay Jones]

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

Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e

[Johannes Schindelin]

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

Re: Compiler warning under cygwin/mingw (was: fix for 50a6c8e)

[SZEDER Gábor]

Hi,

> > 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

Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Feb 29, 2016 at 10:56:22AM +0100, Torsten Bögershausen wrote:
>
>> 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);
>
> 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.

Just to make sure that I am not confused, what you wrote below
matches what I received from you two message upthread.

I am assuming that it is intended that the two messages from you
have the same patch, and the assignment of ALLOC_ARRAY to wargs was
a bug Torsten introduced only to his tree when cutting and pasting.

With that assumption, will queue this one (or the original one,
which to me is the same thing).

Thanks.

> -- >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 <peff@peff.net>
> ---
>  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);

Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e

[Jeff King]

On Mon, Feb 29, 2016 at 11:10:09AM -0800, Junio C Hamano wrote:

> >> -       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);
> >
> > 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.
> 
> Just to make sure that I am not confused, what you wrote below
> matches what I received from you two message upthread.
> 
> I am assuming that it is intended that the two messages from you
> have the same patch, and the assignment of ALLOC_ARRAY to wargs was
> a bug Torsten introduced only to his tree when cutting and pasting.
> 
> With that assumption, will queue this one (or the original one,
> which to me is the same thing).

Oh, <phew>. I am only half as incompetent as I thought, then.

I didn't even re-check what I sent, and just assumed Torsten was quoting
it exactly (I _did_ write something like that while making the fix, and
assumed I had just failed to properly commit).

So yes, what I sent both times is the right thing. ;)

-Peff

Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e

[Torsten Bögershausen]

Oh, <phew>. I am only half as incompetent as I thought, then.

I didn't even re-check what I sent, and just assumed Torsten was quoting
it exactly (I _did_ write something like that while making the fix, and
assumed I had just failed to properly commit).

So yes, what I sent both times is the right thing. ;)

-Peff
--

I have no clue how my copy-paste programing could go so bad :-(
Sorry for the confusion, compat/mingw.c compiles now.

----------------

However, suspecting jk/epipe-in-async, I don't know if we can do
something against this warning:

  CC run-command.o
run-command.c: In function 'async_exit':
run-command.c:631:1: warning: 'noreturn' function does return
  }
  ^

Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e

[Jeff King]

On Tue, Mar 01, 2016 at 06:49:43AM +0100, Torsten Bögershausen wrote:

> However, suspecting jk/epipe-in-async, I don't know if we can do
> something against this warning:
> 
>  CC run-command.o
> run-command.c: In function 'async_exit':
> run-command.c:631:1: warning: 'noreturn' function does return
>  }
>  ^

The only thing that function does is call pthread_exit(), which should
also be marked NORETURN. Looks like the one in compat/win32/pthread.h
isn't?

-Peff

Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

Hi Peff,

On Tue, 1 Mar 2016, Jeff King wrote:

> On Tue, Mar 01, 2016 at 06:49:43AM +0100, Torsten Bögershausen wrote:
> 
> > However, suspecting jk/epipe-in-async, I don't know if we can do
> > something against this warning:
> > 
> >  CC run-command.o run-command.c: In function 'async_exit':
> >  run-command.c:631:1: warning: 'noreturn' function does return } ^
> 
> The only thing that function does is call pthread_exit(), which should
> also be marked NORETURN. Looks like the one in compat/win32/pthread.h
> isn't?

Correct. Expect a patch momentarily.

Ciao,
Dscho

Re: Compiler warning under cygwin/mingw

[Ramsay Jones]

[-- Attachment #1: Type: text/plain, Size: 3377 bytes --]



On 29/02/16 12:32, Ramsay Jones wrote:
> 
> 
> 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.]

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

[-- Attachment #2: grp.h --]
[-- Type: text/x-chdr, Size: 3511 bytes --]

/*	$NetBSD: grp.h,v 1.7 1995/04/29 05:30:40 cgd Exp $	*/

/*-
 * Copyright (c) 1989, 1993
 *	The Regents of the University of California.  All rights reserved.
 * (c) UNIX System Laboratories, Inc.
 * All or some portions of this file are derived from material licensed
 * to the University of California by American Telephone and Telegraph
 * Co. or Unix System Laboratories, Inc. and are reproduced herein with
 * the permission of UNIX System Laboratories, Inc.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 * 1. Redistributions of source code must retain the above copyright
 *    notice, this list of conditions and the following disclaimer.
 * 2. Redistributions in binary form must reproduce the above copyright
 *    notice, this list of conditions and the following disclaimer in the
 *    documentation and/or other materials provided with the distribution.
 * 3. All advertising materials mentioning features or use of this software
 *    must display the following acknowledgement:
 *	This product includes software developed by the University of
 *	California, Berkeley and its contributors.
 * 4. Neither the name of the University nor the names of its contributors
 *    may be used to endorse or promote products derived from this software
 *    without specific prior written permission.
 *
 * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
 * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
 * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
 * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
 * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
 * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
 * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
 * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
 * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
 * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 * SUCH DAMAGE.
 *
 *	@(#)grp.h	8.2 (Berkeley) 1/21/94
 */

#ifndef _GRP_H_
#define	_GRP_H_

#include <sys/cdefs.h>
#include <sys/types.h>
#ifdef __CYGWIN__
#include <cygwin/grp.h>
#endif

#if !defined(_POSIX_SOURCE) && !defined(_XOPEN_SOURCE)
#define	_PATH_GROUP		"/etc/group"
#endif

struct group {
	char	*gr_name;		/* group name */
	char	*gr_passwd;		/* group password */
	gid_t	gr_gid;			/* group id */
	char	**gr_mem;		/* group members */
};

#ifdef __cplusplus
extern "C" {
#endif

#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 **);
#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 *); /* 4.3BSD-Reno & Minix */
char		*group_from_gid (gid_t, int);
int		 setgroupent (int); /* 4.3BSD-Reno & Mac OS X */
#endif /* !__CYGWIN__ */
#endif /* !__INSIDE_CYGWIN__ */

#ifdef __cplusplus
}
#endif

#endif /* !_GRP_H_ */

Re: Compiler warning under cygwin/mingw

[Ramsay Jones]

[-- Attachment #1: Type: text/plain, Size: 3496 bytes --]



On 03/03/16 03:33, Ramsay Jones wrote:
> 
> 
> On 29/02/16 12:32, Ramsay Jones wrote:
>>
>>
>> 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.]
> 
> 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

[-- Attachment #2: grp.h --]
[-- Type: text/x-chdr, Size: 3415 bytes --]

/*	$NetBSD: grp.h,v 1.7 1995/04/29 05:30:40 cgd Exp $	*/

/*-
 * Copyright (c) 1989, 1993
 *	The Regents of the University of California.  All rights reserved.
 * (c) UNIX System Laboratories, Inc.
 * All or some portions of this file are derived from material licensed
 * to the University of California by American Telephone and Telegraph
 * Co. or Unix System Laboratories, Inc. and are reproduced herein with
 * the permission of UNIX System Laboratories, Inc.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 * 1. Redistributions of source code must retain the above copyright
 *    notice, this list of conditions and the following disclaimer.
 * 2. Redistributions in binary form must reproduce the above copyright
 *    notice, this list of conditions and the following disclaimer in the
 *    documentation and/or other materials provided with the distribution.
 * 3. All advertising materials mentioning features or use of this software
 *    must display the following acknowledgement:
 *	This product includes software developed by the University of
 *	California, Berkeley and its contributors.
 * 4. Neither the name of the University nor the names of its contributors
 *    may be used to endorse or promote products derived from this software
 *    without specific prior written permission.
 *
 * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
 * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
 * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
 * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
 * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
 * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
 * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
 * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
 * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
 * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 * SUCH DAMAGE.
 *
 *	@(#)grp.h	8.2 (Berkeley) 1/21/94
 */

#ifndef _GRP_H_
#define	_GRP_H_

#include <sys/cdefs.h>
#include <sys/types.h>
#ifdef __CYGWIN__
#include <cygwin/grp.h>
#endif

#if !defined(_POSIX_SOURCE) && !defined(_XOPEN_SOURCE)
#define	_PATH_GROUP		"/etc/group"
#endif

struct group {
	char	*gr_name;		/* group name */
	char	*gr_passwd;		/* group password */
	gid_t	gr_gid;			/* group id */
	char	**gr_mem;		/* group members */
};

#ifdef __cplusplus
extern "C" {
#endif

#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 **);
#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 *);
char		*group_from_gid (gid_t, int);
int		 setgroupent (int);
#endif /* !__CYGWIN__ */
#endif /* !__INSIDE_CYGWIN__ */

#ifdef __cplusplus
}
#endif

#endif /* !_GRP_H_ */

Re: Compiler warning under cygwin/mingw

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> Hi,
>
>> > 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.

A friendly ping to see if I am missing new development that followed
this message...

>
> 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;
>  					}
>  				}

[PATCH 0/6] modernize t1500

[Eric Sunshine]

This series modernizes t1500; it takes an entirely different approach
than [1][2] and is intended to replace that series. Whereas [1][2]
dropped the systematic function-driven testing of git-rev-parse in favor
of dozens of nearly identical copy/paste tests, this series retains the
structure of the existing script and instead updates the tests to set up
their own needed state rather than relying upon transient and fragile
manipulation of global state.

Due to its systematic function-driven approach, the original script is
small at 87 lines, and easily understood. When [1] dropped the
systematic approach and replaced it with individual copy/paste tests,
the script ballooned to a whopping 573 lines; its v2 successor, while
smaller, still inflated it to 322 lines. Writing that amount of code
correctly (even when primarily copy/paste) is difficult; reviewing it
for correctness is tedious, mind-numbing, and error-prone, as evidenced
by [3].

This series, on the other hand, values the concision of the original and
only makes changes necessary to modernize it; the script size remains
about the same; it drops from 87 to 83 lines.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/291087/focus=291088
[2]: http://thread.gmane.org/gmane.comp.version-control.git/291729/focus=291731
[3]: http://thread.gmane.org/gmane.comp.version-control.git/291729/focus=291745

Eric Sunshine (6):
  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

 t/t1500-rev-parse.sh | 116 +++++++++++++++++++++++++--------------------------
 1 file changed, 56 insertions(+), 60 deletions(-)

-- 
2.8.2.530.g51d527d

[PATCH 1/6] t1500: test_rev_parse: facilitate future test enhancements

[Eric Sunshine]

Tests run by test_rev_parse() are nearly identical; each invokes
git-rev-parse with a single option and compares the result against an
expected value. Such duplication makes it onerous to extend the tests
since any change needs to be repeated in each test. Reduce the
duplication by parameterizing the test and driving it via a for-loop.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t1500-rev-parse.sh | 44 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..03f22fe 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,38 +3,28 @@
 test_description='test git rev-parse'
 . ./test-lib.sh
 
-test_rev_parse() {
+# usage: label is-bare is-inside-git is-inside-work prefix git-dir
+test_rev_parse () {
 	name=$1
 	shift
 
-	test_expect_success "$name: is-bare-repository" \
-	"test '$1' = \"\$(git rev-parse --is-bare-repository)\""
-	shift
-	[ $# -eq 0 ] && return
-
-	test_expect_success "$name: is-inside-git-dir" \
-	"test '$1' = \"\$(git rev-parse --is-inside-git-dir)\""
-	shift
-	[ $# -eq 0 ] && return
-
-	test_expect_success "$name: is-inside-work-tree" \
-	"test '$1' = \"\$(git rev-parse --is-inside-work-tree)\""
-	shift
-	[ $# -eq 0 ] && return
-
-	test_expect_success "$name: prefix" \
-	"test '$1' = \"\$(git rev-parse --show-prefix)\""
-	shift
-	[ $# -eq 0 ] && return
-
-	test_expect_success "$name: git-dir" \
-	"test '$1' = \"\$(git rev-parse --git-dir)\""
-	shift
-	[ $# -eq 0 ] && return
+	for o in is-bare-repository \
+		 is-inside-git-dir \
+		 is-inside-work-tree \
+		 show-prefix \
+		 git-dir
+	do
+		expect="$1"
+		test_expect_success "$name: $o" '
+			echo "$expect" >expect &&
+			git rev-parse --$o >actual &&
+			test_cmp expect actual
+		'
+		shift
+		test $# -eq 0 && break
+	done
 }
 
-# label is-bare is-inside-git is-inside-work prefix git-dir
-
 ROOT=$(pwd)
 
 test_rev_parse toplevel false false true '' .git
-- 
2.8.2.530.g51d527d

[PATCH 2/6] t1500: reduce dependence upon global state

[Eric Sunshine]

This script pollutes its global state by changing its working directory
and capturing that state via $(pwd) in the GIT_CONFIG environment
variable. This makes it difficult to modernize the script since tests
ideally should be self-contained and not rely upon such transient global
state.

With the ultimate goal of eliminating working directory changes, as a
first step, avoid capturing global state by instead taking advantage of
$ROOT which captured $(pwd) prior to any working directory changes, and
compose GIT_CONFIG from it and local knowledge of the working directory.

Subsequent patches will eliminate changes of working directory and drop
GIT_CONFIG altogether.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t1500-rev-parse.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 03f22fe..3d79568 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -49,7 +49,7 @@ test_rev_parse 'core.bare undefined' false false true
 mkdir work || exit 1
 cd work || exit 1
 GIT_DIR=../.git
-GIT_CONFIG="$(pwd)"/../.git/config
+GIT_CONFIG="$ROOT/work/../.git/config"
 export GIT_DIR GIT_CONFIG
 
 git config core.bare false
@@ -63,7 +63,7 @@ test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
 
 mv ../.git ../repo.git || exit 1
 GIT_DIR=../repo.git
-GIT_CONFIG="$(pwd)"/../repo.git/config
+GIT_CONFIG="$ROOT/work/../repo.git/config"
 
 git config core.bare false
 test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''
-- 
2.8.2.530.g51d527d

[PATCH 3/6] t1500: avoid changing working directory outside of tests

[Eric Sunshine]

Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-C <dir>" option to allow callers to
instruct it explicitly in which directory its tests should be run, and
take advantage of this new option to avoid changing the working
directory outside of tests.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t1500-rev-parse.sh | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 3d79568..f294ecc 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,8 +3,18 @@
 test_description='test git rev-parse'
 . ./test-lib.sh
 
-# usage: label is-bare is-inside-git is-inside-work prefix git-dir
+# usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir
 test_rev_parse () {
+	dir=
+	while :
+	do
+		case "$1" in
+		-C) dir="-C $2"; shift; shift ;;
+		-*) error "test_rev_parse: unrecognized option '$1'" ;;
+		*) break ;;
+		esac
+	done
+
 	name=$1
 	shift
 
@@ -17,7 +27,7 @@ test_rev_parse () {
 		expect="$1"
 		test_expect_success "$name: $o" '
 			echo "$expect" >expect &&
-			git rev-parse --$o >actual &&
+			git $dir rev-parse --$o >actual &&
 			test_cmp expect actual
 		'
 		shift
@@ -29,16 +39,11 @@ ROOT=$(pwd)
 
 test_rev_parse toplevel false false true '' .git
 
-cd .git || exit 1
-test_rev_parse .git/ false true false '' .
-cd objects || exit 1
-test_rev_parse .git/objects/ false true false '' "$ROOT/.git"
-cd ../.. || exit 1
+test_rev_parse -C .git .git/ false true false '' .
+test_rev_parse -C .git/objects .git/objects/ false true false '' "$ROOT/.git"
 
-mkdir -p sub/dir || exit 1
-cd sub/dir || exit 1
-test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git"
-cd ../.. || exit 1
+test_expect_success 'setup untracked sub/dir' 'mkdir -p sub/dir'
+test_rev_parse -C sub/dir subdirectory false false true sub/dir/ "$ROOT/.git"
 
 git config core.bare true
 test_rev_parse 'core.bare = true' true false false
@@ -46,32 +51,31 @@ test_rev_parse 'core.bare = true' true false false
 git config --unset core.bare
 test_rev_parse 'core.bare undefined' false false true
 
-mkdir work || exit 1
-cd work || exit 1
+test_expect_success 'setup non-local database ../.git' 'mkdir work'
 GIT_DIR=../.git
 GIT_CONFIG="$ROOT/work/../.git/config"
 export GIT_DIR GIT_CONFIG
 
 git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare = false' false false true ''
 
 git config core.bare true
-test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare = true' true false false ''
 
 git config --unset core.bare
-test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare undefined' false false true ''
 
-mv ../.git ../repo.git || exit 1
+test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'
 GIT_DIR=../repo.git
 GIT_CONFIG="$ROOT/work/../repo.git/config"
 
 git config core.bare false
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = false' false false true ''
 
 git config core.bare true
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = true' true false false ''
 
 git config --unset core.bare
-test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
 
 test_done
-- 
2.8.2.530.g51d527d

[PATCH 4/6] t1500: avoid setting configuration options outside of tests

[Eric Sunshine]

Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-b <value>" option to allow callers to set
"core.bare" explicitly or undefine it, and take advantage of this new
option to avoid setting "core.bare" outside of tests.

Under the hood, "-b <value>" invokes "test_config -C <dir>" (or
"test_unconfig -C <dir>"), which means that git-config knows explicitly
where to find its configuration file. Consequently, the global
GIT_CONFIG environment variable, which was needed by the manual
git-config invocations outside of tests, is no longer needed, and is
thus dropped.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t1500-rev-parse.sh | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index f294ecc..c058aa4 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -6,15 +6,25 @@ test_description='test git rev-parse'
 # usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir
 test_rev_parse () {
 	dir=
+	bare=
 	while :
 	do
 		case "$1" in
 		-C) dir="-C $2"; shift; shift ;;
+		-b) bare="$2"; shift; shift ;;
 		-*) error "test_rev_parse: unrecognized option '$1'" ;;
 		*) break ;;
 		esac
 	done
 
+	case "$bare" in
+	'') ;;
+	t*) bare="test_config $dir core.bare true" ;;
+	f*) bare="test_config $dir core.bare false" ;;
+	u*) bare="test_unconfig $dir core.bare" ;;
+	*) error "test_rev_parse: unrecognized core.bare value '$bare'"
+	esac
+
 	name=$1
 	shift
 
@@ -26,6 +36,7 @@ test_rev_parse () {
 	do
 		expect="$1"
 		test_expect_success "$name: $o" '
+			$bare &&
 			echo "$expect" >expect &&
 			git $dir rev-parse --$o >actual &&
 			test_cmp expect actual
@@ -45,37 +56,27 @@ test_rev_parse -C .git/objects .git/objects/ false true false '' "$ROOT/.git"
 test_expect_success 'setup untracked sub/dir' 'mkdir -p sub/dir'
 test_rev_parse -C sub/dir subdirectory false false true sub/dir/ "$ROOT/.git"
 
-git config core.bare true
-test_rev_parse 'core.bare = true' true false false
+test_rev_parse -b t 'core.bare = true' true false false
 
-git config --unset core.bare
-test_rev_parse 'core.bare undefined' false false true
+test_rev_parse -b u 'core.bare undefined' false false true
 
 test_expect_success 'setup non-local database ../.git' 'mkdir work'
 GIT_DIR=../.git
-GIT_CONFIG="$ROOT/work/../.git/config"
-export GIT_DIR GIT_CONFIG
+export GIT_DIR
 
-git config core.bare false
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse -C work -b f 'GIT_DIR=../.git, core.bare = false' false false true ''
 
-git config core.bare true
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_rev_parse -C work -b t 'GIT_DIR=../.git, core.bare = true' true false false ''
 
-git config --unset core.bare
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_rev_parse -C work -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''
 
 test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'
 GIT_DIR=../repo.git
-GIT_CONFIG="$ROOT/work/../repo.git/config"
 
-git config core.bare false
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = false' false false true ''
+test_rev_parse -C work -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''
 
-git config core.bare true
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = true' true false false ''
+test_rev_parse -C work -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''
 
-git config --unset core.bare
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_rev_parse -C work -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
 
 test_done
-- 
2.8.2.530.g51d527d

[PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Eric Sunshine]

Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-g <dir>" option to allow callers to
specify the value of the GIT_DIR environment variable explicitly, and
take advantage of this new option to avoid polluting the global scope
with GIT_DIR assignments.

Regarding the implementation: Typically, tests avoid polluting the
global state by wrapping transient environment variable assignments
within a subshell, however, this technique can not be used here since
test_config() and test_unconfig() need to know GIT_DIR, as well, but
neither function can be used within a subshell. Consequently, GIT_DIR is
cleared manually via sane_unset().

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t1500-rev-parse.sh | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index c058aa4..525e6d3 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -7,11 +7,13 @@ test_description='test git rev-parse'
 test_rev_parse () {
 	dir=
 	bare=
+	env=
 	while :
 	do
 		case "$1" in
 		-C) dir="-C $2"; shift; shift ;;
 		-b) bare="$2"; shift; shift ;;
+		-g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
 		-*) error "test_rev_parse: unrecognized option '$1'" ;;
 		*) break ;;
 		esac
@@ -36,6 +38,8 @@ test_rev_parse () {
 	do
 		expect="$1"
 		test_expect_success "$name: $o" '
+			test_when_finished "sane_unset GIT_DIR" &&
+			eval $env &&
 			$bare &&
 			echo "$expect" >expect &&
 			git $dir rev-parse --$o >actual &&
@@ -61,22 +65,19 @@ test_rev_parse -b t 'core.bare = true' true false false
 test_rev_parse -b u 'core.bare undefined' false false true
 
 test_expect_success 'setup non-local database ../.git' 'mkdir work'
-GIT_DIR=../.git
-export GIT_DIR
 
-test_rev_parse -C work -b f 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse -C work -g ../.git -b f 'GIT_DIR=../.git, core.bare = false' false false true ''
 
-test_rev_parse -C work -b t 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' true false false ''
 
-test_rev_parse -C work -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''
 
 test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'
-GIT_DIR=../repo.git
 
-test_rev_parse -C work -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''
+test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''
 
-test_rev_parse -C work -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''
+test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''
 
-test_rev_parse -C work -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
 
 test_done
-- 
2.8.2.530.g51d527d

[PATCH 6/6] t1500: be considerate to future potential tests

[Eric Sunshine]

The final batch of git-rev-parse tests work against a non-local object
database named ../repo.git rather than the typically-named ../.git. It
prepares by renaming .git/ to repo.git/ and pointing GIT_DIR at
../repo.git, but never restores the name to .git/, which can be
problematic for tests added in the future. Be more friendly by instead
making repo.git/ a copy of .git/.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t1500-rev-parse.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 525e6d3..af223ed 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -72,7 +72,7 @@ test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' true
 
 test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''
 
-test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'
+test_expect_success 'setup non-local database ../repo.git' 'cp -R .git repo.git'
 
 test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''
 
-- 
2.8.2.530.g51d527d

Re: [PATCH 0/6] modernize t1500

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> This series modernizes t1500; it takes an entirely different approach
> than [1][2] and is intended to replace that series.

Turns out that it wasn't so painful after all.

The only small niggle I have is on 6/6; my preference would be,
because once we set up .git we do not add objects and refs to it, to
make a copy a lot earlier before the tests start.

I'll let it simmer on the list overnight and take a fresh look in
the morning, but I think I like these better than what I had to
tweak yesterday.

Re: [PATCH 4/6] t1500: avoid setting configuration options outside of tests

[Eric Sunshine]

On Tue, May 10, 2016 at 1:20 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Ideally, each test should be responsible for setting up state it needs
> rather than relying upon transient global state. Toward this end, teach
> test_rev_parse() to accept a "-b <value>" option to allow callers to set
> "core.bare" explicitly or undefine it, and take advantage of this new
> option to avoid setting "core.bare" outside of tests.
> [...snip...]
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> @@ -6,15 +6,25 @@ test_description='test git rev-parse'
> +       case "$bare" in
> +       '') ;;
> +       t*) bare="test_config $dir core.bare true" ;;
> +       f*) bare="test_config $dir core.bare false" ;;
> +       u*) bare="test_unconfig $dir core.bare" ;;
> +       *) error "test_rev_parse: unrecognized core.bare value '$bare'"

Oops, this line lost its ;; at some point while refining the code.

> +       esac
> +

Re: [PATCH 4/6] t1500: avoid setting configuration options outside of tests

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
>> @@ -6,15 +6,25 @@ test_description='test git rev-parse'
>> +       case "$bare" in
>> ...
>> +       u*) bare="test_unconfig $dir core.bare" ;;
>> +       *) error "test_rev_parse: unrecognized core.bare value '$bare'"
>
> Oops, this line lost its ;; at some point while refining the code.
>
>> +       esac
>> +

Strictly speaking, you do not have to have one.  I'll squeeze it in
anyways.

Re: [PATCH 0/6] modernize t1500

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> This series modernizes t1500; it takes an entirely different approach
>> than [1][2] and is intended to replace that series.
>
> Turns out that it wasn't so painful after all.
>
> The only small niggle I have is on 6/6; my preference would be,
> because once we set up .git we do not add objects and refs to it, to
> make a copy a lot earlier before the tests start.

-- >8 --
Subject: [PATCH 7/6] t1500: finish preparation upfront

The earlier tests do not attempt to modify the contents of .git (by
creating objects or refs, for example), which means that even if
some earlier tests before "cp -R" fail, the tests in repo.git will
run in an environment that we can expect them to succeed in.

Make it clear that tests in repo.git will be independent from the
results of earlier tests done in .git by moving its initialization
"cp -R .git repo.git" much higher in the test sequence.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t1500-rev-parse.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 811c70f2..cb08677 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -51,6 +51,7 @@ test_rev_parse () {
 }
 
 ROOT=$(pwd)
+test_expect_success 'setup non-local database ../repo.git' 'cp -R .git repo.git'
 
 test_rev_parse toplevel false false true '' .git
 
@@ -72,8 +73,6 @@ test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' true
 
 test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''
 
-test_expect_success 'setup non-local database ../repo.git' 'cp -R .git repo.git'
-
 test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''
 
 test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''
-- 
2.8.2-623-gacdd3f1

Re: [PATCH 0/6] modernize t1500

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> This series modernizes t1500; it takes an entirely different approach
>>> than [1][2] and is intended to replace that series.
>>
>> Turns out that it wasn't so painful after all.
>>
>> The only small niggle I have is on 6/6; my preference would be,
>> because once we set up .git we do not add objects and refs to it, to
>> make a copy a lot earlier before the tests start.
>
> -- >8 --
> Subject: [PATCH 7/6] t1500: finish preparation upfront
>
> The earlier tests do not attempt to modify the contents of .git (by
> creating objects or refs, for example), which means that even if
> some earlier tests before "cp -R" fail, the tests in repo.git will
> run in an environment that we can expect them to succeed in.
>
> Make it clear that tests in repo.git will be independent from the
> results of earlier tests done in .git by moving its initialization
> "cp -R .git repo.git" much higher in the test sequence.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

I think the same logic applies to other preparatory things like
creation of sub/dir in the working tree etc.

Re: [PATCH 0/6] modernize t1500

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> 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.

Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Jeff King]

On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:

> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index c058aa4..525e6d3 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -7,11 +7,13 @@ test_description='test git rev-parse'
>  test_rev_parse () {
>  	dir=
>  	bare=
> +	env=
>  	while :
>  	do
>  		case "$1" in
>  		-C) dir="-C $2"; shift; shift ;;
>  		-b) bare="$2"; shift; shift ;;
> +		-g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;

This will expand $2 inside $env, which is later eval'd. So funny things
happen if there are spaces or metacharacters. It looks like you only use
it with short relative paths ("../repo.git", etc), which is OK, but this
would probably break badly if we ever used absolute paths.

I don't know if it's worth worrying about or not. The usual solution is
something like:

  env_git_dir=$2
  env='GIT_DIR=$env_git_dir; export GIT_DIR'
  ...
  eval "$env"

> @@ -36,6 +38,8 @@ test_rev_parse () {
>  	do
>  		expect="$1"
>  		test_expect_success "$name: $o" '
> +			test_when_finished "sane_unset GIT_DIR" &&
> +			eval $env &&

I was surprised not to see quoting around $env here, but it probably
doesn't matter (I think it may affect how some whitespace is treated,
but the contents of $env are pretty tame).

This will set up the sane_unset regardless of whether $env does
anything. Would it make more sense to stick the test_when_finished
inside $env? You could use regular unset then, too, since you know the
variable would be set.

-Peff

Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Eric Sunshine]

On Tue, May 10, 2016 at 2:39 PM, Jeff King <peff@peff.net> wrote:
> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:
>> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
>> @@ -7,11 +7,13 @@ test_description='test git rev-parse'
>>       while :
>>       do
>>               case "$1" in
>>               -C) dir="-C $2"; shift; shift ;;
>>               -b) bare="$2"; shift; shift ;;
>> +             -g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
>
> This will expand $2 inside $env, which is later eval'd. So funny things
> happen if there are spaces or metacharacters. It looks like you only use
> it with short relative paths ("../repo.git", etc), which is OK, but this
> would probably break badly if we ever used absolute paths.
>
> I don't know if it's worth worrying about or not. The usual solution is
> something like:
>
>   env_git_dir=$2
>   env='GIT_DIR=$env_git_dir; export GIT_DIR'
>   ...
>   eval "$env"

Makes sense; I wasn't quite happy with having $2 interpolated
unquoted. Like you, though, I don't know if it's worth worrying
about...

>> @@ -36,6 +38,8 @@ test_rev_parse () {
>>       do
>>               expect="$1"
>>               test_expect_success "$name: $o" '
>> +                     test_when_finished "sane_unset GIT_DIR" &&
>> +                     eval $env &&
>
> I was surprised not to see quoting around $env here, but it probably
> doesn't matter (I think it may affect how some whitespace is treated,
> but the contents of $env are pretty tame).

I flip-flopped on this one several times, quoting, and not quoting.
Documentation for 'eval' says:

    The args are read and concatenated together into a single
    command.

so, I ultimately left it unquoted, but don't feel strongly about it.

> This will set up the sane_unset regardless of whether $env does
> anything. Would it make more sense to stick the test_when_finished
> inside $env? You could use regular unset then, too, since you know the
> variable would be set.

I didn't worry about it too much because the end result is effectively
the same and, with all the 'case' arms being short one-liners, I think
the code is a bit easier to read as-is; bundling 'test_when_finished'
into the 'env' assignment line would probably require wrapping the
line. I do like the improved encapsulation of your suggestion but
don't otherwise feel strongly about it.

Nevertheless, I can re-roll with these changes if you feel more
strongly than I about it.

Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> I don't know if it's worth worrying about or not. The usual solution is
>> something like:
>>
>>   env_git_dir=$2
>>   env='GIT_DIR=$env_git_dir; export GIT_DIR'
>>   ...
>>   eval "$env"
>
> Makes sense; I wasn't quite happy with having $2 interpolated
> unquoted. Like you, though, I don't know if it's worth worrying
> about...

This is a good change, including the quoting of $env.

> I flip-flopped on this one several times, quoting, and not quoting.
> Documentation for 'eval' says:
>
>     The args are read and concatenated together into a single
>     command.

Which means the two eval's give different results:

    $ e='echo "a  b"'
    $ eval $e
    $ eval "$e"

>> This will set up the sane_unset regardless of whether $env does
>> anything. Would it make more sense to stick the test_when_finished
>> inside $env? You could use regular unset then, too, since you know the
>> variable would be set.
>
> I didn't worry about it too much because the end result is effectively
> the same and, with all the 'case' arms being short one-liners, I think
> the code is a bit easier to read as-is.

Yeah, this is OK.

Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Eric Sunshine]

On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, May 10, 2016 at 2:39 PM, Jeff King <peff@peff.net> wrote:
>> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:
>>>       while :
>>>       do
>>>               case "$1" in
>>>               -C) dir="-C $2"; shift; shift ;;
>>>               -b) bare="$2"; shift; shift ;;
>>> +             -g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
>>> @@ -36,6 +38,8 @@ test_rev_parse () {
>>>       do
>>>               expect="$1"
>>>               test_expect_success "$name: $o" '
>>> +                     test_when_finished "sane_unset GIT_DIR" &&
>>> +                     eval $env &&
>>
>> This will set up the sane_unset regardless of whether $env does
>> anything. Would it make more sense to stick the test_when_finished
>> inside $env? You could use regular unset then, too, since you know the
>> variable would be set.
>
> I didn't worry about it too much because the end result is effectively
> the same and, with all the 'case' arms being short one-liners, I think
> the code is a bit easier to read as-is; bundling 'test_when_finished'
> into the 'env' assignment line would probably require wrapping the
> line. I do like the improved encapsulation of your suggestion but
> don't otherwise feel strongly about it.

Actually, I think we can have improved encapsulation and maintain
readability like this:

    case "$1" in
    ...
    -g) env="$2"; shift;  shift ;;
    ...
    esac

    ...
    test_expect_success "..." '
        if test -n "$env"
        do
            test_when_finished "unset GIT_DIR"
            GIT_DIR="$env"
            export GIT_DIR
        fi
        ...
    '

Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Eric Sunshine]

On Tue, May 10, 2016 at 3:48 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Actually, I think we can have improved encapsulation and maintain
> readability like this:
>
>     case "$1" in
>     ...
>     -g) env="$2"; shift;  shift ;;
>     ...
>     esac
>
>     ...
>     test_expect_success "..." '
>         if test -n "$env"
>         do
>             test_when_finished "unset GIT_DIR"
>             GIT_DIR="$env"
>             export GIT_DIR
>         fi
>         ...
>     '

At this point, I'd also rename 'env' to 'gitdir' to be more meaningful.

Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, May 10, 2016 at 2:39 PM, Jeff King <peff@peff.net> wrote:
>>> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:
>>>>       while :
>>>>       do
>>>>               case "$1" in
>>>>               -C) dir="-C $2"; shift; shift ;;
>>>>               -b) bare="$2"; shift; shift ;;
>>>> +             -g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
>>>> @@ -36,6 +38,8 @@ test_rev_parse () {
>>>>       do
>>>>               expect="$1"
>>>>               test_expect_success "$name: $o" '
>>>> +                     test_when_finished "sane_unset GIT_DIR" &&
>>>> +                     eval $env &&
>>>
>>> This will set up the sane_unset regardless of whether $env does
>>> anything. Would it make more sense to stick the test_when_finished
>>> inside $env? You could use regular unset then, too, since you know the
>>> variable would be set.
>>
>> I didn't worry about it too much because the end result is effectively
>> the same and, with all the 'case' arms being short one-liners, I think
>> the code is a bit easier to read as-is; bundling 'test_when_finished'
>> into the 'env' assignment line would probably require wrapping the
>> line. I do like the improved encapsulation of your suggestion but
>> don't otherwise feel strongly about it.
>
> Actually, I think we can have improved encapsulation and maintain
> readability like this:
>
>     case "$1" in
>     ...
>     -g) env="$2"; shift;  shift ;;
>     ...
>     esac
>
>     ...
>     test_expect_success "..." '
>         if test -n "$env"
>         do
>             test_when_finished "unset GIT_DIR"
>             GIT_DIR="$env"
>             export GIT_DIR
>         fi
>         ...
>     '

That's even better.  Thanks.

Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Jeff King]

On Tue, May 10, 2016 at 03:59:42PM -0400, Eric Sunshine wrote:

> On Tue, May 10, 2016 at 3:48 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > Actually, I think we can have improved encapsulation and maintain
> > readability like this:
> >
> >     case "$1" in
> >     ...
> >     -g) env="$2"; shift;  shift ;;
> >     ...
> >     esac
> >
> >     ...
> >     test_expect_success "..." '
> >         if test -n "$env"
> >         do
> >             test_when_finished "unset GIT_DIR"
> >             GIT_DIR="$env"
> >             export GIT_DIR
> >         fi
> >         ...
> >     '
> 
> At this point, I'd also rename 'env' to 'gitdir' to be more meaningful.

Yeah, I like this even better. As much as I enjoy eval tricks, I think
this case is more readable with the condition written out.

-Peff

Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[SZEDER Gábor]

Quoting Eric Sunshine <sunshine@sunshineco.com>:

> On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine  
> <sunshine@sunshineco.com> wrote:
>> On Tue, May 10, 2016 at 2:39 PM, Jeff King <peff@peff.net> wrote:
>>> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:
>>>>       while :
>>>>       do
>>>>               case "$1" in
>>>>               -C) dir="-C $2"; shift; shift ;;
>>>>               -b) bare="$2"; shift; shift ;;
>>>> +             -g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
>>>> @@ -36,6 +38,8 @@ test_rev_parse () {
>>>>       do
>>>>               expect="$1"
>>>>               test_expect_success "$name: $o" '
>>>> +                     test_when_finished "sane_unset GIT_DIR" &&
>>>> +                     eval $env &&
>>>
>>> This will set up the sane_unset regardless of whether $env does
>>> anything. Would it make more sense to stick the test_when_finished
>>> inside $env? You could use regular unset then, too, since you know the
>>> variable would be set.
>>
>> I didn't worry about it too much because the end result is effectively
>> the same and, with all the 'case' arms being short one-liners, I think
>> the code is a bit easier to read as-is; bundling 'test_when_finished'
>> into the 'env' assignment line would probably require wrapping the
>> line. I do like the improved encapsulation of your suggestion but
>> don't otherwise feel strongly about it.
>
> Actually, I think we can have improved encapsulation and maintain
> readability like this:
>
>     case "$1" in
>     ...
>     -g) env="$2"; shift;  shift ;;
>     ...
>     esac
>
>     ...
>     test_expect_success "..." '
>         if test -n "$env"
>         do
>             test_when_finished "unset GIT_DIR"
>             GIT_DIR="$env"
>             export GIT_DIR
>         fi
>         ...
>     '

I wonder if is it really necessary to specify the path to the .git
directory via $GIT_DIR.  Would 'git --git-dir=/over/there' be just as
good?  If yes, then how about

   git ${gitdir:+--git-dir="$gitdir"} rev-parse ...

Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> I wonder if is it really necessary to specify the path to the .git
> directory via $GIT_DIR.  Would 'git --git-dir=/over/there' be just as
> good?

Then you are testing two different things that may go through
different codepaths.

Adding yet another test to check "git --git-dir=" in addition is
fine, but that is not a replacement.  We do want to make sure that
"GIT_DIR=there git" form keeps giving us the expected outcome.

Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Eric Sunshine]

On Tue, May 10, 2016 at 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder@ira.uka.de> writes:
>> I wonder if is it really necessary to specify the path to the .git
>> directory via $GIT_DIR.  Would 'git --git-dir=/over/there' be just as
>> good?
>
> Then you are testing two different things that may go through
> different codepaths.
>
> Adding yet another test to check "git --git-dir=" in addition is
> fine, but that is not a replacement.  We do want to make sure that
> "GIT_DIR=there git" form keeps giving us the expected outcome.

When working on this, I did test with --git-dir= in place of GIT_DIR
and some tests failed, but I didn't follow through to see what the
actual problem was, partly because the code was in flux and I may have
messed up something else, but primarily for the reason Junio gives
above: I wanted this modernization series to be faithful to the
original; additional testing can be added later.

Re: [PATCH 0/6] modernize t1500

[Eric Sunshine]

On Tue, May 10, 2016 at 2:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Junio C Hamano <gitster@pobox.com> writes:
>> Subject: [PATCH 7/6] t1500: finish preparation upfront
>>
>> The earlier tests do not attempt to modify the contents of .git (by
>> creating objects or refs, for example), which means that even if
>> some earlier tests before "cp -R" fail, the tests in repo.git will
>> run in an environment that we can expect them to succeed in.
>>
>> Make it clear that tests in repo.git will be independent from the
>> results of earlier tests done in .git by moving its initialization
>> "cp -R .git repo.git" much higher in the test sequence.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>
> I think the same logic applies to other preparatory things like
> creation of sub/dir in the working tree etc.

Hmm, so are you suggesting a single 'setup' test at the start of
script which does the 'cp -R' and creates those other directories
needed by later tests?

Re: [PATCH 0/6] modernize t1500

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, May 10, 2016 at 2:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>> Junio C Hamano <gitster@pobox.com> writes:
>>> Subject: [PATCH 7/6] t1500: finish preparation upfront
>>>
>>> The earlier tests do not attempt to modify the contents of .git (by
>>> creating objects or refs, for example), which means that even if
>>> some earlier tests before "cp -R" fail, the tests in repo.git will
>>> run in an environment that we can expect them to succeed in.
>>>
>>> Make it clear that tests in repo.git will be independent from the
>>> results of earlier tests done in .git by moving its initialization
>>> "cp -R .git repo.git" much higher in the test sequence.
>>>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> ---
>>
>> I think the same logic applies to other preparatory things like
>> creation of sub/dir in the working tree etc.
>
> Hmm, so are you suggesting a single 'setup' test at the start of
> script which does the 'cp -R' and creates those other directories
> needed by later tests?

Exactly.