threads / patch / 29830

patch, 2 partsAllow Overriding GIT_BUILD_DIR

Subject: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR

## tl;dr

18 messages between Mar 4, 2012 and Mar 6, 2012. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

greened@obbligato.org· Mar 4, 2012, 23:23 UTC · lore
From: "David A. Greene" <greened@obbligato.org>

Let tests override GIT_BUILD_DIR so git will work if tests are not at the same directory level as standard git tests. Prior to this change, GIT_BUILD_DIR is hardwired to be exactly one directory above where the test lives. A test within contrib/, for example, can now use test-lib.sh and set an appropriate value for GIT_BUILD_DIR.

Signed-off-by: David A. Greene <greened@obbligato.org>
---
 t/test-lib.sh |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)
Show changes to t/test-lib.sh +15 −1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index a65dfc7..cb3a0a2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -55,6 +55,7 @@ unset $(perl -e '
 		.*_TEST
 		PROVE
 		VALGRIND
+                BUILD_DIR
 	));
 	my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
 	print join("\n", @vars);
@@ -901,7 +902,20 @@ then
 	# itself.
 	TEST_DIRECTORY=$(pwd)
 fi
-GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+
+if test -z "$GIT_BUILD_DIR"
+then
+	# We allow tests to override this, in case they want to run tests
+	# outside of t/.
+ 
+        # For in-tree test scripts, this is one level above the
+        # TEST_DIRECTORY (t/), but a test script that lives outside t/
+        # can set this variable to point at the right place so that it
+        # can find t/ directory that house test helpers like
+        # lib-pager*.sh and test vectors like t4013/ as well as
+        # previously built git tools.
+       GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+fi
 
 if test -n "$valgrind"
 then

-- 
1.7.9.1
greened@obbligato.org· Mar 4, 2012, 23:23 UTC · re: greened@obbligato.org · lore

[PATCH 2/2] Support Out-Of-Tree Valgrind Tests

From: "David A. Greene" <greened@obbligato.org>

Allow tests that do not live in the top-level t/ directory to run under valgrind. This requires exporting a couple more variables to indicate where the git tools were built and where the valgrind support files live.

Prior to this chage the valgrind support files were hard-coded to be in a sibling directory to where the valgrind tests are run.

Also prior to this change the base git build was hard-coded to be exactly two directories up from where the valgrind tests are run.

Signed-off-by: David A. Greene <greened@obbligato.org>
---
 t/test-lib.sh          |   22 ++++++++++++++++++++--
 t/valgrind/valgrind.sh |    4 ++--
 2 files changed, 22 insertions(+), 4 deletions(-)
Show changes to 2 files +22 −4

t/test-lib.sh, t/valgrind/valgrind.sh

diff --git a/t/test-lib.sh b/t/test-lib.sh
index cb3a0a2..0ebb3a8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -56,6 +56,7 @@ unset $(perl -e '
 		PROVE
 		VALGRIND
                 BUILD_DIR
+                VALGRIND_TOOLS
 	));
 	my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
 	print join("\n", @vars);
@@ -917,6 +918,20 @@ then
        GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 fi
 
+# GIT_VALGRIND_TOOLS is the location of tools like valgrind.sh.
+if test -z "$GIT_VALGRIND_TOOLS"
+then
+	# We allow tests to override this, in case they want to run tests
+	# outside of t/.
+ 
+        # For in-tree test scripts, this is in TEST_DIRECTORY/valgrind
+        # (t/valgrind), but a test script that lives outside t/ can
+        # set this variable to point at the right place so that it can
+        # find t/valgrind directory that house test helpers like
+        # valgrind.sh.
+       GIT_VALGRIND_TOOLS="$TEST_DIRECTORY"/valgrind
+fi
+
 if test -n "$valgrind"
 then
 	make_symlink () {
@@ -954,11 +969,11 @@ then
 		    test ! -d "$symlink_target" &&
 		    test "#!" != "$(head -c 2 < "$symlink_target")"
 		then
-			symlink_target=../valgrind.sh
+			symlink_target=${GIT_VALGRIND_TOOLS}/valgrind.sh
 		fi
 		case "$base" in
 		*.sh|*.perl)
