[PATCH 0/5] tests: various improvements to the GIT_TEST_INSTALLED feature

[PATCH 0/5] tests: various improvements to the GIT_TEST_INSTALLED feature

[Johannes Schindelin via GitGitGadget]

By setting the GIT_TEST_INSTALLED variable to the path of an installed Git
executable, it is possible to run the test suite also on a specific
installed version (as opposed to a version built from scratch).

The only thing this needs that is unlikely to be installed is the test
helper(s).

However, there have been a few rough edges around that, identified in my
(still ongoing) work to support building Git in Visual Studio (where we do
not want to run GNU Make, and where we have no canonical way to create, say,
hard-linked copies of the built-in commands, and other work to let Git for
Windows play better with BusyBox.

Triggered by a comment of AEvar
[https://public-inbox.org/git/20181102223743.4331-1-avarab@gmail.com/], I
hereby contribute these assorted fixes for the GIT_TEST_INSTALLED feature.

Johannes Schindelin (5):
  tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/
  tests: respect GIT_TEST_INSTALLED when initializing repositories
  t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set
  tests: do not require Git to be built when testing an installed Git
  tests: explicitly use `git.exe` on Windows

 Makefile                |  1 +
 t/lib-gettext.sh        |  7 ++++++-
 t/test-lib-functions.sh |  3 ++-
 t/test-lib.sh           | 15 ++++++++++-----
 4 files changed, 19 insertions(+), 7 deletions(-)


base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-73%2Fdscho%2Ftest-git-installed-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-73/dscho/test-git-installed-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/73
-- 
gitgitgadget

[PATCH 1/5] tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We really need to be able to find the test helpers... Really. This
change was forgotten when we moved the test helpers into t/helper/

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 47a99aa0ed..832ede5099 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -957,7 +957,7 @@ 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:$GIT_BUILD_DIR:$PATH
+	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
 	GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
 else # normal case, use ../bin-wrappers only unless $with_dashes:
 	git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"
-- 
gitgitgadget

[PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It really makes very, very little sense to use a different git
executable than the one the caller indicated via setting the environment
variable GIT_TEST_INSTALLED.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib-functions.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 78d8c3783b..801cc9b2ef 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -900,7 +900,8 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
 	) || exit
-- 
gitgitgadget

[PATCH 3/5] t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It makes very, very little sense to test the built git-sh-i18n when the
user asked specifically to test another one.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gettext.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh
index eec757f104..9eb160c997 100644
--- a/t/lib-gettext.sh
+++ b/t/lib-gettext.sh
@@ -10,7 +10,12 @@ GIT_TEXTDOMAINDIR="$GIT_BUILD_DIR/po/build/locale"
 GIT_PO_PATH="$GIT_BUILD_DIR/po"
 export GIT_TEXTDOMAINDIR GIT_PO_PATH
 
-. "$GIT_BUILD_DIR"/git-sh-i18n
+if test -n "$GIT_TEST_INSTALLED"
+then
+	. "$(git --exec-path)"/git-sh-i18n
+else
+	. "$GIT_BUILD_DIR"/git-sh-i18n
+fi
 
 if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON
 then
-- 
gitgitgadget

[PATCH 4/5] tests: do not require Git to be built when testing an installed Git

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We really only need the test helpers in that case, but that is not what
we test for. So let's skip the test for now when we know that we want to
test an installed Git.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 832ede5099..1ea20dc2dc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -51,7 +51,7 @@ export LSAN_OPTIONS
 
 ################################################################
 # It appears that people try to run tests without building...
-"$GIT_BUILD_DIR/git" >/dev/null
+test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
 if test $? != 1
 then
 	echo >&2 'error: you do not seem to have built git yet.'
-- 
gitgitgadget

[PATCH 5/5] tests: explicitly use `git.exe` on Windows

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the bin-wrappers/* scripts, we already take pains to use `git.exe`
rather than `git`, as this could pick up the wrong thing on Windows
(i.e. if there exists a `git` file or directory in the build directory).

Now we do the same in the tests' start-up code.

This also helps when testing an installed Git, as there might be even
more likely some stray file or directory in the way.

Note: the only way we can record whether the `.exe` suffix is by writing
it to the `GIT-BUILD-OPTIONS` file and sourcing it at the beginning of
`t/test-lib.sh`. This is not a requirement introduced by this patch, but
we move the call to be able to use the `$X` variable that holds the file
extension, if any.

Note also: the many, many calls to `git this` and `git that` are
unaffected, as the regular PATH search will find the `.exe` files on
Windows (and not be confused by a directory of the name `git` that is
in one of the directories listed in the `PATH` variable), while
`/path/to/git` would not, per se, know that it is looking for an
executable and happily prefer such a directory.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile                |  1 +
 t/test-lib-functions.sh |  2 +-
 t/test-lib.sh           | 13 +++++++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index bbfbb4292d..5df0118ce9 100644
--- a/Makefile
+++ b/Makefile
@@ -2590,6 +2590,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+	@echo X=\'$(X)\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 801cc9b2ef..c167b2e1af 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -900,7 +900,7 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \
 			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1ea20dc2dc..3e2a9ce76d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -49,18 +49,23 @@ export ASAN_OPTIONS
 : ${LSAN_OPTIONS=abort_on_error=1}
 export LSAN_OPTIONS
 
+if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+then
+	echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
+	exit 1
+fi
+. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+export PERL_PATH SHELL_PATH
+
 ################################################################
 # It appears that people try to run tests without building...
-test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
+test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null ||
 if test $? != 1
 then
 	echo >&2 'error: you do not seem to have built git yet.'
 	exit 1
 fi
 
-. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-export PERL_PATH SHELL_PATH
-
 # if --tee was passed, write the output not only to the terminal, but
 # additionally to the file test-results/$BASENAME.out, too.
 case "$GIT_TEST_TEE_STARTED, $* " in
-- 
gitgitgadget

Re: [PATCH 1/5] tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> We really need to be able to find the test helpers... Really. This
> change was forgotten when we moved the test helpers into t/helper/

Yeah, it is unfortunate that more and more things have internal
whitebox test that can only be checked with test-helper binaries,
which makes it impossible to fully testing installed git, but that
is not a new issue this fix introduces.  This at least allows us to
keep using the test-helpers from freshly built Git while using the
installed version of Git for most of the tests.

Thanks.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/test-lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 47a99aa0ed..832ede5099 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -957,7 +957,7 @@ 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:$GIT_BUILD_DIR:$PATH
> +	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
>  	GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
>  else # normal case, use ../bin-wrappers only unless $with_dashes:
>  	git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It really makes very, very little sense to use a different git
> executable than the one the caller indicated via setting the environment
> variable GIT_TEST_INSTALLED.

Makes perfect sense.  Shouldn't we be asking where the template
directory of the installed version is and using it instead of the
freshly built one, no?

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/test-lib-functions.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 78d8c3783b..801cc9b2ef 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -900,7 +900,8 @@ test_create_repo () {
>  	mkdir -p "$repo"
>  	(
>  		cd "$repo" || error "Cannot setup test environment"
> -		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> +		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
> +			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
>  		error "cannot run git init -- have you built things yet?"
>  		mv .git/hooks .git/hooks-disabled
>  	) || exit

Re: [PATCH 3/5] t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It makes very, very little sense to test the built git-sh-i18n when the
> user asked specifically to test another one.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Yup.  Makes sense.

>  t/lib-gettext.sh | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh
> index eec757f104..9eb160c997 100644
> --- a/t/lib-gettext.sh
> +++ b/t/lib-gettext.sh
> @@ -10,7 +10,12 @@ GIT_TEXTDOMAINDIR="$GIT_BUILD_DIR/po/build/locale"
>  GIT_PO_PATH="$GIT_BUILD_DIR/po"
>  export GIT_TEXTDOMAINDIR GIT_PO_PATH
>  
> -. "$GIT_BUILD_DIR"/git-sh-i18n
> +if test -n "$GIT_TEST_INSTALLED"
> +then
> +	. "$(git --exec-path)"/git-sh-i18n
> +else
> +	. "$GIT_BUILD_DIR"/git-sh-i18n
> +fi
>  
>  if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON
>  then

Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> We really only need the test helpers in that case, but that is not what
> we test for. So let's skip the test for now when we know that we want to
> test an installed Git.

True, but...  hopefully we are making sure t/helpers/ has been built
in some other ways, though, right?  I do not offhand see how tests
would work in a pristine checkout with "cd t && sh t0000-*.sh" as
t/test-lib.sh is not running $(MAKE) itself (and the design of the
test-lib.sh, as can be seen in the check this part of it makes, is
to just abort if we cannot test) with this patch (and PATCH 1/5)
under the test-installed mode, though.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/test-lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 832ede5099..1ea20dc2dc 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -51,7 +51,7 @@ export LSAN_OPTIONS
>  
>  ################################################################
>  # It appears that people try to run tests without building...
> -"$GIT_BUILD_DIR/git" >/dev/null
> +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
>  if test $? != 1
>  then
>  	echo >&2 'error: you do not seem to have built git yet.'

Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/Makefile b/Makefile
> index bbfbb4292d..5df0118ce9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2590,6 +2590,7 @@ GIT-BUILD-OPTIONS: FORCE
>  	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
>  	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
>  	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
> +	@echo X=\'$(X)\' >>$@+

Made me wonder if a single letter $(X) is a bit too cute to expose
to the outside world; as a narrowly confined local convention in
this single Makefile, it was perfectly fine.  But it should do for
now.  Its terseness is attractive, and our eyes (I do not speak for
those new to our codebase and build structure) are already used to
seeing $X suffix.  Somebody may later come and complain but I am OK
to rename it to something like $EXE at that time, not now.

>  ifdef TEST_OUTPUT_DIRECTORY
>  	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
>  endif
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 801cc9b2ef..c167b2e1af 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -900,7 +900,7 @@ test_create_repo () {
>  	mkdir -p "$repo"
>  	(
>  		cd "$repo" || error "Cannot setup test environment"
> -		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
> +		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \

Good.

>  			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
>  		error "cannot run git init -- have you built things yet?"
>  		mv .git/hooks .git/hooks-disabled
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1ea20dc2dc..3e2a9ce76d 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -49,18 +49,23 @@ export ASAN_OPTIONS
>  : ${LSAN_OPTIONS=abort_on_error=1}
>  export LSAN_OPTIONS
>  
> +if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> +then
> +	echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
> +	exit 1
> +fi

OK, this tells us that we at least attempted to build git once, even
under test-installed mode, and hopefully people won't run $(MAKE) and
immediately ^C it only to fool us by leaving this file while keeping
git binary and t/helpers/ binary unbuilt.

> +. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> +export PERL_PATH SHELL_PATH
> +
>  ################################################################
>  # It appears that people try to run tests without building...
> -test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
> +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null ||

The latter half of this change is a good one.  Given what the
proposed log message of this patch says

    Note also: the many, many calls to `git this` and `git that` are
    unaffected, as the regular PATH search will find the `.exe` files on
    Windows (and not be confused by a directory of the name `git` that is
    in one of the directories listed in the `PATH` variable), while
    `/path/to/git` would not, per se, know that it is looking for an
    executable and happily prefer such a directory.

which I read as "path-prefixed invocation, i.e. some/path/to/git, is
bad, it must be spelled some/path/to/git.exe", I am surprised that
tests ever worked on Windows without these five patches, as this
line used to read like this:

	"$GIT_BUILD_DIR/git" >/dev/null
	if test $? != 1
	then
		...

Wouldn't "$GIT_BUILD_DIR/git" have failed (and "executable not
found" hopefully won't produce exit code 1) and stopped the test
suite from running even after you built git and not under the
test-installed-git mode?

>  if test $? != 1
>  then
>  	echo >&2 'error: you do not seem to have built git yet.'
>  	exit 1
>  fi
>  
> -. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> -export PERL_PATH SHELL_PATH
> -
>  # if --tee was passed, write the output not only to the terminal, but
>  # additionally to the file test-results/$BASENAME.out, too.
>  case "$GIT_TEST_TEE_STARTED, $* " in

Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git

[Jeff King]

On Mon, Nov 12, 2018 at 05:48:37AM -0800, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 832ede5099..1ea20dc2dc 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -51,7 +51,7 @@ export LSAN_OPTIONS
>  
>  ################################################################
>  # It appears that people try to run tests without building...
> -"$GIT_BUILD_DIR/git" >/dev/null
> +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
>  if test $? != 1
>  then
>  	echo >&2 'error: you do not seem to have built git yet.'

The original runs the command independently and then checks $?. Your
replacement chains the ||. I think it works, because the only case that
is different is if running git returns 0 (in which case we currently
complain, but the new code would quietly accept this).

That should never happen, but if it does we'd probably want to complain.
And it's pretty subtle all around.  Maybe this would be a bit clearer:

  if test -n "$GIT_TEST_INSTALLED"
  then
	: assume installed git is OK
  else
	"$GIT_BUILD_DIR/git" >/dev/null
	if test $? != 1
	then
		... die ...
	fi
  fi

Though arguably we should be checking that there is a git in our path in
the first instance. So maybe:

  if test -n "$GIT_TEST_INSTALLED"
	"$GIT_TEST_INSTALLED/git" >/dev/null
  else
	"$GIT_BUILD_DIR/git" >/dev/null
  fi
  if test $? != 1 ...

-Peff

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

[Johannes Schindelin]

Hi Junio,

On Wed, 14 Nov 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > It really makes very, very little sense to use a different git
> > executable than the one the caller indicated via setting the environment
> > variable GIT_TEST_INSTALLED.
> 
> Makes perfect sense.  Shouldn't we be asking where the template
> directory of the installed version is and using it instead of the
> freshly built one, no?

It would make sense, but we don't know how to get that information, do we?

$ git grep DEFAULT_GIT_TEMPLATE_DIR
Makefile:       -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR
builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
builtin/init-db.c:              template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \

In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the
only user in the code is init-db.c which uses it in copy_templates().

And changing the code *now* to let us query Git where it thinks its
templates should be won't work, as this patch is about using the installed
Git (at whatever pre-compiled version that might be).

Ciao,
Dscho

> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/test-lib-functions.sh | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 78d8c3783b..801cc9b2ef 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -900,7 +900,8 @@ test_create_repo () {
> >  	mkdir -p "$repo"
> >  	(
> >  		cd "$repo" || error "Cannot setup test environment"
> > -		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> > +		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
> > +			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> >  		error "cannot run git init -- have you built things yet?"
> >  		mv .git/hooks .git/hooks-disabled
> >  	) || exit
> 

Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git

[Johannes Schindelin]

Hi Junio,

On Wed, 14 Nov 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > We really only need the test helpers in that case, but that is not what
> > we test for. So let's skip the test for now when we know that we want to
> > test an installed Git.
> 
> True, but...  hopefully we are making sure t/helpers/ has been built
> in some other ways, though, right?

We do it implicitly, in the test cases that use the helpers.

However, t/test-lib.sh does not particularly verify that the test helpers
have been built.

And I think that is a good thing: we do have several test scripts, I would
think, that do not require any test helper to begin with. These scripts
can work pretty well without having to build anything (read: on a machine
where there is not even a toolchain available to build things).

Besides, it is pretty much only t/helper/test-tool these days, and it is
unlikely that anybody has a `test-tool` in their `PATH`. If they do,
they'll find out soon enough iff they test with `GIT_TEST_INSTALLED` and
iff they do not have the test helper(s) in t/helper/ ;-)

Ciao,
Dscho

> I do not offhand see how tests would work in a pristine checkout with
> "cd t && sh t0000-*.sh" as t/test-lib.sh is not running $(MAKE) itself
> (and the design of the test-lib.sh, as can be seen in the check this
> part of it makes, is to just abort if we cannot test) with this patch
> (and PATCH 1/5) under the test-installed mode, though.
> 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/test-lib.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 832ede5099..1ea20dc2dc 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -51,7 +51,7 @@ export LSAN_OPTIONS
> >  
> >  ################################################################
> >  # It appears that people try to run tests without building...
> > -"$GIT_BUILD_DIR/git" >/dev/null
> > +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
> >  if test $? != 1
> >  then
> >  	echo >&2 'error: you do not seem to have built git yet.'
> 

Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows

[Johannes Schindelin]

Hi Junio,

On Wed, 14 Nov 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > diff --git a/Makefile b/Makefile
> > index bbfbb4292d..5df0118ce9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2590,6 +2590,7 @@ GIT-BUILD-OPTIONS: FORCE
> >  	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
> >  	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
> >  	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
> > +	@echo X=\'$(X)\' >>$@+
> 
> Made me wonder if a single letter $(X) is a bit too cute to expose
> to the outside world; as a narrowly confined local convention in
> this single Makefile, it was perfectly fine.  But it should do for
> now.  Its terseness is attractive, and our eyes (I do not speak for
> those new to our codebase and build structure) are already used to
> seeing $X suffix.  Somebody may later come and complain but I am OK
> to rename it to something like $EXE at that time, not now.
> 
> >  ifdef TEST_OUTPUT_DIRECTORY
> >  	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
> >  endif
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 801cc9b2ef..c167b2e1af 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -900,7 +900,7 @@ test_create_repo () {
> >  	mkdir -p "$repo"
> >  	(
> >  		cd "$repo" || error "Cannot setup test environment"
> > -		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
> > +		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \
> 
> Good.
> 
> >  			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> >  		error "cannot run git init -- have you built things yet?"
> >  		mv .git/hooks .git/hooks-disabled
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 1ea20dc2dc..3e2a9ce76d 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -49,18 +49,23 @@ export ASAN_OPTIONS
> >  : ${LSAN_OPTIONS=abort_on_error=1}
> >  export LSAN_OPTIONS
> >  
> > +if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> > +then
> > +	echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
> > +	exit 1
> > +fi
> 
> OK, this tells us that we at least attempted to build git once, even
> under test-installed mode, and hopefully people won't run $(MAKE) and
> immediately ^C it only to fool us by leaving this file while keeping
> git binary and t/helpers/ binary unbuilt.
> 
> > +. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> > +export PERL_PATH SHELL_PATH
> > +
> >  ################################################################
> >  # It appears that people try to run tests without building...
> > -test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
> > +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null ||
> 
> The latter half of this change is a good one.  Given what the
> proposed log message of this patch says
> 
>     Note also: the many, many calls to `git this` and `git that` are
>     unaffected, as the regular PATH search will find the `.exe` files on
>     Windows (and not be confused by a directory of the name `git` that is
>     in one of the directories listed in the `PATH` variable), while
>     `/path/to/git` would not, per se, know that it is looking for an
>     executable and happily prefer such a directory.
> 
> which I read as "path-prefixed invocation, i.e. some/path/to/git, is
> bad, it must be spelled some/path/to/git.exe", I am surprised that
> tests ever worked on Windows without these five patches, as this
> line used to read like this:
> 
> 	"$GIT_BUILD_DIR/git" >/dev/null
> 	if test $? != 1
> 	then
> 		...
> 
> Wouldn't "$GIT_BUILD_DIR/git" have failed (and "executable not
> found" hopefully won't produce exit code 1) and stopped the test
> suite from running even after you built git and not under the
> test-installed-git mode?

"$GIT_BUILD_DIR/git" did not fail until I regularly built with Visual
Studio (and my Visual Studio project generator generates a directory named
"git" to live alongside "git.exe").

And when it failed, I patched Git for Windows. Fast-forward, years later I
managed to contribute the patch we are discussing right now ;-)

So yes, it is primarily a concern when testing Git in specific setups
where a "git" directory can live next to the "git.exe" that we want to
test. Not necessarily a big deal for most developers on Windows.

Ciao,
Dscho

> 
> >  if test $? != 1
> >  then
> >  	echo >&2 'error: you do not seem to have built git yet.'
> >  	exit 1
> >  fi
> >  
> > -. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> > -export PERL_PATH SHELL_PATH
> > -
> >  # if --tee was passed, write the output not only to the terminal, but
> >  # additionally to the file test-results/$BASENAME.out, too.
> >  case "$GIT_TEST_TEE_STARTED, $* " in
> 

Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git

[Johannes Schindelin]

Hi Peff,

On Wed, 14 Nov 2018, Jeff King wrote:

> On Mon, Nov 12, 2018 at 05:48:37AM -0800, Johannes Schindelin via GitGitGadget wrote:
> 
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 832ede5099..1ea20dc2dc 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -51,7 +51,7 @@ export LSAN_OPTIONS
> >  
> >  ################################################################
> >  # It appears that people try to run tests without building...
> > -"$GIT_BUILD_DIR/git" >/dev/null
> > +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
> >  if test $? != 1
> >  then
> >  	echo >&2 'error: you do not seem to have built git yet.'
> 
> The original runs the command independently and then checks $?. Your
> replacement chains the ||. I think it works, because the only case that
> is different is if running git returns 0 (in which case we currently
> complain, but the new code would quietly accept this).
> 
> That should never happen, but if it does we'd probably want to complain.
> And it's pretty subtle all around.  Maybe this would be a bit clearer:
> 
>   if test -n "$GIT_TEST_INSTALLED"
>   then
> 	: assume installed git is OK
>   else
> 	"$GIT_BUILD_DIR/git" >/dev/null
> 	if test $? != 1
> 	then
> 		... die ...
> 	fi
>   fi
> 
> Though arguably we should be checking that there is a git in our path in
> the first instance. So maybe:
> 
>   if test -n "$GIT_TEST_INSTALLED"
> 	"$GIT_TEST_INSTALLED/git" >/dev/null
>   else
> 	"$GIT_BUILD_DIR/git" >/dev/null
>   fi
>   if test $? != 1 ...

You're right. I did it using `"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git"`
and I also adjust the error message now.

Will be fixed in the next iteration,
Dscho

> 
> -Peff
> 

Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> The latter half of this change is a good one.  Given what the
>> proposed log message of this patch says
>> 
>>     Note also: the many, many calls to `git this` and `git that` are
>>     unaffected, as the regular PATH search will find the `.exe` files on
>>     Windows (and not be confused by a directory of the name `git` that is
>>     in one of the directories listed in the `PATH` variable), while
>>     `/path/to/git` would not, per se, know that it is looking for an
>>     executable and happily prefer such a directory.
>> 
>> which I read as "path-prefixed invocation, i.e. some/path/to/git, is
>> bad, it must be spelled some/path/to/git.exe", I am surprised that
>> tests ever worked on Windows without these five patches, as this
>> line used to read like this:
>> 
>> 	"$GIT_BUILD_DIR/git" >/dev/null
>> 	if test $? != 1
>> 	then
>> 		...
>> 
>> Wouldn't "$GIT_BUILD_DIR/git" have failed (and "executable not
>> found" hopefully won't produce exit code 1) and stopped the test
>> suite from running even after you built git and not under the
>> test-installed-git mode?
>
> "$GIT_BUILD_DIR/git" did not fail until I regularly built with Visual
> Studio (and my Visual Studio project generator generates a directory named
> "git" to live alongside "git.exe").
>
> And when it failed, I patched Git for Windows. Fast-forward, years later I
> managed to contribute the patch we are discussing right now ;-)

OK, I still cannot read it out of what you wrote in the proposed log
message without aid, but in essense the crux of the problem is that
invoking "some/path/to/git" finds "some/path/to/git.exe" only when
there is no "some/path/to/git" directory at the same time, and if
that directory exists, tries and fails to execute that directory,
and the change in this patch protects us from that problem.

Did I get it right?  I'd really prefer to see it more clearly
written in the log message so the next person who reads "git log"
do not have to ask the same question to you.

Assuming that I read it correctly, I think this patch as a whole
makes tons of sense as a change to make the invocation more robust.

Thanks.

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It would make sense, but we don't know how to get that information, do we?
> ...
> And changing the code *now* to let us query Git where it thinks its
> templates should be won't work, as this patch is about using the installed
> Git (at whatever pre-compiled version that might be).

It won't work, but we can add something like "git var templatedir"
to help those who want to further improve the test-installed mode
next year, preparing for better future by sowing seeds now.

In the meantime, using the same temlate dir as before is probably
the best we can do.  Two and a half tangential thoughts are:

 - But then, we need to make sure $GIT_BUILD_DIR/templates/blt/
   is populated, if we do rely on them (i.e. we probably want to
   make sure we have built).

 - Yet, once installed, the contents of the templatedir can be
   arbitrarily munged by the end user, so anything that depends on
   what is in the template won't work as a reliable test piece.

 - Among what's in templates/blt/, we explicitly disable hooks at
   the beginning of the test repository creation to ensure no hooks
   interfere what we test by default, but we will get affected by
   what is in info/excludes.  The contents of freshly-built one is
   empty, so it is unlikely that this will cause problems, but if we
   use installed templates, we cannot control what's in there,
   letting the tests get affected to random things the end-user
   happens to have.

So after all, if we were to change anything, it might make better
sense not to install anything from any templatedir.

But of course, that is orthogonal to the test-install mode.  If we
want to make the test more robust by emptying the templates, we
should do that also for the test-freshly-baked mode, too.

[PATCH v2 0/5] tests: various improvements to the GIT_TEST_INSTALLED feature

[Johannes Schindelin via GitGitGadget]

By setting the GIT_TEST_INSTALLED variable to the path of an installed Git
executable, it is possible to run the test suite also on a specific
installed version (as opposed to a version built from scratch).

The only thing this needs that is unlikely to be installed is the test
helper(s).

However, there have been a few rough edges around that, identified in my
(still ongoing) work to support building Git in Visual Studio (where we do
not want to run GNU Make, and where we have no canonical way to create, say,
hard-linked copies of the built-in commands, and other work to let Git for
Windows play better with BusyBox.

Triggered by a comment of AEvar
[https://public-inbox.org/git/20181102223743.4331-1-avarab@gmail.com/], I
hereby contribute these assorted fixes for the GIT_TEST_INSTALLED feature.

Changes since v1:

 * Now we verify in test-lib.sh also in the GIT_TEST_INSTALLED case whether
   the Git executable is working (thanks, Peff!).
 * The commit message of 5/5 was touched up.

Johannes Schindelin (5):
  tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/
  tests: respect GIT_TEST_INSTALLED when initializing repositories
  t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set
  tests: do not require Git to be built when testing an installed Git
  tests: explicitly use `git.exe` on Windows

 Makefile                |  1 +
 t/lib-gettext.sh        |  7 ++++++-
 t/test-lib-functions.sh |  3 ++-
 t/test-lib.sh           | 22 ++++++++++++++++------
 4 files changed, 25 insertions(+), 8 deletions(-)


base-commit: d166e6afe5f257217836ef24a73764eba390c58d
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-73%2Fdscho%2Ftest-git-installed-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-73/dscho/test-git-installed-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/73

Range-diff vs v1:

 1:  2b04f9f086 = 1:  3b68e0fe8a tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/
 2:  948b3dc146 = 2:  80d50d5932 tests: respect GIT_TEST_INSTALLED when initializing repositories
 3:  eddea552e4 = 3:  49e408677a t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set
 4:  316e215e54 < -:  ---------- tests: do not require Git to be built when testing an installed Git
 -:  ---------- > 4:  b801dc8027 tests: do not require Git to be built when testing an installed Git
 5:  cd314e1384 ! 5:  fbdb659de6 tests: explicitly use `git.exe` on Windows
     @@ -2,6 +2,17 @@
      
          tests: explicitly use `git.exe` on Windows
      
     +    On Windows, when we refer to `/an/absolute/path/to/git`, it magically
     +    resolves `git.exe` at that location. Except if something of the name
     +    `git` exists next to that `git.exe`. So if we call `$BUILD_DIR/git`, it
     +    will find `$BUILD_DIR/git.exe` *only* if there is not, say, a directory
     +    called `$BUILD_DIR/git`.
     +
     +    Such a directory, however, exists in Git for Windows when building with
     +    Visual Studio (our Visual Studio project generator defaults to putting
     +    the build files into a directory whose name is the base name of the
     +    corresponding `.exe`).
     +
          In the bin-wrappers/* scripts, we already take pains to use `git.exe`
          rather than `git`, as this could pick up the wrong thing on Windows
          (i.e. if there exists a `git` file or directory in the build directory).
     @@ -68,11 +79,12 @@
      +
       ################################################################
       # It appears that people try to run tests without building...
     --test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
     -+test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null ||
     +-"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null
     ++"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
       if test $? != 1
       then
     - 	echo >&2 'error: you do not seem to have built git yet.'
     + 	if test -n "$GIT_TEST_INSTALLED"
     +@@
       	exit 1
       fi
       

-- 
gitgitgadget

[PATCH v2 1/5] tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We really need to be able to find the test helpers... Really. This
change was forgotten when we moved the test helpers into t/helper/

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index aba66cafa2..93883580a8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -957,7 +957,7 @@ 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:$GIT_BUILD_DIR:$PATH
+	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
 	GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
 else # normal case, use ../bin-wrappers only unless $with_dashes:
 	git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"
-- 
gitgitgadget

[PATCH v2 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It really makes very, very little sense to use a different git
executable than the one the caller indicated via setting the environment
variable GIT_TEST_INSTALLED.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib-functions.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index d158c8d0bf..3472716651 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -923,7 +923,8 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
 	) || exit
-- 
gitgitgadget

[PATCH v2 3/5] t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It makes very, very little sense to test the built git-sh-i18n when the
user asked specifically to test another one.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gettext.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh
index eec757f104..9eb160c997 100644
--- a/t/lib-gettext.sh
+++ b/t/lib-gettext.sh
@@ -10,7 +10,12 @@ GIT_TEXTDOMAINDIR="$GIT_BUILD_DIR/po/build/locale"
 GIT_PO_PATH="$GIT_BUILD_DIR/po"
 export GIT_TEXTDOMAINDIR GIT_PO_PATH
 
-. "$GIT_BUILD_DIR"/git-sh-i18n
+if test -n "$GIT_TEST_INSTALLED"
+then
+	. "$(git --exec-path)"/git-sh-i18n
+else
+	. "$GIT_BUILD_DIR"/git-sh-i18n
+fi
 
 if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON
 then
-- 
gitgitgadget

[PATCH v2 4/5] tests: do not require Git to be built when testing an installed Git

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We really only need the test helpers to be built in the worktree in that
case, but that is not what we test for.

On the other hand it is a perfect opportunity to verify that
`GIT_TEST_INSTALLED` points to a working Git.

So let's test the appropriate Git executable. While at it, also adjust
the error message in the `GIT_TEST_INSTALLED` case.

This patch is best viewed with `-w --patience`.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 93883580a8..3d3a65ed0e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -51,10 +51,15 @@ export LSAN_OPTIONS
 
 ################################################################
 # It appears that people try to run tests without building...
-"$GIT_BUILD_DIR/git" >/dev/null
+"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null
 if test $? != 1
 then
-	echo >&2 'error: you do not seem to have built git yet.'
+	if test -n "$GIT_TEST_INSTALLED"
+	then
+		echo >&2 "error: there is no working Git at '$GIT_TEST_INSTALLED'"
+	else
+		echo >&2 'error: you do not seem to have built git yet.'
+	fi
 	exit 1
 fi
 
-- 
gitgitgadget

[PATCH v2 5/5] tests: explicitly use `git.exe` on Windows

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, when we refer to `/an/absolute/path/to/git`, it magically
resolves `git.exe` at that location. Except if something of the name
`git` exists next to that `git.exe`. So if we call `$BUILD_DIR/git`, it
will find `$BUILD_DIR/git.exe` *only* if there is not, say, a directory
called `$BUILD_DIR/git`.

Such a directory, however, exists in Git for Windows when building with
Visual Studio (our Visual Studio project generator defaults to putting
the build files into a directory whose name is the base name of the
corresponding `.exe`).

In the bin-wrappers/* scripts, we already take pains to use `git.exe`
rather than `git`, as this could pick up the wrong thing on Windows
(i.e. if there exists a `git` file or directory in the build directory).

Now we do the same in the tests' start-up code.

This also helps when testing an installed Git, as there might be even
more likely some stray file or directory in the way.

Note: the only way we can record whether the `.exe` suffix is by writing
it to the `GIT-BUILD-OPTIONS` file and sourcing it at the beginning of
`t/test-lib.sh`. This is not a requirement introduced by this patch, but
we move the call to be able to use the `$X` variable that holds the file
extension, if any.

Note also: the many, many calls to `git this` and `git that` are
unaffected, as the regular PATH search will find the `.exe` files on
Windows (and not be confused by a directory of the name `git` that is
in one of the directories listed in the `PATH` variable), while
`/path/to/git` would not, per se, know that it is looking for an
executable and happily prefer such a directory.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile                |  1 +
 t/test-lib-functions.sh |  2 +-
 t/test-lib.sh           | 13 +++++++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 016fdcdb81..21b3978744 100644
--- a/Makefile
+++ b/Makefile
@@ -2591,6 +2591,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+	@echo X=\'$(X)\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3472716651..274cbc2d6e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -923,7 +923,7 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \
 			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3d3a65ed0e..e12addc324 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -49,9 +49,17 @@ export ASAN_OPTIONS
 : ${LSAN_OPTIONS=abort_on_error=1}
 export LSAN_OPTIONS
 
+if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+then
+	echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
+	exit 1
+fi
+. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+export PERL_PATH SHELL_PATH
+
 ################################################################
 # It appears that people try to run tests without building...
-"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null
+"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
 if test $? != 1
 then
 	if test -n "$GIT_TEST_INSTALLED"
@@ -63,9 +71,6 @@ then
 	exit 1
 fi
 
-. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-export PERL_PATH SHELL_PATH
-
 # if --tee was passed, write the output not only to the terminal, but
 # additionally to the file test-results/$BASENAME.out, too.
 case "$GIT_TEST_TEE_STARTED, $* " in
-- 
gitgitgadget

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

[Jeff King]

On Wed, Nov 14, 2018 at 02:16:37PM +0100, Johannes Schindelin wrote:

> > Makes perfect sense.  Shouldn't we be asking where the template
> > directory of the installed version is and using it instead of the
> > freshly built one, no?
> 
> It would make sense, but we don't know how to get that information, do we?
> 
> $ git grep DEFAULT_GIT_TEMPLATE_DIR
> Makefile:       -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
> builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR
> builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
> builtin/init-db.c:              template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
> contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \
> 
> In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the
> only user in the code is init-db.c which uses it in copy_templates().
> 
> And changing the code *now* to let us query Git where it thinks its
> templates should be won't work, as this patch is about using the installed
> Git (at whatever pre-compiled version that might be).

Do we actually care where the templates are? I thought the point was to
override for the built Git to use the built templates instead of the
installed one. For an installed Git, shouldn't we not be overriding the
templates at all? I.e.:

  if test -n "$GIT_TEST_INSTALLED"
  then
	"$GIT_TEST_INSTALLED/git" init
  else
	"$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt"
  fi >&3 2>&4

(That's all leaving aside the question of whether we ought to be using a
clean template dir instead of this).

-Peff

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

[Johannes Schindelin]

Hi Junio,

On Wed, 14 Nov 2018, Jeff King wrote:

> On Wed, Nov 14, 2018 at 02:16:37PM +0100, Johannes Schindelin wrote:
> 
> > > Makes perfect sense.  Shouldn't we be asking where the template
> > > directory of the installed version is and using it instead of the
> > > freshly built one, no?
> > 
> > It would make sense, but we don't know how to get that information, do we?
> > 
> > $ git grep DEFAULT_GIT_TEMPLATE_DIR
> > Makefile:       -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
> > builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR
> > builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
> > builtin/init-db.c:              template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
> > contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \
> > 
> > In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the
> > only user in the code is init-db.c which uses it in copy_templates().
> > 
> > And changing the code *now* to let us query Git where it thinks its
> > templates should be won't work, as this patch is about using the installed
> > Git (at whatever pre-compiled version that might be).
> 
> Do we actually care where the templates are? I thought the point was to
> override for the built Git to use the built templates instead of the
> installed one. For an installed Git, shouldn't we not be overriding the
> templates at all? I.e.:
> 
>   if test -n "$GIT_TEST_INSTALLED"
>   then
> 	"$GIT_TEST_INSTALLED/git" init
>   else
> 	"$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt"
>   fi >&3 2>&4
> 
> (That's all leaving aside the question of whether we ought to be using a
> clean template dir instead of this).

I fear that that might buy us a ton of trouble. Just like we override the
system config, we should override the templates.

Ciao,
Dscho

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

[Jeff King]

On Thu, Nov 15, 2018 at 01:29:58PM +0100, Johannes Schindelin wrote:

> > Do we actually care where the templates are? I thought the point was to
> > override for the built Git to use the built templates instead of the
> > installed one. For an installed Git, shouldn't we not be overriding the
> > templates at all? I.e.:
> > 
> >   if test -n "$GIT_TEST_INSTALLED"
> >   then
> > 	"$GIT_TEST_INSTALLED/git" init
> >   else
> > 	"$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt"
> >   fi >&3 2>&4
> > 
> > (That's all leaving aside the question of whether we ought to be using a
> > clean template dir instead of this).
> 
> I fear that that might buy us a ton of trouble. Just like we override the
> system config, we should override the templates.

Yes, it might. I guess it just seems plausible to me that somebody would
expect GIT_TEST_INSTALLED to be as close to the installed experience as
possible. I dunno. I do not use it myself.

At any rate, my point was that for GIT_TEST_INSTALLED, either:

  1. We can use a known-clean set of templates (either our local
     templates/blt, or an even-cleaner empty set).

or

  2. We do not need to specify any template, and it will just use
     whatever it came installed with.

And in either case, we do not have to worry about asking it "where are
your templates?".

-Peff