Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Jeff King <peff@peff.net>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git
Date: Wed, 14 Nov 2018 14:41:48 +0100 (STD)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1811141441030.39@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20181114125206.GA32308@sigill.intra.peff.net>

Hi Peff,

On Wed, 14 Nov 2018, Jeff King wrote:

> On Mon, Nov 12, 2018 at 05:48:37AM -0800, Johannes Schindelin via GitGitGadget wrote:
> 
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 832ede5099..1ea20dc2dc 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -51,7 +51,7 @@ export LSAN_OPTIONS
> >  
> >  ################################################################
> >  # It appears that people try to run tests without building...
> > -"$GIT_BUILD_DIR/git" >/dev/null
> > +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
> >  if test $? != 1
> >  then
> >  	echo >&2 'error: you do not seem to have built git yet.'
> 
> The original runs the command independently and then checks $?. Your
> replacement chains the ||. I think it works, because the only case that
> is different is if running git returns 0 (in which case we currently
> complain, but the new code would quietly accept this).
> 
> That should never happen, but if it does we'd probably want to complain.
> And it's pretty subtle all around.  Maybe this would be a bit clearer:
> 
>   if test -n "$GIT_TEST_INSTALLED"
>   then
> 	: assume installed git is OK
>   else
> 	"$GIT_BUILD_DIR/git" >/dev/null
> 	if test $? != 1
> 	then
> 		... die ...
> 	fi
>   fi
> 
> Though arguably we should be checking that there is a git in our path in
> the first instance. So maybe:
> 
>   if test -n "$GIT_TEST_INSTALLED"
> 	"$GIT_TEST_INSTALLED/git" >/dev/null
>   else
> 	"$GIT_BUILD_DIR/git" >/dev/null
>   fi
>   if test $? != 1 ...

You're right. I did it using `"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git"`
and I also adjust the error message now.

Will be fixed in the next iteration,
Dscho

> 
> -Peff
> 