[PATCH] use "git init-db" in tests

[PATCH] use "git init-db" in tests

[Alex Riesen]

Unless there was a special reason to use git-init-db from $PATH, could
the patch below be accepted? It makes the test suite use git-init-db
from the "the binaries we have just built".

The reason, why we need the patch even if the $PATH is set, will be
better explained by glibc's sources: the lookup does not stop if the
file exists but has no execute permission.

See file posix/execvp.c, the loop iterating over elements in $PATH:

          switch (errno)
            {
            case EACCES:
              /* Record the we got a `Permission denied' error.  If we end
                 up finding no executable we can use, we want to diagnose
                 that we did find one but were denied access.  */
              got_eacces = true;
                 ...
              break;
            ...
            }
      ...
      /* We tried every element and none of them worked.  */
      if (got_eacces)
        /* At least one failure was due to permissions, so report that
           error.  */
        __set_errno (EACCES);

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>

---

 By the same reason, the whole testsuite should be reviewed to use 
 "git <command>" notation.

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

4417503d10870c913cff4f635acf274b7da75864
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0539dac..3704e5f 100755
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -190,8 +190,8 @@ test=trash
 rm -fr "$test"
 mkdir "$test"
 cd "$test"
-git-init-db --template=../../templates/blt/ 2>/dev/null ||
-error "cannot run git-init-db"
+git init-db --template=../../templates/blt/ 2>/dev/null ||
+error "cannot run git init-db"
 
 mv .git/hooks .git/hooks-disabled
 
-- 
0.99.9.GIT

Re: [PATCH] use "git init-db" in tests

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Unless there was a special reason to use git-init-db from $PATH, could
> the patch below be accepted? It makes the test suite use git-init-db
> from the "the binaries we have just built".

A few lines above what you patched we do make sure the PATH
starts with the top of the build directory where you would find
git-init-db we have just built, so unless I am missing something...

Re: [PATCH] use "git init-db" in tests

[Alex Riesen]

Alex Riesen, Thu, Dec 08, 2005 21:25:55 +0100:
> -git-init-db --template=../../templates/blt/ 2>/dev/null ||
> -error "cannot run git-init-db"
> +git init-db --template=../../templates/blt/ 2>/dev/null ||
> +error "cannot run git init-db"

"$GIT_EXEC_PATH/git init-db", of course.

Signed-off-by: Alex Riesen <fork0@users.sf.net>

---

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

33dbb3dacf1b38c40f091dd800ea2dafdfa1fd3f
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0539dac..777c748 100755
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -190,8 +190,8 @@ test=trash
 rm -fr "$test"
 mkdir "$test"
 cd "$test"
-git-init-db --template=../../templates/blt/ 2>/dev/null ||
-error "cannot run git-init-db"
+"$GIT_EXEC_PATH/git" init-db --template=../../templates/blt/ 2>/dev/null ||
+error "cannot run $GIT_EXEC_PATH/git init-db"
 
 mv .git/hooks .git/hooks-disabled
 
-- 
0.99.9.GIT

Re: [PATCH] use "git init-db" in tests

[Alex Riesen]

Junio C Hamano, Thu, Dec 08, 2005 21:47:41 +0100:
> 
> > Unless there was a special reason to use git-init-db from $PATH, could
> > the patch below be accepted? It makes the test suite use git-init-db
> > from the "the binaries we have just built".
> 
> A few lines above what you patched we do make sure the PATH
> starts with the top of the build directory where you would find
> git-init-db we have just built, so unless I am missing something...
> 

You do miss something. glibc will happily continue lookup if
git-init-db in the top of the build directory is not executable, and
it will take the next one it finds (and people _will_ have git-init-db
in PATH).

Re: [PATCH] use "git init-db" in tests

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> You do miss something. glibc will happily continue lookup if
> git-init-db in the top of the build directory is not executable, and
> it will take the next one it finds (and people _will_ have git-init-db
> in PATH).

And the reason git-init-db we just built is not executable
is...?

Re: [PATCH] use "git init-db" in tests

[Alex Riesen]

On 12/8/05, Junio C Hamano <junkio@cox.net> wrote:
> > You do miss something. glibc will happily continue lookup if
> > git-init-db in the top of the build directory is not executable, and
> > it will take the next one it finds (and people _will_ have git-init-db
> > in PATH).
>
> And the reason git-init-db we just built is not executable
> is...?

An accident? Like a filesystem not supporting executable permission?
What is the reason to report success from the test run in that conditions?

Re: [PATCH] use "git init-db" in tests

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> An accident? Like a filesystem not supporting executable permission?
> What is the reason to report success from the test run in that conditions?

