Bug in test-lib.sh: test_create_repo() / RFC

Bug in test-lib.sh: test_create_repo() / RFC

[Michael J Gruber]

Hi there,

running the test suite with -v for the upcoming release exposed a
certain problem with test_create_repo() whose consequences I can't quite
fathom at the moment. That means: I don't know whether it's maint
material or forbidden fruits during rc-cycle...

Problem:
Since a6d63b7 (test-lib: avoid assuming that templates/ are in the
GIT_EXEC_PATH, 2009-02-04), test_create_repo() assumes to be called from
a directory such that `pwd`/../templates/blt/ contains templates for
git-init.

Several tests (see below) call test_create_repo() from a different
directory, which means the repo is created without any of the default
files (and that a mv .git/hooks .git/hooks-disabled later in the
function errors out). Now, for most tests this probably doesn't matter
at all but it's not nice.

RFC:
I see several possible solutions:

- Make sure all tests use test_create_repo() from t/. Cumbersome and
fragile.

- Simply use $(TEST_DIRECTORY)/../templates/blt/. Nice and easy. But
uses the templates from the git repo containing t/ even when testing
against and installed git (just like now, for most of the tests).

- Teach git a "--templates-dir" option similar to "--html-path" and use
that (from the git actually being tested). Means we use the templates
belonging to the tested git; but also means we can test only git
versions containing that new option.

What do you think?

Michael

Affected tests:
t0050-filesystem.sh
t1007-hash-object.sh
t1302-repo-version.sh
t2103-update-index-ignore-missing.sh
t4027-diff-submodule.sh
t5300-pack-object.sh
t5513-fetch-track.sh
t5600-clone-fail-cleanup.sh
t5601-clone.sh
t5700-clone-reference.sh
t5710-info-alternate.sh
t6026-merge-attr.sh
t7001-mv.sh
t7010-setup.sh
t7401-submodule-summary.sh
t7506-status-submodule.sh
t7508-status.sh

Re: Bug in test-lib.sh: test_create_repo() / RFC

[Jeff King]

On Mon, Apr 20, 2009 at 04:51:18PM +0200, Michael J Gruber wrote:

> Problem:
> Since a6d63b7 (test-lib: avoid assuming that templates/ are in the
> GIT_EXEC_PATH, 2009-02-04), test_create_repo() assumes to be called from
> a directory such that `pwd`/../templates/blt/ contains templates for
> git-init.
> 
> Several tests (see below) call test_create_repo() from a different
> directory, which means the repo is created without any of the default
> files (and that a mv .git/hooks .git/hooks-disabled later in the
> function errors out). Now, for most tests this probably doesn't matter
> at all but it's not nice.

If I am understanding it correctly, I think this is simply a bug
introduced by a6d63b7, and the fix is maint-worthy.

And I think the right fix is:

> - Simply use $(TEST_DIRECTORY)/../templates/blt/. Nice and easy. But
> uses the templates from the git repo containing t/ even when testing
> against and installed git (just like now, for most of the tests).

The original code (before a6d63b7) used $(GIT_EXEC_PATH), since it
handily pointed to "$(TEST_DIRECTORY)/.." (and I didn't check, but I
suspect that line predates $(TEST_DIRECTORY) entirely). So I think this
is just doing what the a6d63b7 should have done in the first place,
instead of using `pwd` to guess at the top-level location.

One thing to consider: we now have code to test an installed version of
git. Should it be using the vanilla hooks from the source directory, or
should it be using the regular templates? I think that question is
orthogonal to the bug you mention, though -- the current code is trying
to use the hooks from the source directory, but it is just failing to do
so; by fixing it, you certainly won't be breaking the
test-installed-version case.

-Peff

Re: Bug in test-lib.sh: test_create_repo() / RFC

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Apr 20, 2009 at 04:51:18PM +0200, Michael J Gruber wrote:
>
>> Problem:
>> Since a6d63b7 (test-lib: avoid assuming that templates/ are in the
>> GIT_EXEC_PATH, 2009-02-04), test_create_repo() assumes to be called from
>> a directory such that `pwd`/../templates/blt/ contains templates for
>> git-init.
>> 
>> Several tests (see below) call test_create_repo() from a different
>> directory, which means the repo is created without any of the default
>> files (and that a mv .git/hooks .git/hooks-disabled later in the
>> function errors out). Now, for most tests this probably doesn't matter
>> at all but it's not nice.
>
> If I am understanding it correctly, I think this is simply a bug
> introduced by a6d63b7, and the fix is maint-worthy.
>
> And I think the right fix is:
>
>> - Simply use $(TEST_DIRECTORY)/../templates/blt/. Nice and easy. But
>> uses the templates from the git repo containing t/ even when testing
>> against and installed git (just like now, for most of the tests).
>
> The original code (before a6d63b7) used $(GIT_EXEC_PATH), since it
> handily pointed to "$(TEST_DIRECTORY)/.." (and I didn't check, but I
> suspect that line predates $(TEST_DIRECTORY) entirely). So I think this
> is just doing what the a6d63b7 should have done in the first place,
> instead of using `pwd` to guess at the top-level location.
>
> One thing to consider: we now have code to test an installed version of
> git. Should it be using the vanilla hooks from the source directory, or
> should it be using the regular templates? I think that question is
> orthogonal to the bug you mention, though -- the current code is trying
> to use the hooks from the source directory, but it is just failing to do
> so; by fixing it, you certainly won't be breaking the
> test-installed-version case.

