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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux