"./t0001-init.sh --valgrind" is broken

"./t0001-init.sh --valgrind" is broken

[Christian Couder]

Hi,

It looks like commit 57ea7123c86771f47f34e7d92d1822d8b429897a (git.c:
make sure we do not leak GIT_* to alias scripts, Dec 20 14:50:19 2015)
broke "./t0001-init.sh --valgrind".

I get:

expecting success:
        (
                env | sed -ne "/^GIT_/s/=.*//p" &&
                echo GIT_PREFIX &&        # setup.c
                echo GIT_TEXTDOMAINDIR    # wrapper-for-bin.sh
        ) | sort | uniq >expected &&
        cat <<-\EOF >script &&
        #!/bin/sh
        env | sed -ne "/^GIT_/s/=.*//p" | sort >actual
        exit 0
        EOF
        chmod 755 script &&
        git config alias.script \!./script &&
        ( mkdir sub && cd sub && git script ) &&
        test_cmp expected actual

--- expected    2016-03-03 00:05:17.113754381 +0000
+++ actual      2016-03-03 00:05:19.041783583 +0000
@@ -10,7 +10,6 @@
 GIT_PREFIX
 GIT_TEMPLATE_DIR
 GIT_TEST_TEE_STARTED
-GIT_TEXTDOMAINDIR
 GIT_TRACE_BARE
 GIT_VALGRIND
 GIT_VALGRIND_ENABLED
not ok 6 - No extra GIT_* on alias scripts

It's late here so I don't have time to work on this tonight.

Thanks,
Christian.

Re: "./t0001-init.sh --valgrind" is broken

[Duy Nguyen]

On Thu, Mar 3, 2016 at 7:07 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> Hi,
>
> It looks like commit 57ea7123c86771f47f34e7d92d1822d8b429897a (git.c:
> make sure we do not leak GIT_* to alias scripts, Dec 20 14:50:19 2015)
> broke "./t0001-init.sh --valgrind".

Just wanted to confirm the problem. I will look at it later today.
-- 
Duy

Re: "./t0001-init.sh --valgrind" is broken

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Mar 3, 2016 at 7:07 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> Hi,
>>
>> It looks like commit 57ea7123c86771f47f34e7d92d1822d8b429897a (git.c:
>> make sure we do not leak GIT_* to alias scripts, Dec 20 14:50:19 2015)
>> broke "./t0001-init.sh --valgrind".
>
> Just wanted to confirm the problem. I will look at it later today.

Thanks.

Re: "./t0001-init.sh --valgrind" is broken

[Johannes Sixt]

Am 03.03.2016 um 02:04 schrieb Duy Nguyen:
> On Thu, Mar 3, 2016 at 7:07 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> Hi,
>>
>> It looks like commit 57ea7123c86771f47f34e7d92d1822d8b429897a (git.c:
>> make sure we do not leak GIT_* to alias scripts, Dec 20 14:50:19 2015)
>> broke "./t0001-init.sh --valgrind".
> 
> Just wanted to confirm the problem. I will look at it later today.
> 

Here's a patch.

---- 8< ----
Subject: [PATCH] t0001: fix GIT_* environment variable check under --valgrind

When a test case is run without --valgrind, the wrap-for-bin.sh
helper script inserts the environment variable GIT_TEXTDOMAINDIR, but
when run with --valgrind, the variable is missing. A recently
introduced test case expects the presence of the variable, though, and
fails under --valgrind.

Rewrite the test case to strip conditially defined environment variables
from both expected and actual output.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t0001-init.sh | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 295aa59..a5b9e7a 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -88,19 +88,17 @@ test_expect_success 'plain nested in bare through aliased command' '
 '
 
 test_expect_success 'No extra GIT_* on alias scripts' '
-	(
-		env | sed -ne "/^GIT_/s/=.*//p" &&
-		echo GIT_PREFIX &&        # setup.c
-		echo GIT_TEXTDOMAINDIR    # wrapper-for-bin.sh
-	) | sort | uniq >expected &&
-	cat <<-\EOF >script &&
-	#!/bin/sh
-	env | sed -ne "/^GIT_/s/=.*//p" | sort >actual
-	exit 0
+	write_script script <<-\EOF &&
+	env |
+		sed -n \
+			-e "/^GIT_PREFIX=/d" \
+			-e "/^GIT_TEXTDOMAINDIR=/d" \
+			-e "/^GIT_/s/=.*//p" |
+		sort
 	EOF
