Re: [NEW REPLACEMENT PATCH] git-checkout: Add a test case for relative paths use.

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 8, 2007, 14:32 UTC
Message-ID: <Pine.LNX.4.64.0711081427450.4362@racer.site>
In-Reply-To: <11945276321726-git-send-email-dsymonds@gmail.com>

Hi,

just a few nitpicks:

On Fri, 9 Nov 2007, David Symonds wrote:

Show 8 quoted lines

> +test_expect_success setup '
> +
> +	echo base > file0 &&
> +	git add file0 &&
> +	mkdir dir1 &&
> +	echo hello > dir1/file1 &&
> +	git add dir1/file1 &&
> +	test_tick &&

please move the test_tick directly in front of the commit. Readers might assume that it has an effect on mkdir otherwise.

Show 6 quoted lines

> +	mkdir dir2 &&
> +	echo bonjour > dir2/file2 &&
> +	git add dir2/file2 &&
> +	git commit -m "populate tree"
> +
> +'

Please lose the empty line before the closing quote. (This applies to all tests.)

Show 16 quoted lines

> +test_expect_success 'remove and restore with relative path' '
> +
> +	cd dir1 &&
> +	rm ../file0 &&
> +	git checkout HEAD -- ../file0 && test -f ../file0 &&
> +	rm ../dir2/file2 &&
> +	git checkout HEAD -- ../dir2/file2 && test -f ../dir2/file2 &&
> +	rm ../file0 ./file1 &&
> +	git checkout HEAD -- .. && test -f ../file0 && test -f ./file1 &&
> +	rm file1 &&
> +	git checkout HEAD -- ../dir1/../dir1/file1 && test -f ./file1
> +
> +'
> +
> +test_expect_failure 'checkout with relative path outside tree should fail (1)' \
> +	'git checkout HEAD -- ../file0'

Maybe do that with an existing file? Since the test script lives in t/, and the test is run in t/trash/, we can test for "../Makefile".

Also, I would shorten the message to "relative path outside tree should fail".

> +test_expect_failure 'checkout with relative path outside tree should fail (2)' \
> +	'cd dir1 && git checkout HEAD -- ./file0'

I am not convinced that this should fail.

> +test_expect_failure 'checkout with relative path outside tree should fail (2)' \
> +	'cd dir1 && git checkout HEAD -- ../../file0'

Please add some other test like

test_expect_success 'checkout with empty prefix' '
	rm file0 &&
	git checkout HEAD -- file0 &&
	test base = "$(cat file0)"
'

Thanks, Dscho