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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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