-	chmod 755 script &&
+	./script >expected &&
 	git config alias.script \!./script &&
-	( mkdir sub && cd sub && git script ) &&
+	( mkdir sub && cd sub && git script >../actual ) &&
 	test_cmp expected actual
 '
 
-- 
2.7.0.118.g90056ae

Re: "./t0001-init.sh --valgrind" is broken

[Duy Nguyen]

+the-other-Johannes who added valgrind support.

On Thu, Mar 3, 2016 at 1:55 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> ---- 8< ----
> Subject: [PATCH] t0001: fix GIT_* environment variable check under --valgrind
>
> When a test case is run without --valgrind, the wrap-for-bin.sh
> helper script inserts the environment variable GIT_TEXTDOMAINDIR, but
> when run with --valgrind, the variable is missing. A recently
> introduced test case expects the presence of the variable, though, and
> fails under --valgrind.

Yep.

It's interesting though that valgrind sets up some variables without
going through bin-wrappers. That's understandable because valgrind
support is added (in 4e1be63) 10 months before bin-wrappers (in
ea92519).  But it's probably better that we inject valgrind command
from inside bin-wrappers script, the same way we inject gdb, I think.

> Rewrite the test case to strip conditially defined environment variables
> from both expected and actual output.

Or we could set GIT_TEXTDOMAINDIR in the "if test -n $valgrind" code
in test-lib.sh, which makes the two more consistent. Also simpler
patch.

> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  t/t0001-init.sh | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 295aa59..a5b9e7a 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -88,19 +88,17 @@ test_expect_success 'plain nested in bare through aliased command' '
>  '
>
>  test_expect_success 'No extra GIT_* on alias scripts' '
> -       (
> -               env | sed -ne "/^GIT_/s/=.*//p" &&
> -               echo GIT_PREFIX &&        # setup.c
> -               echo GIT_TEXTDOMAINDIR    # wrapper-for-bin.sh
> -       ) | sort | uniq >expected &&
> -       cat <<-\EOF >script &&
> -       #!/bin/sh
> -       env | sed -ne "/^GIT_/s/=.*//p" | sort >actual
> -       exit 0
> +       write_script script <<-\EOF &&
> +       env |
> +               sed -n \
> +                       -e "/^GIT_PREFIX=/d" \
> +                       -e "/^GIT_TEXTDOMAINDIR=/d" \
> +                       -e "/^GIT_/s/=.*//p" |
> +               sort
>         EOF
> -       chmod 755 script &&
> +       ./script >expected &&
>         git config alias.script \!./script &&
> -       ( mkdir sub && cd sub && git script ) &&
> +       ( mkdir sub && cd sub && git script >../actual ) &&
>         test_cmp expected actual
>  '
>
> --
> 2.7.0.118.g90056ae
>



-- 
Duy

Re: "./t0001-init.sh --valgrind" is broken

[Duy Nguyen]

On Thu, Mar 3, 2016 at 7:09 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> But it's probably better that we inject valgrind command
> from inside bin-wrappers script, the same way we inject gdb, I think.

