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

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

[Matthew Ogilvie]

This patch series runs the test suite without the dashed commands
in the PATH.

Changes since version 1:

  - Added patch 3 that documents the pre-existing GIT_TEST_INSTALLED
    feature.

  - Rename what used to be "test-bin" directory as "bin-wrappers", to
    more clearly describe what it is.

  - Split off patch 6 (INSTALL documention) from patch
    4 (was patch 3), describing how the bin-wrappers directory
    is a very convenient way to manually test an uninstalled build.
    I also reworded it a bit to mention the downsides.  Junio
    doesn't seem to agree this is useful; splitting it off makes
    it easy to leave it out.

Not changed:

There was some discussion about not building the bin-wrappers directory
unless you are actually running tests.  But I don't really think it
is worth the additional complexity to manage this.  The "make all"
target is already building several full-up binaries to support the
test suite (not just the small sed-ed scripts that this adds).  And any
such solution would need to deal with parallel execution locking
issues, as well as additional uglyness (either duplicating a list
of bindir executables within test-lib.sh itself, or calling into a
parent directory makefile that might already be running in
a grandparent process).

Matthew Ogilvie (6):
  t2300: use documented technique to invoke git-sh-setup
  t3409 t4107 t7406: use dashless commands
  t/README: Document GIT_TEST_INSTALLED and GIT_TEST_EXEC_PATH
  build dashless "bin-wrappers" directory similar to installed bindir
  run test suite without dashed git-commands in PATH
  INSTALL: document a simpler way to run uninstalled builds

 .gitignore                         |    1 +
 INSTALL                            |   11 +++++++-
 Makefile                           |   49 ++++++++++++++++++++++++++---------
 t/README                           |   21 +++++++++++++++
 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 +++++++++++++++---------
 wrap-for-bin.sh                    |   15 +++++++++++
 10 files changed, 120 insertions(+), 42 deletions(-)
 create mode 100644 wrap-for-bin.sh

[PATCH v2 1/6] 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 v2 2/6] 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 v2 3/6] t/README: Document GIT_TEST_INSTALLED and GIT_TEST_EXEC_PATH

[Matthew Ogilvie]

These were added without documentation in 2009-03-16 (6720721).

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 t/README |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index d8f6c7d..4e1d7dd 100644
--- a/t/README
+++ b/t/README
@@ -75,6 +75,19 @@ 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.
 
+You can also set the GIT_TEST_INSTALLED environment variable to
+the bindir of an existing git installation to test that installation.
+You still need to have built this git sandbox, from which various
+test-* support programs, templates, and perl libraries are used.
+If your installed git is incomplete, it will silently test parts of
+your built version instead.
+
+When using GIT_TEST_INSTALLED, you can also set GIT_TEST_EXEC_PATH to
+override the location of the dashed-form subcommands (what
+GIT_EXEC_PATH would be used for during normal operation).
+GIT_TEST_EXEC_PATH defaults to `$GIT_TEST_INSTALLED/git --exec-path`.
+
+
 Skipping Tests
 --------------
 
-- 
1.6.4.GIT

[PATCH v2 4/6] build dashless "bin-wrappers" directory similar to installed bindir

[Matthew Ogilvie]

The new bin-wrappers 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.  It also provides a simplified way to test run custom
built git executables without installing them first.

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

There was some discussion of only building bin-wrappers when
actually running tests, but I don't think it is worth the
extra complexity.  See the cover letter (0/6).

 .gitignore      |    1 +
 Makefile        |   49 ++++++++++++++++++++++++++++++++++++-------------
 wrap-for-bin.sh |   15 +++++++++++++++
 3 files changed, 52 insertions(+), 13 deletions(-)
 create mode 100644 wrap-for-bin.sh

diff --git a/.gitignore b/.gitignore
index ac02a58..5d32289 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 /GIT-CFLAGS
 /GIT-GUI-VARS
 /GIT-VERSION-FILE
+/bin-wrappers/
 /git
 /git-add
 /git-add--interactive
diff --git a/Makefile b/Makefile
index 5a0b3d4..77892ec 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,30 @@ 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 %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+
+all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+
+bin-wrappers/%: wrap-for-bin.sh
+	@mkdir -p bin-wrappers
+	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	     -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 +1783,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 +1900,7 @@ clean:
 		$(LIB_FILE) $(XDIFF_LIB)
 	$(RM) $(ALL_PROGRAMS) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
