Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 10, 2017, 18:50 UTC
Message-ID: <xmqqshnllw2f.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <cc23463af8096c5f8429f939ce881cf0eb5c2dcd.1486740772.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 15 quoted lines

> From: Michael Rappazzo <rappazzo@gmail.com>
>
> t2027-worktree-list has an incorrect expectation for --git-common-dir
> which has been adjusted and marked to expect failure.
>
> Some of the tests added have been marked to expect failure.  These
> demonstrate a problem with the way that some options to git rev-parse
> behave when executed from a subdirectory of the main worktree.
>
> [jes: fixed incorrect assumption that objects/ lives in the
> worktree-specific git-dir (it lives in the common dir instead).]
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Just one iffy part; otherwise looks good.

Show 35 quoted lines

> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 038e24c4014..f39f783f2db 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru
>  
>  test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
>  
> +test_expect_success 'git-common-dir from worktree root' '
> +	echo .git >expect &&
> +	git rev-parse --git-common-dir >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_failure 'git-common-dir inside sub-dir' '
> +	mkdir -p path/to/child &&
> +	test_when_finished "rm -rf path" &&
> +	echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
> +	git -C path/to/child rev-parse --git-common-dir >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git-path from worktree root' '
> +	echo .git/objects >expect &&
> +	git rev-parse --git-path objects >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_failure 'git-path inside sub-dir' '
> +	mkdir -p path/to/child &&
> +	test_when_finished "rm -rf path" &&
> +	echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
> +	git -C path/to/child rev-parse --git-path objects >actual &&
> +	test_cmp expect actual
> +'

All of these look sensible.

Show 24 quoted lines

>  test_done
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 292a0720fcc..1d6e27a09d8 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -200,4 +200,21 @@ EOF
>  	test_cmp expect actual
>  '
>  
> +test_expect_failure 'rev-parse --shared-index-path' '
> +	rm -rf .git &&
> +	test_create_repo . &&
> +	git update-index --split-index &&
> +	ls -t .git/sharedindex* | tail -n 1 >expect &&
> +	git rev-parse --shared-index-path >actual &&
> +	test_cmp expect actual &&
> +	mkdir work &&
> +	test_when_finished "rm -rf work" &&
> +	(
> +		cd work &&
> +		ls -t ../.git/sharedindex* | tail -n 1 >expect &&
> +		git rev-parse --shared-index-path >actual &&
> +		test_cmp expect actual
> +	)

This looks iffy. If we expect multiple sharedindex* files, the first output from "ls -t" may or may not match the real one in use (multiple things do happen within a single second or whatever your filesystem's time granularity is). Two "ls -t" run in this test would (hopefully) give stable results, but I suspect that the chance the first line in the output matches what --shared-index-path reports is 50% if we expect to have two sharedindex* files.

On the other hand, if we do not expect multiple sharedindex* files, use of "ls" piped to "tail" is simply misleading.

If this test can be written in such a way that there is only one such file that match the pattern, it would be the cleanest to understand and explain. As there is only a single invocation of "update-index --split-index" immediately after a new repository is created, I suspect that the expectation to see only one sharedindex* file already holds (because its name is unpredictable, we still need to catch it with wildcard), and if that is the case, we can just lose "-t" and pipe to tail, i.e. "ls .git/sharedindex* >expect".

Show 19 quoted lines

> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 465eeeacd3d..c1a072348e7 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -8,16 +8,24 @@ test_expect_success 'setup' '
>  	test_commit init
>  '
>  
> -test_expect_success 'rev-parse --git-common-dir on main worktree' '
> +test_expect_failure 'rev-parse --git-common-dir on main worktree' '
>  	git rev-parse --git-common-dir >actual &&
>  	echo .git >expected &&
>  	test_cmp expected actual &&
>  	mkdir sub &&
>  	git -C sub rev-parse --git-common-dir >actual2 &&
> -	echo sub/.git >expected2 &&
> +	echo ../.git >expected2 &&
>  	test_cmp expected2 actual2
>  '

OK, this swaps a wrong expectation with a more usable one.

Show 8 quoted lines

> +test_expect_failure 'rev-parse --git-path objects linked worktree' '
> +	echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
> +	test_when_finished "rm -rf linked-tree && git worktree prune" &&
> +	git worktree add --detach linked-tree master &&
> +	git -C linked-tree rev-parse --git-path objects >actual &&
> +	test_cmp expect actual
> +'
> +

Looking good.

Thanks.