-			symlink_target=../unprocessed-script
+			symlink_target=${GIT_VALGRIND_TOOLS}/unprocessed-script
 		esac
 		# create the link, or replace it if it is out of date
 		make_symlink "$symlink_target" "$GIT_VALGRIND/bin/$base" || exit
@@ -986,7 +1001,10 @@ then
 	IFS=$OLDIFS
 	PATH=$GIT_VALGRIND/bin:$PATH
 	GIT_EXEC_PATH=$GIT_VALGRIND/bin
+	# Make these available in valgrind.sh
+	export GIT_BUILD_DIR
 	export GIT_VALGRIND
+	export GIT_VALGRIND_TOOLS
 elif test -n "$GIT_TEST_INSTALLED" ; then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
 	error "Cannot run git from $GIT_TEST_INSTALLED."
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 582b4dc..d638d10 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -13,10 +13,10 @@ TRACK_ORIGINS=--track-origins=yes
 
 exec valgrind -q --error-exitcode=126 \
 	--leak-check=no \
-	--suppressions="$GIT_VALGRIND/default.supp" \
+	--suppressions="$GIT_VALGRIND_TOOLS/default.supp" \
 	--gen-suppressions=all \
 	$TRACK_ORIGINS \
 	--log-fd=4 \
 	--input-fd=4 \
 	$GIT_VALGRIND_OPTIONS \
-	"$GIT_VALGRIND"/../../"$base" "$@"
+	"$GIT_BUILD_DIR"/"$base" "$@"

-- 
1.7.9.1
Thomas Rast· Mar 5, 2012, 07:53 UTC · re: greened@obbligato.org · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

greened@obbligato.org writes:
Show 13 quoted lines
> +# GIT_VALGRIND_TOOLS is the location of tools like valgrind.sh.
> +if test -z "$GIT_VALGRIND_TOOLS"
> +then
> +	# We allow tests to override this, in case they want to run tests
> +	# outside of t/.
> + 
> +        # For in-tree test scripts, this is in TEST_DIRECTORY/valgrind
> +        # (t/valgrind), but a test script that lives outside t/ can
> +        # set this variable to point at the right place so that it can
> +        # find t/valgrind directory that house test helpers like
> +        # valgrind.sh.
> +       GIT_VALGRIND_TOOLS="$TEST_DIRECTORY"/valgrind
> +fi

I'm a bit curious: why isn't it enough to spell that path $GIT_BUILD_DIR/t/valgrind instead of making it fully configurable?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
David A. Greene· Mar 5, 2012, 18:11 UTC · re: Thomas Rast · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

Thomas Rast <trast@inf.ethz.ch> writes:
Show 18 quoted lines
> greened@obbligato.org writes:
>
>> +# GIT_VALGRIND_TOOLS is the location of tools like valgrind.sh.
>> +if test -z "$GIT_VALGRIND_TOOLS"
>> +then
>> +	# We allow tests to override this, in case they want to run tests
>> +	# outside of t/.
>> + 
>> +        # For in-tree test scripts, this is in TEST_DIRECTORY/valgrind
>> +        # (t/valgrind), but a test script that lives outside t/ can
>> +        # set this variable to point at the right place so that it can
>> +        # find t/valgrind directory that house test helpers like
>> +        # valgrind.sh.
>> +       GIT_VALGRIND_TOOLS="$TEST_DIRECTORY"/valgrind
>> +fi
>
> I'm a bit curious: why isn't it enough to spell that path
> $GIT_BUILD_DIR/t/valgrind instead of making it fully configurable?

For the same reason that TEST_DIRECTORY is different and unrelated from GIT_BUILD_DIR. It's my understanding that GIT_BUILD_DIR could end up being somewhere compeltely unrelated to where TOP_SRC/t/valgrind is. At least that's why I introduced a new parameter.

                           -Dave
Thomas Rast· Mar 6, 2012, 08:46 UTC · re: David A. Greene · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

