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

From: Jeff King <peff@peff.net>
Date: Nov 14, 2018, 12:52 UTC
Message-ID: <20181114125206.GA32308@sigill.intra.peff.net>
In-Reply-To: <316e215e54b921330b91c763255eb25f475a64ae.1542030510.git.gitgitgadget@gmail.com>

On Mon, Nov 12, 2018 at 05:48:37AM -0800, Johannes Schindelin via GitGitGadget wrote:

Show 13 quoted lines

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

-Peff