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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 14, 2018, 05:14 UTC
Message-ID: <xmqq5zx0p7ej.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <cd314e1384312cd5b0c0031efd40c6442074e11c.1542030510.git.gitgitgadget@gmail.com>

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

Show 9 quoted lines

> 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.

Show 13 quoted lines

>  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.

Show 16 quoted lines

>  			"--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.

Show 7 quoted lines

> +. "$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?

Show 12 quoted lines

>  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