Hi Junio,
On Wed, 14 Nov 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> 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
>