dag@cray.com (David A. Greene) writes:
Show 25 quoted lines
> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> greened@obbligato.org writes:
>>
>>> +# GIT_VALGRIND_TOOLS is the location of tools like valgrind.sh.
>>> +if test -z "$GIT_VALGRIND_TOOLS"
>>> +then
>>> +	# We allow tests to override this, in case they want to run tests
>>> +	# outside of t/.
>>> + 
>>> +        # For in-tree test scripts, this is in TEST_DIRECTORY/valgrind
>>> +        # (t/valgrind), but a test script that lives outside t/ can
>>> +        # set this variable to point at the right place so that it can
>>> +        # find t/valgrind directory that house test helpers like
>>> +        # valgrind.sh.
>>> +       GIT_VALGRIND_TOOLS="$TEST_DIRECTORY"/valgrind
>>> +fi
>>
>> I'm a bit curious: why isn't it enough to spell that path
>> $GIT_BUILD_DIR/t/valgrind instead of making it fully configurable?
>
> For the same reason that TEST_DIRECTORY is different and unrelated from
> GIT_BUILD_DIR.  It's my understanding that GIT_BUILD_DIR could end up
> being somewhere compeltely unrelated to where TOP_SRC/t/valgrind is.
> At least that's why I introduced a new parameter.

I'm just worried that for such a fringe use-case, the maintainer of the out-of-tree tests will never notice that he missed to customize *this* particular parameter. So I'd rather have it spelled in terms of the existing two (?).

Don't we, right now, get stuff as follows:
  item                   path
  --------------------------------------------
  test-lib.sh            $TEST_DIRECTORY
  git                    $GIT_BUILD_DIR/bin-wrappers
  valgrind.sh            $TEST_DIRECTORY/valgrind
  git (with --valgrind)  $TEST_DIRECTORY/valgrind/bin
You are saying this must change to an entirely new path
  valgrind.sh            $GIT_VALGRIND_TOOLS
  git (with --valgrind)  $GIT_VALGRIND_TOOLS/bin
but what's wrong with simply
  valgrind.sh            $GIT_BUILD_DIR/t/valgrind
  git (with --valgrind)  $TEST_DIRECTORY/valgrind/bin

In the common case of t/, these just map to what we had before. In the out-of-tree case, we'd create valgrind/bin in the test directory for the *temporary* stuff, and still look for the wrapping valgrind.sh in the git tree.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
David A. Greene· Mar 6, 2012, 14:40 UTC · re: Thomas Rast · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

Thomas Rast <trast@inf.ethz.ch> writes:
Show 12 quoted lines
>>> I'm a bit curious: why isn't it enough to spell that path
>>> $GIT_BUILD_DIR/t/valgrind instead of making it fully configurable?
>>
>> For the same reason that TEST_DIRECTORY is different and unrelated from
>> GIT_BUILD_DIR.  It's my understanding that GIT_BUILD_DIR could end up
>> being somewhere compeltely unrelated to where TOP_SRC/t/valgrind is.
>> At least that's why I introduced a new parameter.
>
> I'm just worried that for such a fringe use-case, the maintainer of the
> out-of-tree tests will never notice that he missed to customize *this*
> particular parameter.  So I'd rather have it spelled in terms of the
> existing two (?).

I understand your concern. Perhaps it could be mitigated with some "HOWTO" comments at the top of the script. I'm nervous about basing the value on other variables because that's what limited the script to such a narrow scope in the first place.

Show 5 quoted lines
> Don't we, right now, get stuff as follows:
>
>   item                   path
>   --------------------------------------------
>   test-lib.sh            $TEST_DIRECTORY

Right now, yes, but it breaks for out-of-tree tests. In the out-of-tree case, TEST_DIRECTORY doesn't contain test-lib.sh. For exmaple, in t7900-subtree.sh, I do this:

. ../../../t/test-lib.sh
because TEST_DIRECTORY is set to some directory under contrib/subtree.
>   git                    $GIT_BUILD_DIR/bin-wrappers
I think so.
>   valgrind.sh            $TEST_DIRECTORY/valgrind
That's what it is now and it's wrong for out-of-tree tests.
>   git (with --valgrind)  $TEST_DIRECTORY/valgrind/bin
Yep.  This is ok.
> You are saying this must change to an entirely new path
>
>   valgrind.sh            $GIT_VALGRIND_TOOLS
>   git (with --valgrind)  $GIT_VALGRIND_TOOLS/bin
The first, yes.  The second, no.  We can leave that alone.
> but what's wrong with simply
>
>   valgrind.sh            $GIT_BUILD_DIR/t/valgrind
>   git (with --valgrind)  $TEST_DIRECTORY/valgrind/bin
These are two separate issues.