Yup, I think that is the right approach.

[PATCH-maint] test-lib.sh: Help test_create_repo() find the templates dir

[Michael J Gruber]

Currently, test_create_repo() expects that templates can be found below
$GIT_EXEC_DIR. This assumption fails when tests are run against a git
installed somewhere else.
Therefore, use $TEST_DIRECTORY as introduced in 2d84e9fb and expect
templates to be present in $TEST_DIRECTORY/.. which should be the root
dir of the git checkout.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
This is a patch against maint as discussed in
http://article.gmane.org/gmane.comp.version-control.git/11699 although
maint does not (yet) contain a6d63b7 which actually exposes the problem.

The patch does not apply cleanly to master and needs a different commit
message there anyways. Therefore, I will send a follow-up with a patch
against master. I suggest considering this "maint for master", i.e.
applying to master where the bug is actually exposed.

 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 59d82d2..dbce26a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -434,7 +434,7 @@ test_create_repo () {
 	repo="$1"
 	mkdir -p "$repo"
 	cd "$repo" || error "Cannot setup test environment"
-	"$GIT_EXEC_PATH/git" init "--template=$GIT_EXEC_PATH/templates/blt/" >&3 2>&4 ||
+	"$GIT_EXEC_PATH/git" init "--template=$TEST_DIRECTORY/../templates/blt/" >&3 2>&4 ||
 	error "cannot run git init -- have you built things yet?"
 	mv .git/hooks .git/hooks-disabled
 	cd "$owd"
-- 
1.6.3.rc0.201.g3759d

[PATCH-master] test-lib.sh: Help test_create_repo() find the templates dir

[Michael J Gruber]

Currently, test_create_repo() expects that templates can be found below
`pwd`/.. This assumption fails when tests are run against a git
installed somewhere else or test_create_repo() is called from
subdirectiories (several tests do this).
Therefore, use $TEST_DIRECTORY as introduced in 2d84e9fb and expect
templates to be present in $TEST_DIRECTORY/.. which should be the root
dir of the git checkout.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4bd986f..dad1437 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -491,7 +491,7 @@ test_create_repo () {
 	repo="$1"
 	mkdir -p "$repo"
 	cd "$repo" || error "Cannot setup test environment"
-	"$GIT_EXEC_PATH/git-init" "--template=$owd/../templates/blt/" >&3 2>&4 ||
+	"$GIT_EXEC_PATH/git-init" "--template=$TEST_DIRECTORY/../templates/blt/" >&3 2>&4 ||
 	error "cannot run git init -- have you built things yet?"
 	mv .git/hooks .git/hooks-disabled
 	cd "$owd"
-- 
1.6.3.rc0.201.g3759d

Re: [PATCH-maint] test-lib.sh: Help test_create_repo() find the templates dir

[Johannes Schindelin]

Hi,

On Tue, 21 Apr 2009, Michael J Gruber wrote:

> This is a patch against maint as discussed in 
> http://article.gmane.org/gmane.comp.version-control.git/11699 although 
> maint does not (yet) contain a6d63b7 which actually exposes the problem.

So it is a fix in need of a problem?

Ciao,
Dscho

Re: [PATCH-maint] test-lib.sh: Help test_create_repo() find the templates dir

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 21.04.2009 11:26:
> Hi,
> 
> On Tue, 21 Apr 2009, Michael J Gruber wrote:
> 
>> This is a patch against maint as discussed in 
>> http://article.gmane.org/gmane.comp.version-control.git/11699 although 
>> maint does not (yet) contain a6d63b7 which actually exposes the problem.
> 
> So it is a fix in need of a problem?

Your question is answered by the second paragraph which I wrote and you
cut... I suggested applying to master only. (Admittedly, the word "only"
does not appear there.)

Cheers,
Michael