[PATCH 0/4] Run test suite without dashed commands in PATH

[PATCH 0/4] Run test suite without dashed commands in PATH

[Matthew Ogilvie]

This patch series runs the test suite without the dashed commands
in the PATH.  This improves the usefulness of the test suite
as examples of how to do things, and more importantly,
minimizes any possibility of regressions in bindir-installed
scripts that might prevent them from working in a standard
install where most dashed commands are not in the PATH (git-cvsserver
was broken in this way until Sep 2009: d2feb01aa5).

The scripts in the "test-bin" directory that patch 3 creates
can also be useful for manually testing a build of git without
installing it; you typically don't need to set any environment
variables (except maybe PATH) for the test-bin scripts to use
the build properly.

Trivial note: This is a cleaned up resend of part of a hodgepodge
cvsserver patch series that I sent last January.

---------

By the way, has anyone considered the possibility of splitting
up the large directory at the top of the git source tree?  I
suspect that no one is interested, or it would have been done
already, but I thought I would mention it anyway.  Perhaps split
off separate directories for libgit.a, builtins, other C-based
excutables, test support executables, scripts, output execdir, and
output intermediate (object) files for each of the other
directories.  Or some subset of these.

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

Matthew Ogilvie (4):
  t2300: use documented technique to invoke git-sh-setup
  t3409 t4107 t7406: use dashless commands
  build dashless "test-bin" directory similar to installed bindir
  run test suite without dashed git-commands in PATH

 .gitignore                         |    1 +
 INSTALL                            |    8 +++++-
 Makefile                           |   46 +++++++++++++++++++++++++----------
 t/README                           |    8 ++++++
 t/t2300-cd-to-toplevel.sh          |    2 +-
 t/t3409-rebase-preserve-merges.sh  |    6 ++--
 t/t4107-apply-ignore-whitespace.sh |   20 ++++++++--------
 t/t7406-submodule-update.sh        |    4 +-
 t/test-lib.sh                      |   33 ++++++++++++++++---------
 test-bin-wrapper.sh                |   13 ++++++++++
 10 files changed, 99 insertions(+), 42 deletions(-)
 create mode 100644 test-bin-wrapper.sh

[PATCH 1/4] t2300: use documented technique to invoke git-sh-setup

[Matthew Ogilvie]

