[PATCH 1/2] Allow Overriding GIT_BUILD_DIR

[PATCH 1/2] Allow Overriding GIT_BUILD_DIR

[greened]

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

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

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

[greened]

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

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

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

[Thomas Rast]

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?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[David A. Greene]

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.

                           -Dave

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

[Thomas Rast]

dag@cray.com (David A. Greene) writes:

> 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

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

[David A. Greene]

Thomas Rast <trast@inf.ethz.ch> writes:

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

> 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

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

[Junio C Hamano]

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?

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?

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

[David A. Greene]

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!

                        -Dave

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

[Junio C Hamano]

"David A. Greene" <dag@cray.com> writes:

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

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

[Junio C Hamano]

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

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

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

[David A. Greene]

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

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

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

> 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

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

[Junio C Hamano]

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

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

[David A. Greene]

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

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

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

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

Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR

[Junio C Hamano]

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.

Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR

[David A. Greene]

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

Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR

[Junio C Hamano]

dag@cray.com (David A. Greene) writes:

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

Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR

[David A. Greene]

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

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