Hi,
On Wed, 21 Jan 2009, Jeff King wrote:
> 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
Except that A had to check the link first, and it was out-of-date already
-- except if you changed a script into a builtin _and_ run make while a
valgrinded test is called _and_ you're unlucky.
> > +--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.
Okay.
> > + # 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).
Okay.
> > + case "$base" in
> > + *.sh|*.perl)
> > + symlink_target=../unprocessed-script
> > + esac
>
> AFAIK, this triggers an error if I try to call "git-foo.perl" directly.
Yep.
> What does this have to do with valgrind?
Nothing, except that the infrastructure is there now.
> Why does this error checking happen when I run --valgrind, but _not_
> otherwise?
Because we can only check for that kind of mistake in our scripts (which
the author would not realize is a mistake when running on a system where
GIT_SHELL=/bin/sh) when we redirect GIT_EXEC_PATH.
So basically, it would take a tremendous effort otherwise, but here, it is
just easy.
> 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.
Hmm. Maybe.
> > + # 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.
As I mentioned earlier: let's bridge this bridge when we face it
(probably it involves making a test-readlink).
Or are you insisting that the patch should be reworked _now_ so that
GIT_EXEC_PATH _always_ points somewhere else?
I hope not, because then you break Windows.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
