From: Johannes Sixt <j.sixt@viscovery.net>
Date: Thu, 25 Mar 2010 07:38:09 GMT
Subject: Re: [PATCHv2 2/2] tests for git alternate command
Message-ID: <4BAB12E1.8070602@viscovery.net>
In-Reply-To: <1269497251-13103-3-git-send-email-judge.packham@gmail.com>

Am 3/25/2010 7:07, schrieb Chris Packham:
> Signed-off-by: Chris Packham <judge.packham@gmail.com>

Your patches violate whitespace rules. Use 'git show --check' to see the
questionable lines.

> ---
> I wasn't sure about the test numbering so I just grabbed the highest one. Still
> need to add tests for the deletion use case.

According to t/README, t1* would be a suitable category, perhaps t1430.

>  t/t9800-git-alternate.sh |   95 ++++++++++++++++++++++++++++++++++++++++++++++

"git" in the name is redundant

> +test_expect_success \
> +	'Setup for rest of the test' '

The modern style for test headers is

test_expect_success 'Setup for rest of the test' '
	test goes here
'

It may improve readability if you insert a blank line after the headline.

> +	mkdir -p base &&
> +	cd base &&
> +	git init &&
> +	echo test > a.txt &&
> +	echo test > b.txt &&
> +	echo test > c.txt &&
> +	git add *.txt &&
> +	git commit -a -m "Initial Commit" &&
> +	cd .. &&

Do not use 'cd dir && ... && cd ..', use (cd dir && ...) like you did in
the rest of the tests.

> +test_expect_success \
> +	'Add alternate after clone' '
> +	(cd B &&
> +	git alternate -a ../base/.git/objects
> +	)

We saw tests like this written with more whitespace like:

	(
		cd B &&
		git alternate -a ../base/.git/objects
	)

> +test_expect_success \
> +	'add same alternate fails adding existing abs path' '
> +	(cd B &&
> +	test_must_fail git alternate -a $PWD/base/.git/objects

This use of $PWD is OK, but for consistency it should be $(pwd) like
below. Moreover, it needs double-quotes.

> +test_expect_success \
> +	'test git alternate display' '
> +	testbase=$PWD

You must write this as (d-quotes not needed here)

	testbase=$(pwd) &&

for the benefit of Windows. The difference is that $PWD returns /c/path,
but $(pwd) returns c:/path.

When the alternate was set up using the command, git has only ever seen a
c:/path style path (regardless of whether you used $PWD or $(pwd), because
the path is converted to c:/path by the shell before it invokes git), and
therefore the alternates file contains this style.

But when the expected result is constructed, the /c/path style is *not*
converted to c:/path; and a mismatch would be detected.

> +	(cd B &&
> +	git alternate >actual &&
> +	{
> +		echo "Object store $testbase/base/.git/objects"
> +		echo "    referenced via $testbase/B/.git"
> +	} >expect &&
> +	test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success \
> +	'test git alternate recursive display' '
> +	testbase=$PWD

Ditto.

> +#rm -rf A B C D base
> \ No newline at end of file

test_done

-- Hannes