This is needed to allow the test suite to run against a standard
install bin directory instead of GIT_EXEC_PATH.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 t/t2300-cd-to-toplevel.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
index 3b01ad2..9965bc5 100755
--- a/t/t2300-cd-to-toplevel.sh
+++ b/t/t2300-cd-to-toplevel.sh
@@ -8,7 +8,7 @@ test_cd_to_toplevel () {
 	test_expect_success $3 "$2" '
 		(
 			cd '"'$1'"' &&
-			. git-sh-setup &&
+			. "$(git --exec-path)"/git-sh-setup &&
 			cd_to_toplevel &&
 			[ "$(pwd -P)" = "$TOPLEVEL" ]
 		)
-- 
1.6.4.GIT

[PATCH 2/4] t3409 t4107 t7406: use dashless commands

[Matthew Ogilvie]

This is needed to allow test suite to run against a standard
install bin directory instead of GIT_EXEC_PATH.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 t/t3409-rebase-preserve-merges.sh  |    6 +++---
 t/t4107-apply-ignore-whitespace.sh |   20 ++++++++++----------
 t/t7406-submodule-update.sh        |    4 ++--
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 297d165..8f785e7 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -32,14 +32,14 @@ export GIT_AUTHOR_EMAIL
 test_expect_success 'setup for merge-preserving rebase' \
 	'echo First > A &&
 	git add A &&
-	git-commit -m "Add A1" &&
+	git commit -m "Add A1" &&
 	git checkout -b topic &&
 	echo Second > B &&
 	git add B &&
-	git-commit -m "Add B1" &&
+	git commit -m "Add B1" &&
 	git checkout -f master &&
 	echo Third >> A &&
-	git-commit -a -m "Modify A2" &&
+	git commit -a -m "Modify A2" &&
 
 	git clone ./. clone1 &&
 	cd clone1 &&
diff --git a/t/t4107-apply-ignore-whitespace.sh b/t/t4107-apply-ignore-whitespace.sh
index 484654d..b04fc8f 100755
--- a/t/t4107-apply-ignore-whitespace.sh
+++ b/t/t4107-apply-ignore-whitespace.sh
@@ -136,37 +136,37 @@ void print_int(int num) {
 EOF
 
 test_expect_success 'file creation' '
-	git-apply patch1.patch
+	git apply patch1.patch
 '
 
 test_expect_success 'patch2 fails (retab)' '
-	test_must_fail git-apply patch2.patch
+	test_must_fail git apply patch2.patch
 '
 
 test_expect_success 'patch2 applies with --ignore-whitespace' '
-	git-apply --ignore-whitespace patch2.patch
+	git apply --ignore-whitespace patch2.patch
 '
 
 test_expect_success 'patch2 reverse applies with --ignore-space-change' '
-	git-apply -R --ignore-space-change patch2.patch
+	git apply -R --ignore-space-change patch2.patch
 '
 
 git config apply.ignorewhitespace change
 
 test_expect_success 'patch2 applies (apply.ignorewhitespace = change)' '
-	git-apply patch2.patch
+	git apply patch2.patch
 '
 
 test_expect_success 'patch3 fails (missing string at EOL)' '
-	test_must_fail git-apply patch3.patch
+	test_must_fail git apply patch3.patch
 '
 
 test_expect_success 'patch4 fails (missing EOL at EOF)' '
-	test_must_fail git-apply patch4.patch
+	test_must_fail git apply patch4.patch
 '
 
 test_expect_success 'patch5 applies (leading whitespace)' '
-	git-apply patch5.patch
+	git apply patch5.patch
 '
 
 test_expect_success 'patches do not mangle whitespace' '
@@ -175,11 +175,11 @@ test_expect_success 'patches do not mangle whitespace' '
 
 test_expect_success 're-create file (with --ignore-whitespace)' '
 	rm -f main.c &&
-	git-apply patch1.patch
+	git apply patch1.patch
 '
 
 test_expect_success 'patch5 fails (--no-ignore-whitespace)' '
-	test_must_fail git-apply --no-ignore-whitespace patch5.patch
+	test_must_fail git apply --no-ignore-whitespace patch5.patch
 '
 
 test_done
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 2d33d9e..8e2449d 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -14,8 +14,8 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD.
 
 compare_head()
 {
-    sha_master=`git-rev-list --max-count=1 master`
-    sha_head=`git-rev-list --max-count=1 HEAD`
+    sha_master=`git rev-list --max-count=1 master`
+    sha_head=`git rev-list --max-count=1 HEAD`
 
     test "$sha_master" = "$sha_head"
 }
-- 
1.6.4.GIT

[PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

[Matthew Ogilvie]

The new test-bin directory contains wrapper scripts
for executables that will be installed into the standard
bindir.  It explicitly does not contain most dashed-commands.
The scripts automatically set environment variables to run out
of the source tree, not the installed directory.

This will allow running the test suite without dashed commands in
the PATH, and provides a simplified way to run custom built git
executables without installing them first.

test-bin also contains wrappers for some test suite support executables,
where the test suite will soon make use of them.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 .gitignore          |    1 +
 INSTALL             |    8 +++++++-
 Makefile            |   46 +++++++++++++++++++++++++++++++++-------------
 test-bin-wrapper.sh |   13 +++++++++++++
 4 files changed, 54 insertions(+), 14 deletions(-)
 create mode 100644 test-bin-wrapper.sh

diff --git a/.gitignore b/.gitignore
index ac02a58..c981e4c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -10,6 +10,7 @@
 /git-apply
 /git-archimport
 /git-archive
+/test-bin/
 /git-bisect
 /git-bisect--helper
 /git-blame
diff --git a/INSTALL b/INSTALL
index be504c9..d092d33 100644
--- a/INSTALL
+++ b/INSTALL
@@ -39,7 +39,13 @@ Issues of note:
    with --disable-transition option to avoid this.
 
  - You can use git after building but without installing if you
-   wanted to.  Various git commands need to find other git
+   want to.
+
+   The simplest option is to use the wrapper scripts that are built
+   and saved into `pwd`/test-bin, by either putting test-bin in your
+   PATH, or invoking the scripts in test-bin using their full paths.
+
+   Alternatively, various git commands need to find other git
    commands and scripts to do their work, so you would need to
    arrange a few environment variables to tell them that their
    friends will be found in your built source area instead of at
diff --git a/Makefile b/Makefile
index 5a0b3d4..e153ff0 100644
--- a/Makefile
+++ b/Makefile
@@ -416,6 +416,15 @@ ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
 # what 'all' will build but not install in gitexecdir
 OTHER_PROGRAMS = git$X
 
+# what test wrappers are needed and 'install' will install, in bindir
+BINDIR_PROGRAMS_NEED_X += git
+BINDIR_PROGRAMS_NEED_X += git-upload-pack
+BINDIR_PROGRAMS_NEED_X += git-receive-pack
+BINDIR_PROGRAMS_NEED_X += git-upload-archive
+BINDIR_PROGRAMS_NEED_X += git-shell
+
+BINDIR_PROGRAMS_NO_X += git-cvsserver
+
 # Set paths to tools early so that they can be used for version tests.
 ifndef SHELL_PATH
 	SHELL_PATH = /bin/sh
@@ -1690,19 +1699,27 @@ endif
 
 ### Testing rules
 
-TEST_PROGRAMS += test-chmtime$X
-TEST_PROGRAMS += test-ctype$X
-TEST_PROGRAMS += test-date$X
-TEST_PROGRAMS += test-delta$X
-TEST_PROGRAMS += test-dump-cache-tree$X
-TEST_PROGRAMS += test-genrandom$X
-TEST_PROGRAMS += test-match-trees$X
-TEST_PROGRAMS += test-parse-options$X
-TEST_PROGRAMS += test-path-utils$X
-TEST_PROGRAMS += test-sha1$X
-TEST_PROGRAMS += test-sigchain$X
+TEST_PROGRAMS_NEED_X += test-chmtime
+TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-date
+TEST_PROGRAMS_NEED_X += test-delta
+TEST_PROGRAMS_NEED_X += test-dump-cache-tree
+TEST_PROGRAMS_NEED_X += test-genrandom
+TEST_PROGRAMS_NEED_X += test-match-trees
+TEST_PROGRAMS_NEED_X += test-parse-options
+TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-sha1
+TEST_PROGRAMS_NEED_X += test-sigchain
+
+TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
-all:: $(TEST_PROGRAMS)
+test_bindir_programs := $(patsubst %,test-bin/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+
+all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+
+test-bin/%: test-bin-wrapper.sh GIT-CFLAGS
+	@mkdir -p test-bin
+	$(QUIET_GEN)sed -e 's|__GIT_EXEC_PATH__|$(shell pwd)|' -e 's|__PROG__|$(@F)|' < $< > $@ && chmod +x $@
 
 # GNU make supports exporting all variables by "export" without parameters.
 # However, the environment gets quite big, and some programs have problems
@@ -1763,11 +1780,13 @@ endif
 gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir))
 export gitexec_instdir
 
+install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X)
+
 install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git$X git-upload-pack$X git-receive-pack$X git-upload-archive$X git-shell$X git-cvsserver '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 ifndef NO_PERL
 	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
@@ -1878,6 +1897,7 @@ clean:
 		$(LIB_FILE) $(XDIFF_LIB)
 	$(RM) $(ALL_PROGRAMS) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
+	$(RM) -r test-bin
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h TAGS tags cscope*
 	$(RM) -r autom4te.cache
 	$(RM) config.log config.mak.autogen config.mak.append config.status config.cache
diff --git a/test-bin-wrapper.sh b/test-bin-wrapper.sh
new file mode 100644
index 0000000..a01eee1
--- /dev/null
+++ b/test-bin-wrapper.sh
@@ -0,0 +1,13 @@
+#!/bin/sh
+
+# test-bin-wrapper.sh: Template for git executable wrapper scripts
+# to run test suite against sandbox, but with only bindir-installed
+# executables in PATH.  The Makefile copies this into various
+# files in test-bin, substituting __GIT_EXEC_PATH__ and __PROG__.
+
+GIT_EXEC_PATH="__GIT_EXEC_PATH__"
+GIT_TEMPLATE_DIR="__GIT_EXEC_PATH__/templates/blt"
+GITPERLLIB="__GIT_EXEC_PATH__/perl/blib/lib"
+export GIT_EXEC_PATH GIT_TEMPLATE_DIR GITPERLLIB
+
+exec "${GIT_EXEC_PATH}/__PROG__" "$@"
-- 
1.6.4.GIT

[PATCH 4/4] run test suite without dashed git-commands in PATH

[Matthew Ogilvie]

Only put test-bin in the PATH (do not put GIT_EXEC_PATH in the PATH),
to emulate the default installed user environment, and
ensure all the programs run correctly in such an
environment.  This is now the default, although
it can be overridden with a --with-dashes test option when running
tests.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 t/README      |    8 ++++++++
 t/test-lib.sh |   33 +++++++++++++++++++++------------
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/t/README b/t/README
index d8f6c7d..b61c34b 100644
--- a/t/README
+++ b/t/README
@@ -75,6 +75,14 @@ appropriately before running "make".
 	As the names depend on the tests' file names, it is safe to
 	run the tests with this option in parallel.
 
+--with-dashes::
+	By default tests are run without dashed forms of
+	commands (like git-commit) in the PATH (it only uses
+	wrappers from TOP/git-bin).  Use this option to include TOP
+	in the PATH, which conains all the dashed forms of commands.
+	This option is currently implied by other options like --valgrind
+	and GIT_TEST_INSTALLED.
+
 Skipping Tests
 --------------
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ec3336a..54570ac 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -105,6 +105,8 @@ do
 		verbose=t; shift ;;
 	-q|--q|--qu|--qui|--quie|--quiet)
 		quiet=t; shift ;;
