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

[Junio C Hamano <gitster@pobox.com>]

"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