GIT_BUILD_DIR may not be anywhere within the source tree, right? If so, t/valgrind may not have any relation whatsoever to GIT_BUILD_DIR. Hence GIT_VALGRIND_TOOLS.

The second part is correct. test-lib.sh sets it up that way (~line 983) and it works fine for out-of-tree tests.

> In the common case of t/, these just map to what we had before.  In the
> out-of-tree case, we'd create valgrind/bin in the test directory for the
> *temporary* stuff, and still look for the wrapping valgrind.sh in the
> git tree.

Putting valgrind/bin in the test directory is fine. There's no change there. It's this looking for the wrapping valgrind.sh that fails in the current scheme. We cannot rely on GIT_BUILD_DIR to find it as noted above.

                              -Dave
Junio C Hamano· Mar 6, 2012, 18:49 UTC · re: David A. Greene · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

greened@obbligato.org (David A. Greene) writes:
Show 8 quoted lines
>> Don't we, right now, get stuff as follows:
>>
>>   item                   path
>>   --------------------------------------------
>>   test-lib.sh            $TEST_DIRECTORY
>
> Right now, yes, but it breaks for out-of-tree tests.  In the out-of-tree
> case, TEST_DIRECTORY doesn't contain test-lib.sh.  For exmaple, in

Could it be that the reason for the breakage is because you are setting TEST_DIRECTORY to the directory that contains out-of-tree tests, instead of $GIT_BUILD_DIR/t/ directory?

The place you perform your operations are relative to $TRASH_DIRECTORY, and you would still find the main git relative to $GIT_BUILD_DIR.

Shouldn't TEST_DIRECTORY merely a short-hand for GIT_BUILD_DIR/t? What do you find relative to $TEST_DIRECTORY that cannot be found relative to GIT_BUILD_DIR/t?

David A. Greene· Mar 6, 2012, 22:12 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> greened@obbligato.org (David A. Greene) writes:
>
>>> Don't we, right now, get stuff as follows:
>>>
>>>   item                   path
>>>   --------------------------------------------
>>>   test-lib.sh            $TEST_DIRECTORY
>>
>> Right now, yes, but it breaks for out-of-tree tests.  In the out-of-tree
>> case, TEST_DIRECTORY doesn't contain test-lib.sh.  For exmaple, in
>
> Could it be that the reason for the breakage is because you are
> setting TEST_DIRECTORY to the directory that contains out-of-tree
> tests, instead of $GIT_BUILD_DIR/t/ directory?

Well, yes. I thought that's what out-of-tree tests are supposed to do. They don't live in $GIT_BUILD_DIR/t/ after all.

Perhaps I've misunderstood how the test system is supposed to work. A table as you described in README would be most helpful. I thought TEST_DIRECTORY is supposed to point to where the tests to run are located.

> Shouldn't TEST_DIRECTORY merely a short-hand for GIT_BUILD_DIR/t?
> What do you find relative to $TEST_DIRECTORY that cannot be found
> relative to GIT_BUILD_DIR/t?

If that's what TEST_DIRECTORY is supposed to be, always, then it should be stated in the comments and README. I had no idea this was an invariant.

Thanks for clarifying!
                        -Dave
Junio C Hamano· Mar 6, 2012, 22:21 UTC · re: David A. Greene · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

"David A. Greene" <dag@cray.com> writes:
Show 34 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> greened@obbligato.org (David A. Greene) writes:
>>
>>>> Don't we, right now, get stuff as follows:
>>>>
>>>>   item                   path
>>>>   --------------------------------------------
>>>>   test-lib.sh            $TEST_DIRECTORY
>>>
>>> Right now, yes, but it breaks for out-of-tree tests.  In the out-of-tree
>>> case, TEST_DIRECTORY doesn't contain test-lib.sh.  For exmaple, in
>>
>> Could it be that the reason for the breakage is because you are
>> setting TEST_DIRECTORY to the directory that contains out-of-tree
>> tests, instead of $GIT_BUILD_DIR/t/ directory?
>
> Well, yes.  I thought that's what out-of-tree tests are supposed to do.
> They don't live in $GIT_BUILD_DIR/t/ after all.
>
> Perhaps I've misunderstood how the test system is supposed to work.  A
> table as you described in README would be most helpful.  I thought
> TEST_DIRECTORY is supposed to point to where the tests to run are
> located.
>
>> Shouldn't TEST_DIRECTORY merely a short-hand for GIT_BUILD_DIR/t?
>> What do you find relative to $TEST_DIRECTORY that cannot be found
>> relative to GIT_BUILD_DIR/t?
>
> If that's what TEST_DIRECTORY is supposed to be, always, then it should
> be stated in the comments and README.  I had no idea this was an
> invariant.
>
> Thanks for clarifying!