Let's be reasonable.  I was hoping to hear from you a real-world
breakage case that I overlooked due to my lack of access to
platforms you may have access to.  I am not interested in a
theoretical failure case discussion very much.  If your
filesystem does not support executables, why do you expect
things to run from the freshly built directory to begin with?

Linkage error of git-init-db (or git wrapper) may leave the file
created but leave that in unexecutable form, which could be a
valid concern, but that would signal an error to the make during
the build stage, and "test" target depends on "all" target.

And please do not start arguing that you can cd to 't' directory
after such a build failure and manually say "make".  You can do
that without even running make at the top level and cause the
same failure.  I consider both of them pilot errors.

Re: [PATCH] use "git init-db" in tests

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Linkage error of git-init-db (or git wrapper) may leave the file
> created but leave that in unexecutable form, which could be a
> valid concern, but that would signal an error to the make during
> the build stage, and "test" target depends on "all" target.

BTW, I sometimes wished if it were easier to disable that "test:
all" dependency and run tests without building things first,
i.e. deliberately using the already installed binaries, so that
I can make sure that updated or new tests reproduce and catch
problems with the existing code first and then make sure the
binaries built from the updated sources fix the problem.

Of course that is a very specialized application so a makefile
variable "make NO_BUILD_BEFORE_TEST=YesPlease test" would not
make much sense, but I could have done something like this:

---
diff --git a/Makefile b/Makefile
index 01b6643..8dd7be7 100644
--- a/Makefile
+++ b/Makefile
@@ -439,7 +439,13 @@ doc:
 
 ### Testing rules
 
-test: all
+ifdef NO_BUILD_BEFORE_TEST
+test_depends = 
+else
+test_depends = all
+endif
+
+test: $(test_depends)
 	$(MAKE) -C t/ all
 
 test-date$X: test-date.c date.o ctype.o

Re: [PATCH] use "git init-db" in tests

[Petr Baudis]

Dear diary, on Fri, Dec 09, 2005 at 09:52:27AM CET, I got a letter
where Junio C Hamano <junkio@cox.net> said that...
> Junio C Hamano <junkio@cox.net> writes:
> 
> > Linkage error of git-init-db (or git wrapper) may leave the file
> > created but leave that in unexecutable form, which could be a
> > valid concern, but that would signal an error to the make during
> > the build stage, and "test" target depends on "all" target.
> 
> BTW, I sometimes wished if it were easier to disable that "test:
> all" dependency and run tests without building things first,
> i.e. deliberately using the already installed binaries, so that
> I can make sure that updated or new tests reproduce and catch
> problems with the existing code first and then make sure the
> binaries built from the updated sources fix the problem.
> 
> Of course that is a very specialized application so a makefile
> variable "make NO_BUILD_BEFORE_TEST=YesPlease test" would not
> make much sense, but I could have done something like this:

I think having "test-standalone" or "test-nobuild" or whatever rule and

	test: all test-nobuild

	test-nobuild:
		...

would be much more elegant.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: [PATCH] use "git init-db" in tests

[Alex Riesen]

On 12/9/05, Junio C Hamano <junkio@cox.net> wrote:
> > An accident? Like a filesystem not supporting executable permission?
> > What is the reason to report success from the test run in that conditions?
>
> Let's be reasonable.  I was hoping to hear from you a real-world
> breakage case that I overlooked due to my lack of access to
> platforms you may have access to. I am not interested in a
> theoretical failure case discussion very much.  If your
> filesystem does not support executables, why do you expect
> things to run from the freshly built directory to begin with?

In my case it was the freshly build directory where a chmod 0666 * was
done. This directory wont rebuild (the dates are correct), and the
tests run, as if nothing happened.
I actually noticed only after a half an hour, that I wasn't running
the executables I expected.

> Linkage error of git-init-db (or git wrapper) may leave the file
> created but leave that in unexecutable form, which could be a
> valid concern, but that would signal an error to the make during
> the build stage, and "test" target depends on "all" target.
>
> And please do not start arguing that you can cd to 't' directory
> after such a build failure and manually say "make".  You can do
> that without even running make at the top level and cause the
> same failure.  I consider both of them pilot errors.

Yes, but they are not obvious. It is also not obvious what to do,
because everything seams alright.

Re: [PATCH] use "git init-db" in tests

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> In my case it was the freshly build directory where a chmod 0666 * was
> done. This directory wont rebuild (the dates are correct), and the
> tests run, as if nothing happened.

I see.  I appreciate your honesty ;-), it is a pilot error
alright, but surely it would be nicer if we catch it.

Although your proposed change makes it harder to implement
my desire to be able to run tests with installed binaries than
it already is, I'd take the patch for now.  It would catch this
particular pilot error.

I however would think adding a dependency in t/Makefile to
make sure the top level is built before starting to run test a
overkill (I think I've seen some projects to do that), so I'd
not go there.