+	$(RM) -r bin-wrappers
 	$(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/wrap-for-bin.sh b/wrap-for-bin.sh
new file mode 100644
index 0000000..ee2bc98
--- /dev/null
+++ b/wrap-for-bin.sh
@@ -0,0 +1,15 @@
+#!/bin/sh
+
+# wrap-for-bin.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 bin-wrappers, 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"
+PATH="__GIT_EXEC_PATH__/bin-wrappers:$PATH"
+export GIT_EXEC_PATH GIT_TEMPLATE_DIR GITPERLLIB PATH
+
+exec "${GIT_EXEC_PATH}/__PROG__" "$@"
-- 
1.6.4.GIT

[PATCH v2 5/6] run test suite without dashed git-commands in PATH

[Matthew Ogilvie]

Only put bin-wrappers in the PATH (not GIT_EXEC_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 4e1d7dd..8c5d892 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.
+
 You can also set the GIT_TEST_INSTALLED environment variable to
 the bindir of an existing git installation to test that installation.
 You still need to have built this git sandbox, from which various
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ec3336a..85377c8 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 ../bin-wrappers only unless $with_dashes:
+	git_bin_dir="$TEST_DIRECTORY/../bin-wrappers"
+	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

[PATCH v2 6/6] INSTALL: document a simpler way to run uninstalled builds

[Matthew Ogilvie]

The new scripts automatically saved in the bin-wrappers
directory allow you test run a build when you have neither installed
git nor tweaked environment variables.  Mention this in INSTALL,
along with the slight performance issue of doing so.

This can be especially handy for manually testing
network-invoked git (from ssh, web servers, or similar), but
it is also handy with a plain command prompt.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 INSTALL |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/INSTALL b/INSTALL
index be504c9..ee718c6 100644
--- a/INSTALL
+++ b/INSTALL
@@ -39,7 +39,19 @@ 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 for running some manual tests of a build
+   before installing it is to use the wrapper scripts that are built
+   and saved into `pwd`/bin-wrappers.  Either invoke the scripts in
+   bin-wrappers using their full paths, put bin-wrappers in your
+   PATH, or copy/symlink just the bin-wrappers scripts into somewhere
+   already in your PATH.  But this option is slightly inefficient,
+   so for a more permanent solution we recommend either installing
+   git (you can set a prefix to install right next to your
+   build directory), or use the alternative below.
+
+   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
-- 
1.6.4.GIT

Re: [PATCH v2 6/6] INSTALL: document a simpler way to run uninstalled builds

[Junio C Hamano]

Matthew Ogilvie <mmogilvi_git@miniinfo.net> writes:

> diff --git a/INSTALL b/INSTALL
> index be504c9..ee718c6 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -39,7 +39,19 @@ 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 for running some manual tests of a build
> +   before installing it is to use the wrapper scripts that are built
> +   and saved into `pwd`/bin-wrappers.  Either invoke the scripts in
> +   bin-wrappers using their full paths, put bin-wrappers in your
> +   PATH, or copy/symlink just the bin-wrappers scripts into somewhere
> +   already in your PATH.  But this option is slightly inefficient,
> +   so for a more permanent solution we recommend either installing
> +   git (you can set a prefix to install right next to your
> +   build directory), or use the alternative below.

Drop "simplest" and "slightly".

I think you forgot to mention an important improvement you made since the
earlier iteration on the wrap-for-bin script in the cover letter.  With
the new wrap-for-bin, we do not have to worry about the case a random
binary in the directory is run without first adding anything to user's
$PATH anymore, no?

With that change, this round's implementation deserves to be recommended
as the new preferred way to run "use after building without installing", I
think.  So how about making the text like this?

    You can use git after building but without installing if you want to
    test drive it.  Simply run git found in bin-wrappers directory in the
    build directory, or prepend that directory to your $PATH.  This
    however is less efficient than running an installed git, as you always
    need an extra fork+exec to run any git subcommand.

    It is still possible to use git without installing by setting a few
    environment variables, which was the way this was done traditionally.
    But using git found in bin-wrappers directory in the build directory
    is far simpler.  Just for a historical reference, the old way went
    like this:

	GIT_EXEC_PATH=`pwd`
	PATH=`pwd`:$PATH
	GITPERLLIB=`pwd`/perl/blib/lib
	export GIT_EXEC_PATH PATH GITPERLLIB

Re: [PATCH v2 5/6] run test suite without dashed git-commands in PATH

[Jakub Narebski]

Matthew Ogilvie <mmogilvi_git@miniinfo.net> writes:

> diff --git a/t/README b/t/README
> index 4e1d7dd..8c5d892 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.
> +

Shouldn't it be 'TOP/bin-wrappers' and not 'TOP/git-bin'?  
Shouldn't TOP be explained somewhere, or is it obvious in the context?

s/conains/contains/.

> +else # normal case, use ../bin-wrappers only unless $with_dashes:

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH v2 5/6] run test suite without dashed git-commands in PATH

[Matthew Ogilvie]

On Tue, Dec 01, 2009 at 09:24:46AM -0800, Jakub Narebski wrote:
> Matthew Ogilvie <mmogilvi_git@miniinfo.net> writes:
> > +--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.
> > +
> 
> Shouldn't it be 'TOP/bin-wrappers' and not 'TOP/git-bin'?  
> Shouldn't TOP be explained somewhere, or is it obvious in the context?
> 
> s/conains/contains/.

I've appended a incremental patch for these.

Junio, do you want me to re-roll the 3 bin-wrappers patches to
include both this and the "@@ vs __" patch, or do you want
to just add or squash them in from the emails?

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

From: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
Date: Tue, 1 Dec 2009 22:16:19 -0700
Subject: [PATCH] t/README: fix spelling in --with-dashes documentation

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 t/README |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/t/README b/t/README
index 8c5d892..dcd3ebb 100644
--- a/t/README
+++ b/t/README
@@ -78,10 +78,11 @@ appropriately before running "make".
 --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.
+	wrappers from ../bin-wrappers).  Use this option to include
+	the build directory (..) in the PATH, which contains all
+	the dashed forms of commands.  This option is currently
+	implied by other options like --valgrind and
+	GIT_TEST_INSTALLED.
 
 You can also set the GIT_TEST_INSTALLED environment variable to
 the bindir of an existing git installation to test that installation.
