Re: [PATCH 1/2 v2] Add valgrind support in test scripts

[Jeff King <peff@peff.net>]

On Wed, Jan 21, 2009 at 02:10:17AM +0100, Johannes Schindelin wrote:

> 	- symbolic links are inspected for correct targets now, and if they
> 	  point somewhere else than expected, they are removed (this can
> 	  error out if the file could not be removed) and recreated.

Now you _do_ have a race on this, and triggering it will cause you to
run a random version of git from your PATH, not using valgrind (instead
of running the version from the repo using valgrind). Something like:

  A: execvp("git-foo")
  B: oops, "git-foo" is out of date
  B: rm $GIT_VALGRIND/git-foo
  A: look for $GIT_VALGRIND/git-foo; not there
  A: look for $PATH[1]/git-foo; ok, there it is
  B: ln -s ../../git-valgrind $GIT_VALGRIND/git-foo

> +--valgrind::
> +	Execute all Git binaries with valgrind and stop on errors (the
> +	exit code will be 126).

It doesn't necessarily stop: it just causes the command to fail, which
causes the test to fail. Which _will_ stop if you have "-i".

Also, you might want to mention that valgrind errors go to stderr, so
using "-v" is helpful.

> +	# override all git executables in PATH and TEST_DIRECTORY/..
> +	GIT_VALGRIND=$TEST_DIRECTORY/valgrind/bin

I think you should leave GIT_VALGRIND pointing to the main valgrind
directory. That way it is more convenient for people using
GIT_VALGRIND_OPTIONS to make use of GIT_VALGRIND without having to ".."
everything (for example, they may want to pick and choose suppressions
to load for their platform).

> +			case "$base" in
> +			*.sh|*.perl)
> +				symlink_target=../unprocessed-script
> +			esac

AFAIK, this triggers an error if I try to call "git-foo.perl" directly.
What does this have to do with valgrind? Why does this error checking
happen when I run --valgrind, but _not_ otherwise?

And yes, I know the answer is "because it's easy to do here, since
--valgrind is munging the PATH anyway". But my point is that that is an
_implementation_ detail, and the external behavior to a user is
nonsensical.

The fact that there are other uses for munging the PATH than valgrind
implies to me that we should _always_ be munging the PATH like this to
catch these sorts of errors. And then "--valgrind" can just change the
way we munge.

> +			# create the link, or replace it if it is out of date
> +			if test ! -h "$GIT_VALGRIND"/"$base" ||
> +			    test "$symlink_target" != \
> +					"$(readlink "$GIT_VALGRIND"/"$base")"
> +			then

readlink is not portable; it's part of GNU coreutils. Right now valgrind
basically only runs on Linux, which I think generally means that
readlink will be available (though I have no idea if there are
distributions that vary in this). However, there is an experimental
valgrind port to FreeBSD and NetBSD, which are unlikely to have
readlink.

-Peff