Re: [PATCH] t: make many tests depend less on the refs being files

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

[-- Attachment #1: Type: text/plain, Size: 4006 bytes --]

Hi,

of course I messed up and did not fix the Cc: list... now fixed ;-)


On Mon, 21 May 2018, Johannes Schindelin wrote:

> Hi Chris,
> 
> On Mon, 21 May 2018, Christian Couder wrote:
> 
> > From: David Turner <dturner@twopensource.com>
> 
> I vaguely remember that Dave suggested using a different email address
> these days...
> 
> *clicketyclick*
> 
> ah, yes:
> https://public-inbox.org/git/1474935093-26757-3-git-send-email-dturner@twosigma.com/
> 
> So maybe update it here, too, to 
> 
> 	From: David Turner <novalis@novalis.org>
> 
> ?
> 
> > So that they work under alternate ref storage backends.
> > 
> > This will be really needed when such alternate ref storage backends are
> > developed. But this could already help by making clear to readers that
> > some tests do not depend on which ref backend is used.
> > 
> > This patch just takes care of many low hanging fruits. It does not try
> > to completely solves the issue.
> > 
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> > ---
> 
> This is great. I am all for making the tests better ;-)
> 
> > diff --git a/t/lib-t6000.sh b/t/lib-t6000.sh
> > index 3f2d873fec..b8567cdf94 100644
> > --- a/t/lib-t6000.sh
> > +++ b/t/lib-t6000.sh
> > @@ -4,11 +4,10 @@ mkdir -p .git/refs/tags
> >  
> >  >sed.script
> >  
> > -# Answer the sha1 has associated with the tag. The tag must exist in .git/refs/tags
> > +# Answer the sha1 has associated with the tag. The tag must exist under refs/tags
> >  tag () {
> >  	_tag=$1
> > -	test -f ".git/refs/tags/$_tag" || error "tag: \"$_tag\" does not exist"
> > -	cat ".git/refs/tags/$_tag"
> > +	git rev-parse --verify "refs/tags/$_tag" || error "tag: \"$_tag\" does not exist"
> 
> Line longer than 80 columns...
> 
> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> > index 0680dec808..886a9e3b72 100755
> > --- a/t/t5500-fetch-pack.sh
> > +++ b/t/t5500-fetch-pack.sh
> > @@ -30,7 +30,7 @@ add () {
> >  	test_tick &&
> >  	commit=$(echo "$text" | git commit-tree $tree $parents) &&
> >  	eval "$name=$commit; export $name" &&
> > -	echo $commit > .git/refs/heads/$branch &&
> > +	git update-ref refs/heads/$branch $commit &&
> 
> I think we have to be careful here to quote both "refs/heads/$branch" and
> "$commit" here, as a bug that introduces spaces into $commit or $branch
> would have been caught earlier, but not now, unless we quote.
> 
> This goes for all introduced `update-ref` calls.
> 
> Maybe even for some `git rev-parse --verify` calls.
> 
> > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> > index 8f5c811dd7..c3b89ae783 100755
> > --- a/t/t9903-bash-prompt.sh
> > +++ b/t/t9903-bash-prompt.sh
> > @@ -148,7 +148,7 @@ test_expect_success 'prompt - inside .git directory' '
> >  test_expect_success 'prompt - deep inside .git directory' '
> >  	printf " (GIT_DIR!)" >expected &&
> >  	(
> > -		cd .git/refs/heads &&
> > +		cd .git/objects &&
> >  		__git_ps1 >"$actual"
> >  	) &&
> >  	test_cmp expected "$actual"
> > -- 
> 
> This one looks wrong.
> 
> Upon cursory review, one might be tempted to assume that the file is now
> written into .git/objects/ instead of .git/refs/heads/. And the patch
> context provided in the email is not enough to see (gawd, I hate
> mail-based patch review, it really takes all my Git tools away from me).
> The trick is that `actual` points at an absolute path:
> 
> 	#!/bin/sh
> 	#
> 	# Copyright (c) 2012 SZEDER Gábor
> 	#
> 
> 	test_description='test git-specific bash prompt functions'
> 
> 	. ./lib-bash.sh
> 
> 	. "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh"
> 
> 	actual="$TRASH_DIRECTORY/actual"
> 	[...]
> 
> So the remaining question (which probably wants to be added to the commit
> message together with a hint that `actual` points at an absolute path) is:
> Why not `cd .git` instead?
> 
> Ciao,
> Dscho