-- 
1.6.4.GIT

Re: [PATCH v2 5/6] run test suite without dashed git-commands in PATH

[Junio C Hamano]

Matthew Ogilvie <mmogilvi_git@miniinfo.net> writes:

> Junio, do you want me to re-roll the 3 bin-wrappers patches to
> include both this and the "@@ vs __" patch, or do you want
> to just add or squash them in from the emails?

I expect that very soon I will be paying much less attention to topics
that will not be in 1.6.6 (in fact, I maybe am already as of tonight), to
save my mental bandwidth to concentrate more on bugfixes and regressions.

Please re-roll and feed me complete patches for anything that are not in
'next', telling me which ones to drop from 'pu' if I have older versions.

Just FYI, earlier you said to me something like "I see you already queued
this to 'pu', ...", but please be aware that being in 'pu' does not mean
that much---not much more than being in the mailing list archive.  They
are fair game for rewriting, replacing and dropping.  I merely keep them
on 'pu' so that it would be easier for me and others to look at them than
having to hunt for them in the mailing list archive.

Thanks.

Re: [PATCH v2 4/6] build dashless "bin-wrappers" directory similar to installed bindir

[Junio C Hamano]

Matthew Ogilvie <mmogilvi_git@miniinfo.net> writes:

> diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh
> new file mode 100644
> index 0000000..ee2bc98
> --- /dev/null
> +++ b/wrap-for-bin.sh
> @@ -0,0 +1,15 @@
> +#!/bin/sh
> +
> +# wrap-for-bin.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 bin-wrappers, 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"
> +PATH="__GIT_EXEC_PATH__/bin-wrappers:$PATH"
> +export GIT_EXEC_PATH GIT_TEMPLATE_DIR GITPERLLIB PATH
> +
> +exec "${GIT_EXEC_PATH}/__PROG__" "$@"

Two issues, one minor and one not so minor but not grave:

 - Everywhere else we seem to use "@@UPPERCASE_NAME@@" not
   double-underscore as placeholders like the above.

 - @@PROG@@ is under our control and it is easy for us to guarantee that
   it won't have any funny letters, but GIT_EXEC_PATH is not.  Is it safe
   to do a simple-minded "sed" replacement, or does it need the usual sq
   trick employed in the other replacement in our Makefile?

Re: [PATCH v2 4/6] build dashless "bin-wrappers" directory similar to installed bindir

[Matthew Ogilvie]

On Sun, Nov 29, 2009 at 10:28:46PM -0800, Junio C Hamano wrote:
> Matthew Ogilvie <mmogilvi_git@miniinfo.net> writes:
> > +GIT_EXEC_PATH="__GIT_EXEC_PATH__"
> > +exec "${GIT_EXEC_PATH}/__PROG__" "$@"
> 
> Two issues, one minor and one not so minor but not grave:
> 
>  - Everywhere else we seem to use "@@UPPERCASE_NAME@@" not
>    double-underscore as placeholders like the above.
> 
>  - @@PROG@@ is under our control and it is easy for us to guarantee that
>    it won't have any funny letters, but GIT_EXEC_PATH is not.  Is it safe
>    to do a simple-minded "sed" replacement, or does it need the usual sq
>    trick employed in the other replacement in our Makefile?