+	--with-dashes)
+		with_dashes=t; shift ;;
 	--no-color)
 		color=; shift ;;
 	--no-python)
@@ -551,19 +553,8 @@ test_done () {
 # Test the binaries we have just built.  The tests are kept in
 # t/ subdirectory and are run in 'trash directory' subdirectory.
 TEST_DIRECTORY=$(pwd)
-if test -z "$valgrind"
+if test -n "$valgrind"
 then
-	if test -z "$GIT_TEST_INSTALLED"
-	then
-		PATH=$TEST_DIRECTORY/..:$PATH
-		GIT_EXEC_PATH=$TEST_DIRECTORY/..
-	else
-		GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
-		error "Cannot run git from $GIT_TEST_INSTALLED."
-		PATH=$GIT_TEST_INSTALLED:$TEST_DIRECTORY/..:$PATH
-		GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
-	fi
-else
 	make_symlink () {
 		test -h "$2" &&
 		test "$1" = "$(readlink "$2")" || {
@@ -625,6 +616,24 @@ else
 	PATH=$GIT_VALGRIND/bin:$PATH
 	GIT_EXEC_PATH=$GIT_VALGRIND/bin
 	export GIT_VALGRIND
+elif test -n "$GIT_TEST_INSTALLED" ; then
+	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
+	error "Cannot run git from $GIT_TEST_INSTALLED."
+	PATH=$GIT_TEST_INSTALLED:$TEST_DIRECTORY/..:$PATH
+	GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
+else # normal case, use ../test-bin only unless $with_dashes:
+	git_bin_dir="$TEST_DIRECTORY/../test-bin"
+	if ! test -x "$git_bin_dir/git" ; then
+		if test -z "$with_dashes" ; then
+			say "$git_bin_dir/git is not executable; using GIT_EXEC_PATH"
+		fi
+		with_dashes=t
+	fi
+	PATH="$git_bin_dir:$PATH"
+	GIT_EXEC_PATH=$TEST_DIRECTORY/..
+	if test -n "$with_dashes" ; then
+		PATH="$TEST_DIRECTORY/..:$PATH"
+	fi
 fi
 GIT_TEMPLATE_DIR=$(pwd)/../templates/blt
 unset GIT_CONFIG
-- 
1.6.4.GIT

Re: [PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

[Junio C Hamano]

Matthew Ogilvie <mmogilvi_git@miniinfo.net> writes:

> The new test-bin directory contains wrapper scripts for executables that
> will be installed into the standard bindir.  It explicitly does not
> contain most dashed-commands.  The scripts automatically set environment
> variables to run out of the source tree, not the installed directory.

Even though I haven't tried the series, I like most of the things I see in
this series.

 - Patch #1 and #2 are good and are independent from the later patches, as
   without them running tests with GIT_TEST_INSTALLED would not work.

   By the way, 6720721 (test-lib.sh: Allow running the test suite against
   installed git, 2009-03-16) failed to document the feature in t/README.
   Could you please fix this while you are at it?

 - Running tests before installation in an environment slightly closer to
   the final installation (i.e. lacks dashed commands in the $PATH during
   the time tests are run) is a good direction to go.

 - I like the Makefile changes that uses these new BINDIR_PROGRAMS_NEED_X,
   TEST_PROGRAMS_NEED_X.  Parameterizing commands listed in the actions
   part is good.

 - It certainly is _possible_ to add $(pwd)/test-bin to $PATH instead of
   the established practice of using GIT_EXEC_PATH for every day/permanent
   use without installation, but I doubt we should _encourage_ it for a
   few reasons:

   . The set-up will force one extra exec due to the wrapper; this is good
     for the purpose of running tests, but unnecessary for a set-up for
     every day/permanent use by people, compared with the already working
     approach.  The user needs to change an environment variable _anyway_
     (either GIT_EXEC_PATH with the traditional approach, or PATH with
     your patch).

   . The new component to be added to $PATH shouldn't be named "test-bin/"
     if it is meant for every day/permanent use.

   . Advertising this forces the Makefile build test-bin/ contents from
     "all" target.  I think test-bin/ should only depend on "test" (iow,
     after "make all && make install" there shouldn't have to be "test-bin"
     directory.

   I would rather treat it an unintended side-effect that you can add that
   directory to the $PATH.  It is designed to work in such an environment
   (otherwise the tests won't exercise the version of git they are meant
   to test), but I do not think it is _meant_ to be _used_  by end users
   that way.  And an unintended side-effect does not have to be mentioned
   in INSTALL (especially with the directory name with "test" in it).

Re: [PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

[Jeff King]

On Sat, Nov 28, 2009 at 11:44:39AM -0800, Junio C Hamano wrote:

>    . Advertising this forces the Makefile build test-bin/ contents from
>      "all" target.  I think test-bin/ should only depend on "test" (iow,
>      after "make all && make install" there shouldn't have to be "test-bin"
>      directory.

Would implementing it that way mean that:

  make && cd t && make

does not work (or worse, might silently use stale information in
test-bin)? Dealing with this is part of the reason the valgrind code
(which similarly sets up a pseudo-installed directory) does everything
in test-lib.sh, and not as part of the make process.

-Peff

Re: [PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Nov 28, 2009 at 11:44:39AM -0800, Junio C Hamano wrote:
>
>>    . Advertising this forces the Makefile build test-bin/ contents from
>>      "all" target.  I think test-bin/ should only depend on "test" (iow,
>>      after "make all && make install" there shouldn't have to be "test-bin"
>>      directory.
>
> Would implementing it that way mean that:
>
>   make && cd t && make
>
> does not work (or worse, might silently use stale information in
> test-bin)?

Why can't t/Makefile have a dependency on its 'default' target that goes
up and prepares test-bin/, i.e. "cd .. && make test-bin-stuff"?

Re: [PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

[Jeff King]

On Sat, Nov 28, 2009 at 06:32:33PM -0800, Junio C Hamano wrote:

> > Would implementing it that way mean that:
> >
> >   make && cd t && make
> >
> > does not work (or worse, might silently use stale information in
> > test-bin)?
> 
> Why can't t/Makefile have a dependency on its 'default' target that goes
> up and prepares test-bin/, i.e. "cd .. && make test-bin-stuff"?

Yeah, that would work (I really should have phrased my other response
less as a critique and more as "please don't break this workflow"). But
I don't think the default target would be enough. I would also expect

  make && cd t && make tXXXX-YYYY.sh

to work correctly (and to be pedantic, I am actually more interested in
the equivalent situation that one has one window looking at code and
compiling, and another window running a test script, but they are
functionally equivalent here).

I also like to be able to simply run ./tXXXX-YYYY.sh. I can accept
losing that if there is something to be gained, but if we can keep it, I
suspect I am not the only one who uses it.

-Peff

Re: [PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   make && cd t && make tXXXX-YYYY.sh
>
> to work correctly ...
>
> I also like to be able to simply run ./tXXXX-YYYY.sh.

I think both can be done by running "cd .. && make test-bin-stuff" from
test-lib.sh if you wanted to.  Isn't it essentially what you do for
valgrind?

Re: [PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

[Jeff King]

On Sat, Nov 28, 2009 at 09:07:05PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >   make && cd t && make tXXXX-YYYY.sh
> >
> > to work correctly ...
> >
> > I also like to be able to simply run ./tXXXX-YYYY.sh.
> 
> I think both can be done by running "cd .. && make test-bin-stuff" from
> test-lib.sh if you wanted to.  Isn't it essentially what you do for
> valgrind?

Yes, except that the valgrind setup doesn't actually interact with the
Makefile; it just assumes that git-* is interesting. I would be happy
with a solution that is triggered from test-lib. It may even be possible
to refactor the valgrind stuff, but I'm not sure. What Dscho eventually
submitted doesn't bear much resemblence to my original attempt, and I've
forgotten all of the details.

-Peff

Re: [PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

[Matthew Ogilvie]

On Sat, Nov 28, 2009 at 06:32:33PM -0800, Junio C Hamano wrote:
> Why can't t/Makefile have a dependency on its 'default' target that goes
> up and prepares test-bin/, i.e. "cd .. && make test-bin-stuff"?

That wouldn't help with someone manually running specific tests,
which is a common way to run tests when working on git:

   hackhackhack && make && cd t && ./tXXXX-*.sh

Perhaps we could invoke make from test-lib.sh, but at that point
it starts to look more attractive to just do it the same way as the
valgrind option, even though that means duplicating the appropriate
list of executables in test-lib.sh.

Also, "make all" already builds some test suite support
binaries (test-*).  Is it really worth any effort to leave
a few short wrapper scripts out of "make all" when it is already
building several test binaries?

(See also my much longer email response that crossed this one in the
mail.)

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

Re: [PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

[Matthew Ogilvie]

On Sat, Nov 28, 2009 at 11:44:39AM -0800, Junio C Hamano wrote:
>  - Patch #1 and #2 are good and are independent from the later patches, as
>    without them running tests with GIT_TEST_INSTALLED would not work.
> 
>    By the way, 6720721 (test-lib.sh: Allow running the test suite against
>    installed git, 2009-03-16) failed to document the feature in t/README.
>    Could you please fix this while you are at it?

Sure, I'll include another patch for this when I re-roll the
series.  It will probably mention something about still needing
a build so that it can access the test-* support executables.

>  - It certainly is _possible_ to add $(pwd)/test-bin to $PATH instead of
>    the established practice of using GIT_EXEC_PATH for every day/permanent
>    use without installation, but I doubt we should _encourage_ it for a
>    few reasons:
> 
>    . The set-up will force one extra exec due to the wrapper; this is good
>      for the purpose of running tests, but unnecessary for a set-up for
>      every day/permanent use by people, compared with the already working
>      approach.  The user needs to change an environment variable _anyway_
>      (either GIT_EXEC_PATH with the traditional approach, or PATH with
>      your patch).
> 
>    . The new component to be added to $PATH shouldn't be named "test-bin/"
>      if it is meant for every day/permanent use.
> 
>    . Advertising this forces the Makefile build test-bin/ contents from
>      "all" target.  I think test-bin/ should only depend on "test" (iow,
>      after "make all && make install" there shouldn't have to be "test-bin"
>      directory.
> 
>    I would rather treat it an unintended side-effect that you can add that
>    directory to the $PATH.  It is designed to work in such an environment
>    (otherwise the tests won't exercise the version of git they are meant
>    to test), but I do not think it is _meant_ to be _used_  by end users
>    that way.  And an unintended side-effect does not have to be mentioned
>    in INSTALL (especially with the directory name with "test" in it).

I personally like the idea of being able to use an uninstalled
build without touching environment variables at all.  Just specify
the full path to the the version you want to run on the
command line, as in: ~/SANDBOX/test-bin/git WHATEVER
Especially handy for trying "ssh MACHINE /PATH/SANDBOX/...".

FYI: There are already a number of test suite support executables
built by "make all" (test-*).  It might be hard to eliminate them
from "all" without risking stale executables.  As Jeff
King <peff@peff.net> pointed out in a separate email, some people
(including me) often don't use the top-level "make test" target
to run tests.

I'm still thinking about this.  I've noted some possible changes
to the patch series below, some of which are mutually exclusive.
Any opinions?

Options geared towards isolating/hiding test-bin:

  1. Scrap the part of the patch that modifies INSTALL.

  2. Perhaps use hardlinks, symlinks, and/or copies within test-bin,
     instead of wrapper scripts, to eliminate the extra exec.  Since
     test-lib.sh already sets up necessary environment variables,
     they don't strictly need to be set in the wrappers as
     well.  On the other hand, hardlinks and copies are potentially
     vulnerable to stale executable issues, and symlinks typically
     don't work on Windows.  

  3. Scrap pre-built test-bin completely, and switch to a solution
     that more closely resembles the valgrind option (have test-lib.sh
     build the directory).  This can't use the same makefile variables
     to define the contents of the directory, though.

Options geared towards making something like test-bin an official way
to run an uninstalled build:

  4. Rename test-bin.  Perhaps "bin-wrappers", "bin-dashless",
     "bin-install", "bin", or "bindir".  Any preferences?

  5. The current patch doesn't quite handle the simple
     "~/SANDBOX/test-bin/gitSOMETHING WHATEVER" idiom perfectly
     if the executable (gitSOMETHING) tries to run additional
     git commands without adjusting the PATH first.  I could enhance
     the wrapper to prefix test-bin onto the PATH just in case it
     isn't there already.

Other cleanup options:

  6. There is a stale script issue if someone does something like:
       make
       cp -a . /some/other/path
       cd /some/other/path
       [optional modifications, without a "make clean"]
       make
       [run tests; uses wrong executables...]
     Including GIT-CFLAGS as a makefile dependency for the
     wrappers was intended to address this, but looking
     closer, I don't think it works.  Perhaps I should
     include $(shell pwd) in GIT-CFLAGS, or make a new GIT-PWD target
     that works similarly to GIT-CFLAGS.  Without this, a workaround
     (and probably best option overall) is to do a "make clean" after
     copying a sandbox.

  7. Enable similar dashless environment when
     GIT_TEST_INSTALLED and/or valgrind are enabled?

  8. Include wrappers for other dashed-commands in test-bin, which
     would always fail, in case someone runs tests with an installed
     GIT_EXEC_PATH already in their PATH.  This might catch a new test
     using dashes in such an environment.  I don't really think this
     is worth it, though.  Most people don't have GIT_EXEC_PATH in their
     PATH, and some such person would notice any problems soon.

  9. This may be outside the scope of this patch series, but perhaps
     git executables could try to find argv[0] in the PATH
     (if argv[0] is not absolute), and see if they can find various other
     executables (GIT_EXEC_PATH) and data files (perl, templates,
     etc) using paths relative to itself.  This may include
     manually dereferencing argv[0] if it is a symlink.  GIT_EXEC_PATH
     and friends still takes precedence, but only fallback on
     compile-time defaults if "find relative to argv[0]" fails.
     It looks like Makefile RUNTIME_PREFIX enables something like
     this, but it is currently disabled by default on most platforms.

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

Re: [PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

[Nanako Shiraishi]

Quoting Matthew Ogilvie <mmogilvi_git@miniinfo.net>

>   4. Rename test-bin.  Perhaps "bin-wrappers", "bin-dashless",
>      "bin-install", "bin", or "bindir".  Any preferences?

"gitrun" or "rungit"?

I prefer the latter because too many files begin with "git" after compiling.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/