For the best of both worlds, we should recreate bin-wrappers in
test-lib.sh (i.e. the valgrind way), not in Makefile. Somewhat
unrelated, but because topdir is getting really crowded and
bin-wrappers is used for the test suite only, it should be moved
inside t/ (i'm going to move all test-* to t/ too, later).
-- 
Duy

Re: "./t0001-init.sh --valgrind" is broken

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Mar 3, 2016 at 7:09 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> But it's probably better that we inject valgrind command
>> from inside bin-wrappers script, the same way we inject gdb, I think.
>
> For the best of both worlds, we should recreate bin-wrappers in
> test-lib.sh (i.e. the valgrind way), not in Makefile.
>
> Somewhat
> unrelated, but because topdir is getting really crowded and
> bin-wrappers is used for the test suite only, it should be moved
> inside t/ (i'm going to move all test-* to t/ too, later).

Weren't there people who pointed their bin-wrappers/ with $PATH to
test/use freshly baked Git before they convince themselves that they
want to install it?  Not building it from the top-level Makefile
and moving it to elsewhere would be two breakages for them.

I am not sure if that is a good idea.

Moving test-* sources out of the top-level is a good idea, and
placing test-* binaries somewhere other than the top-level is also a
good idea.  Just like t/lib-*.sh are helpers for tests, these are
also test helpers that happen to be written in C and compiled, so I
don't have a strong objection to make t/ the new location for
them--a different location (e.g. a new "test-helpers/" directory) is
also something I can go with.

Thanks.

In any case, are these two messages objections to J6t's fix, or are
you fine with the fix for 2.8-rc1 and merely raising ideas to redo
it in a different (i.e. your) way after 2.8 final, or are you
planning to do a fix in a different way for 2.8-rc1?

Re: "./t0001-init.sh --valgrind" is broken

[Jeff King]

On Thu, Mar 03, 2016 at 07:09:12PM +0700, Duy Nguyen wrote:

> +the-other-Johannes who added valgrind support.
> 
> On Thu, Mar 3, 2016 at 1:55 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> > ---- 8< ----
> > Subject: [PATCH] t0001: fix GIT_* environment variable check under --valgrind
> >
> > When a test case is run without --valgrind, the wrap-for-bin.sh
> > helper script inserts the environment variable GIT_TEXTDOMAINDIR, but
> > when run with --valgrind, the variable is missing. A recently
> > introduced test case expects the presence of the variable, though, and
> > fails under --valgrind.
> 
> Yep.
> 
> It's interesting though that valgrind sets up some variables without
> going through bin-wrappers. That's understandable because valgrind
> support is added (in 4e1be63) 10 months before bin-wrappers (in
> ea92519).  But it's probably better that we inject valgrind command
> from inside bin-wrappers script, the same way we inject gdb, I think.

I had the same thought and even started on a patch, but it doesn't quite
work. The bin-wrappers are all about intercepting what goes into the
user's $PATH, and pointing our libexec dir at the main build.

So we have "git" and "git-upload-pack" in bin-wrappers, but not
"git-log". Whereas the valgrind code wants to intercept _all_ of the
test script's invocations of git, including ones spawned by scripts,
other git commands, etc. So conceptually, it wants to intercept
$GIT_EXEC_PATH.

-Peff

Re: "./t0001-init.sh --valgrind" is broken

[Johannes Sixt]

Am 03.03.2016 um 13:09 schrieb Duy Nguyen:
> +the-other-Johannes who added valgrind support.
>
> On Thu, Mar 3, 2016 at 1:55 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> ---- 8< ----
>> Subject: [PATCH] t0001: fix GIT_* environment variable check under --valgrind
>>
>> When a test case is run without --valgrind, the wrap-for-bin.sh
>> helper script inserts the environment variable GIT_TEXTDOMAINDIR, but
>> when run with --valgrind, the variable is missing. A recently
>> introduced test case expects the presence of the variable, though, and
>> fails under --valgrind.
>
> Yep.
>
> It's interesting though that valgrind sets up some variables without
> going through bin-wrappers. That's understandable because valgrind
> support is added (in 4e1be63) 10 months before bin-wrappers (in
> ea92519).  But it's probably better that we inject valgrind command
> from inside bin-wrappers script, the same way we inject gdb, I think.
>
>> Rewrite the test case to strip conditially defined environment variables
>> from both expected and actual output.
>
> Or we could set GIT_TEXTDOMAINDIR in the "if test -n $valgrind" code
> in test-lib.sh, which makes the two more consistent. Also simpler
> patch.

My fix (or something along its lines) is needed nevertheless. Just 
s/--valgrind/--with-dashes/g in the commit message if you want to fix 
the --valgrind case differently ;-)

I run tests on Windows --with-dashes in the hopes that it saves a fork 
and exec or two on every git invocation.

-- Hannes