Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows
Johannes Schindelin 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 5/5] tests: explicitly use `git.exe` on Windows
Hi Junio,
On Wed, 14 Nov 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget"
> 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 5/5] tests: explicitly use `git.exe` on Windows
"Johannes Schindelin via GitGitGadget"
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
