Hi, On Tue, 20 Jan 2009, Jeff King wrote: > 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. Hmm. I actually liked the hierarchy to be shallow, but I could be convinced... > > + 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"/ ? Yeah. Makes it more readable, doesn't it? > 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). Actually, the two _are_ orthogonal from the technical viewpoint. But with the infrastructure we have in place, it was already very easy to make sure that calls to a Git program we no longer ship are caught. I vividly remember such a bug costing me 3 hours of my life, and a few hairs. So I think "as it's already _that_ easy, we should catch them bugs, too". Needs some documentation though, I agree. > > + 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. I really would not care all that much about that. 'GIT_TEST_OPTS==--valgrind make test' should be run by experts. And even if it is a dummy driving the test, the next "make" call should take care of that. > Copying would solve that (provided you copied to a tempfile and did > an atomic rename). Or writing this snippet as a C helper. Nah, that is really too much work for such a rare thing. Think about it. The symlinks are set up once. And even if you do that with -j50, there is hardly a chance that two processes conflict with each other, and even if they do, they do the same thing. No, what I really want to fix is a script being replaced by a binary. > > --- /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. I set GIT_EXEC_PATH... to $GIT_VALGRIND. 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
