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
>