Not so fast. The questions in the message you are responding to were not rhetorical.

Junio C Hamano· Mar 6, 2012, 22:37 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> "David A. Greene" <dag@cray.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>> ...
>>> Could it be that the reason for the breakage is because you are
>>> setting TEST_DIRECTORY to the directory that contains out-of-tree
>>> tests, instead of $GIT_BUILD_DIR/t/ directory?
>> ...
>>> Shouldn't TEST_DIRECTORY merely a short-hand for GIT_BUILD_DIR/t?
>>> What do you find relative to $TEST_DIRECTORY that cannot be found
>>> relative to GIT_BUILD_DIR/t?
>> ...
>>
>> Thanks for clarifying!
>
> Not so fast. The questions in the message you are responding to were
> not rhetorical.
So I ended up looking at t/test-lib.sh, sigh...

We have this bit from 62f5390 (test-lib: Allow overriding of TEST_DIRECTORY, 2010-08-19):

   # Test the binaries we have just built.  The tests are kept in
   # t/ subdirectory and are run in 'trash directory' subdirectory.
   if test -z "$TEST_DIRECTORY"
   then
           # We allow tests to override this, in case they want to run tests
           # outside of t/, e.g. for running tests on the test library
           # itself.
           TEST_DIRECTORY=$(pwd)
   fi
   GIT_BUILD_DIR="$TEST_DIRECTORY"/..

that says "As a side benefit this change also makes it easy for us to move the t/*.sh tests into subdirectories if we ever want to do that."

This expects that an out-of-tree test script is expected to set TEST_DIRECTORY before dot-sourcing test-lib.sh, e.g.

	#!/bin/sh
	TEST_DIRECTORY=/srv/project/git/git.git/t
        test_description='an out-of-tree test'
        . "$TEST_DIRECTORY/test-lib.sh"

which in turn lets the test framework to learn GIT_BUILD_DIR. From there, 'git' will be found in GIT_BUILD_DIR/bin-wrappers and the valgrind variants are found in a similar way.

One thing that is potentially missing is a way for such an out-of-tree test scripts to ship with supporting material in a separate file, relative to the test script. The in-tree t/t4013-diff-various.sh has its test vectors kept in t/t4013/ directory and finds them by doing

	expect="$TEST_DIRECTORY/t4013/diff.$test"

This is because the working directory after test-lib comes back to us may not be "trash" directory under TEST_DIRECTORY, and ../t4013/ is not the right way to find it. If an out-of-tree test t9999 wants to do something similar, it needs to do something like:

	#!/bin/sh
        HERE=$(PWD)
	TEST_DIRECTORY=/srv/project/git/git.git/t
        test_description='an out-of-tree test'
        . "$TEST_DIRECTORY/test-lib.sh"
and find it relative to $HERE, e.g. "$HERE/../t9999/diff.$test"

Of course, it would be nice to use a name better than $HERE for such a purpose ;-)