You've already applied the v2 patch to pu, but if it is still
a concern, you might either add or squash in the patch at
the bottom of this message for the @@ vs __ issue.  But first,
some thoughts about escaping $(shell pwd):

-------------

The patch below also switches to using single quotes in the wrapper
script, but that by itself doesn't fix everything.  Basically it
fixes '"', '$', (and maybe/partially '\\') at the expense of
breaking '\''.  Several characters are still
broken: '\'', '|', '&', '\\', and maybe others.  Either option
should handle ' ' (space) OK.

The "standard" *_SQ replacement used elsewhere in the makefile
is inadequate in this context, since we really needs to escape
makefile, shell, sed, and then shell again.  It might be possible
to define something like the following, and then use it instead
of $(shell pwd).  You could also try several nested
$(subst ...)'s instead of sed.  I have not tried either of these.

build_dir_MQ=$(shell pwd | sed -e "s/'/'\"'\"'/g" \
                               -e 's/[|&\\]/\\&/g' \
                               -e "s/'/'\"'\"'/g" )

Rationale:

Working backwards, the last -e makes sure the sed argument doesn't
get split or combined by the shell before it gets to sed.  The
middle -e is intended to escape characters that sed would
misinterpret.  The first -e is so that single quotes get
properly interpreted by the shell that interprets
bin-wrappers/* generated scripts.

But I don't really think this is worth it:

1. I think there are parts of test-lib.sh (and maybe other places)
that don't even handle spaces in top-level $(pwd), let alone more
complicated characters.  (git itself should be robust, though)

2. Technically, stuff like SHELL_PATH_SQ (anything that is used
by the makefile as the replacement text for a sed 's' command) may
benefit from something similar.  (But other uses like -DVAR='$(xx_SQ)'
are probably perfect already.)

3. It doesn't seem like a severe restriction to require that users
build git in a directory with a reasonably normal full path name.

-------------

From: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
Date: Mon, 30 Nov 2009 22:57:40 -0700
Subject: [PATCH] wrap-for-bin.sh: Use @@var@@ for Makefile variable expansion

We use @@var@@-style variable expansion in most other places, so be
consistent with them.

Also, rename it to @@BUILD_DIR@@ to clarify what it is expanded with,
and use single quotes in wrap-for-bin.sh to reduce the number of shell
meta characters that might mess it up.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 Makefile        |    4 ++--
 wrap-for-bin.sh |   12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index c8f0c5f..811db45 100644
--- a/Makefile
+++ b/Makefile
@@ -1791,8 +1791,8 @@ all:: $(TEST_PROGRAMS) $(test_bindir_programs)
 bin-wrappers/%: wrap-for-bin.sh
 	@mkdir -p bin-wrappers
 	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-	     -e 's|__GIT_EXEC_PATH__|$(shell pwd)|' \
-	     -e 's|__PROG__|$(@F)|' < $< > $@ && \
+	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
+	     -e 's|@@PROG@@|$(@F)|' < $< > $@ && \
 	chmod +x $@
 
 # GNU make supports exporting all variables by "export" without parameters.
diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh
index ee2bc98..c5075c9 100644
--- a/wrap-for-bin.sh
+++ b/wrap-for-bin.sh
@@ -4,12 +4,12 @@
 # to run test suite against sandbox, but with only bindir-installed
 # executables in PATH.  The Makefile copies this into various
 # files in bin-wrappers, substituting
-# __GIT_EXEC_PATH__ and __PROG__.
+# @@BUILD_DIR@@ and @@PROG@@.
 
-GIT_EXEC_PATH="__GIT_EXEC_PATH__"
-GIT_TEMPLATE_DIR="__GIT_EXEC_PATH__/templates/blt"
-GITPERLLIB="__GIT_EXEC_PATH__/perl/blib/lib"
-PATH="__GIT_EXEC_PATH__/bin-wrappers:$PATH"
+GIT_EXEC_PATH='@@BUILD_DIR@@'
+GIT_TEMPLATE_DIR='@@BUILD_DIR@@/templates/blt'
+GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib'
+PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
 export GIT_EXEC_PATH GIT_TEMPLATE_DIR GITPERLLIB PATH
 
-exec "${GIT_EXEC_PATH}/__PROG__" "$@"
+exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
-- 
1.6.4.GIT

Re: [PATCH v2 0/6] Run test suite without dashed commands in PATH

[Junio C Hamano]

I added t9150 to your second patch and pushed the result out on 'pu'.