On Tue, Jan 20, 2009 at 04:04:28PM +0100, Johannes Schindelin wrote: > +else > + # override all git executables in PATH and TEST_DIRECTORY/.. > + GIT_VALGRIND=$TEST_DIRECTORY/valgrind > + mkdir -p "$GIT_VALGRIND" Isn't this mkdir unnecessary, since it is actually part of the repository (i.e., there is a gitignore there already). However, I think it makes more sense to put the symlink cruft into "$GIT_VALGRIND/bin". That way you can clean up the cruft very easily. In which case you do need to "mkdir" that directory. > + OLDIFS=$IFS > + IFS=: > + for path in $PATH:$TEST_DIRECTORY/.. > + do > + ls "$TEST_DIRECTORY"/../git "$path"/git-* 2> /dev/null | Why aren't these both "$path"/ ? But more importantly, do we really need to bother overriding the whole $PATH? In theory, we aren't calling anything git-* that isn't in "$TEST_DIRECTORY/..". And while it might be nice to catch it if we do, it seems like detecting that is totally orthogonal to running valgrind, and we get different behavior from valgrind versus not. And I think the two should be as similar as possible (with the obvious except of actually, you know, running valgrind). > + base=$(basename "$file") > + test ! -h "$GIT_VALGRIND"/"$base" || continue > + > + if test "#!" = "$(head -c 2 < "$file")" > + then > + # do not override scripts > + ln -s ../../"$base" "$GIT_VALGRIND"/"$base" > + else > + ln -s valgrind.sh "$GIT_VALGRIND"/"$base" > + fi It would be nice to actually detect errors. But you have to differentiate between EEXIST and other errors, which is a pain. And you can't use "ln -sf" because it isn't atomic. Copying would solve that (provided you copied to a tempfile and did an atomic rename). Or writing this snippet as a C helper. > --- /dev/null > +++ b/t/valgrind/valgrind.sh > @@ -0,0 +1,12 @@ > +#!/bin/sh > + > +base=$(basename "$0") > + > +exec valgrind -q --error-exitcode=126 \ > + --leak-check=no \ > + --suppressions="$GIT_VALGRIND/default.supp" \ > + --gen-suppressions=all \ > + --log-fd=4 \ > + --input-fd=4 \ > + $GIT_VALGRIND_OPTIONS \ > + "$GIT_VALGRIND"/../../"$base" "$@" Hm. My version had to do some magic with the GIT_EXEC_PATH, but I think that is because I didn't set GIT_EXEC_PATH in the first place. If yours works (and I haven't really tested it -- I remember it being a real pain in the butt to make sure valgrind was getting called from every code path), then I like your approach much better. -Peff -- 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