David A. Greene· Mar 6, 2012, 23:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> that says "As a side benefit this change also makes it easy for us
> to move the t/*.sh tests into subdirectories if we ever want to do
> that."
>
> This expects that an out-of-tree test script is expected to set
> TEST_DIRECTORY before dot-sourcing test-lib.sh, e.g.
>
> 	#!/bin/sh
> 	TEST_DIRECTORY=/srv/project/git/git.git/t
>         test_description='an out-of-tree test'
>         . "$TEST_DIRECTORY/test-lib.sh"

Hmm...I think I missed this part when I originally tried this. I originally tried to set TEST_DIRECTORY in the environment and I misunderstood its role.

> which in turn lets the test framework to learn GIT_BUILD_DIR.  From
> there, 'git' will be found in GIT_BUILD_DIR/bin-wrappers and the
> valgrind variants are found in a similar way.

Ok, I see. So TEST_DIRECTORY is supposed to point to the "official" location of git's tests and testing support files. That wasn't clear to me.

Show 7 quoted lines
> One thing that is potentially missing is a way for such an out-of-tree
> test scripts to ship with supporting material in a separate file,
> relative to the test script.  The in-tree t/t4013-diff-various.sh
> has its test vectors kept in t/t4013/ directory and finds them by
> doing
>
> 	expect="$TEST_DIRECTORY/t4013/diff.$test"
Right.  I did not run into this issue but I can see how others might.
Show 12 quoted lines
> This is because the working directory after test-lib comes back to
> us may not be "trash" directory under TEST_DIRECTORY, and ../t4013/
> is not the right way to find it.  If an out-of-tree test t9999 wants
> to do something similar, it needs to do something like:
>
> 	#!/bin/sh
>         HERE=$(PWD)
> 	TEST_DIRECTORY=/srv/project/git/git.git/t
>         test_description='an out-of-tree test'
>         . "$TEST_DIRECTORY/test-lib.sh"
>
> and find it relative to $HERE, e.g. "$HERE/../t9999/diff.$test"
Ahh...
> Of course, it would be nice to use a name better than $HERE for such
> a purpose ;-)

I think naming is a big issue here. Perhaps TEST_DIRECTORY needs a better name, something like GIT_TEST_SUPPORT or such?

So before you apply my patches let me try to restructure the git-subtree tests with this newly provided insight and see if I can get it to work.

Thanks, Junio!
                           -Dave
Junio C Hamano· Mar 6, 2012, 23:12 UTC · re: David A. Greene · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

"David A. Greene" <dag@cray.com> writes:
> Ok, I see.  So TEST_DIRECTORY is supposed to point to the "official"
> location of git's tests and testing support files.  That wasn't clear to
> me.

That is how I read the intent of what test-lib.sh does. I do not think it has much to do with official-ness, but more about where you find pieces of the framework from (e.g. diff-lib.sh, lib-gpg.sh, etc.)

> I think naming is a big issue here.  Perhaps TEST_DIRECTORY needs a
> better name, something like GIT_TEST_SUPPORT or such?

I do not think so; the biggest problem I see is that nobody documented these variables like Thomas did in the previous message we saw in this thread (and Thomas knew more about them than all because he added t/perf/ recently and had to play with these variables).

Once the roles of variables are well understood, I do not think it is worth renaming the existing uses.

David A. Greene· Mar 6, 2012, 22:54 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

Junio C Hamano <gitster@pobox.com> writes:
Show 24 quoted lines
>>> Could it be that the reason for the breakage is because you are
>>> setting TEST_DIRECTORY to the directory that contains out-of-tree
>>> tests, instead of $GIT_BUILD_DIR/t/ directory?
>>
>> Well, yes.  I thought that's what out-of-tree tests are supposed to do.
>> They don't live in $GIT_BUILD_DIR/t/ after all.
>>
>> Perhaps I've misunderstood how the test system is supposed to work.  A
>> table as you described in README would be most helpful.  I thought
>> TEST_DIRECTORY is supposed to point to where the tests to run are
>> located.
>>
>>> Shouldn't TEST_DIRECTORY merely a short-hand for GIT_BUILD_DIR/t?
>>> What do you find relative to $TEST_DIRECTORY that cannot be found
>>> relative to GIT_BUILD_DIR/t?
>>
>> If that's what TEST_DIRECTORY is supposed to be, always, then it should
>> be stated in the comments and README.  I had no idea this was an
>> invariant.
>>
>> Thanks for clarifying!
>
> Not so fast. The questions in the message you are responding to were
> not rhetorical.

Ah, ok. I don't think I have the proper guru status to answer them. :) Regardless, it seems we need some documentation on what each of these variables is.

                           -Dave
Junio C Hamano· Mar 6, 2012, 18:43 UTC · re: Thomas Rast · lore

Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

Thomas Rast <trast@inf.ethz.ch> writes:
Show 23 quoted lines
> Don't we, right now, get stuff as follows:
>
>   item                   path
>   --------------------------------------------
>   test-lib.sh            $TEST_DIRECTORY
>   git                    $GIT_BUILD_DIR/bin-wrappers
>   valgrind.sh            $TEST_DIRECTORY/valgrind
>   git (with --valgrind)  $TEST_DIRECTORY/valgrind/bin
>
> You are saying this must change to an entirely new path
>
>   valgrind.sh            $GIT_VALGRIND_TOOLS
>   git (with --valgrind)  $GIT_VALGRIND_TOOLS/bin
>
> but what's wrong with simply
>
>   valgrind.sh            $GIT_BUILD_DIR/t/valgrind
>   git (with --valgrind)  $TEST_DIRECTORY/valgrind/bin
>
> In the common case of t/, these just map to what we had before.  In the
> out-of-tree case, we'd create valgrind/bin in the test directory for the
> *temporary* stuff, and still look for the wrapping valgrind.sh in the
> git tree.
Sounds sane.  Simple is good.

No matter what happens to this particular patch, could we have the above table (the final version of it, that is) in t/README or something, please?

Thanks.
Junio C Hamano· Mar 5, 2012, 06:33 UTC · re: greened@obbligato.org · lore

Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR

Both of your changes seem to have broken indentation to use 8-SP at the beginning of some (but not all) lines instead 1-HT. I'll queue a fixed up version and push the result out in 'pu' later, so please double check to make sure I didn't screw up.

Thanks.
David A. Greene· Mar 5, 2012, 18:10 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR

Junio C Hamano <gitster@pobox.com> writes:
> Both of your changes seem to have broken indentation to use 8-SP at
> the beginning of some (but not all) lines instead 1-HT.  I'll queue
> a fixed up version and push the result out in 'pu' later, so please
> double check to make sure I didn't screw up.

Right. This is because you flagged an indentation issue with the previous version of the patch. I think what happened is that the previous version included the 1-HT (what is HT - half-tab?) spacing but it "looked funny" with the additional "+" from the diff line.

So in some way this was deliberate to make the patch look better. I have no problem with you (or me!) changing it back.

Thanks for taking these up!
                          -Dave
Junio C Hamano· Mar 5, 2012, 20:08 UTC · re: David A. Greene · lore

Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR

dag@cray.com (David A. Greene) writes:
Show 11 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Both of your changes seem to have broken indentation to use 8-SP at
>> the beginning of some (but not all) lines instead 1-HT.  I'll queue
>> a fixed up version and push the result out in 'pu' later, so please
>> double check to make sure I didn't screw up.
>
> Right.  This is because you flagged an indentation issue with the
> previous version of the patch.  I think what happened is that the
> previous version included the 1-HT (what is HT - half-tab?) spacing but
> it "looked funny" with the additional "+" from the diff line.

No, with your earlier patch, all the existing lines used horizontal tabs for indenting, and the line you added used runs of spaces. When such a hunk is shown in diff output, "+" will make it obvious that only the new line you added is wrong (because the initial "+" and " " is absorbed in the first horizontal tab for Tab-indented lines) and that is how I noticed and pointed out "a funny indentation" to you.

David A. Greene· Mar 6, 2012, 14:21 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
>> Right.  This is because you flagged an indentation issue with the
>> previous version of the patch.  I think what happened is that the
>> previous version included the 1-HT (what is HT - half-tab?) spacing but
>> it "looked funny" with the additional "+" from the diff line.
>
> No, with your earlier patch, all the existing lines used horizontal
> tabs for indenting, and the line you added used runs of spaces.
> When such a hunk is shown in diff output, "+" will make it obvious
> that only the new line you added is wrong (because the initial "+"
> and " " is absorbed in the first horizontal tab for Tab-indented
> lines) and that is how I noticed and pointed out "a funny
> indentation" to you.

Hmm...when I went back to the file it indeed had horizontal tabs. Ah, I think I know what happened. I had to cut-n-paste into an e-mail because I couldn't get git send-email to work at the time (it apparently gives up after failing to authenticate even if the server presents more than one authentication method). So I think the mailer might have replaced tabs with spaces. I don't know. In any case, it's moot.

You indicated you'd fix up the patch. I am happy to do that as well if you want a proper re-submission. Just let me know.

                                    -Dave

← back to recent threads