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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 21, 2018, 09:41 UTC
Message-ID: <nycvar.QRO.7.76.6.1805211126140.77@tvgsbejvaqbjf.bet>
In-Reply-To: <20180521055143.14701-1-chriscool@tuxfamily.org>

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>

Show 12 quoted lines

> 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 ;-)

Show 15 quoted lines

> 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...

Show 10 quoted lines

> 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.

Show 14 quoted lines

> 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