Re: [PATCH 1/2] t6300: test sort with multiple keys

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 20, 2012, 00:38 UTC
Message-ID: <7vk3wuo0sa.fsf@alter.siamese.dyndns.org>
In-Reply-To: <e5b3ab37553f384235f3cb14e42f7e2b56507bde.1345410836.git.draenog@pld-linux.org>

Kacper Kornet <draenog@pld-linux.org> writes:

Show 7 quoted lines

> Documentation of git-for-each-ref says that --sort=<key> option can be
> used multiple times, in which case the last key becomes the primary key.
> However this functionality was never checked in test suite and is
> currently broken. This commit adds appropriate test in preparation for fix.
>
> Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
> ---

Thanks.

Show 11 quoted lines

> +test_expect_success 'Create branches to test sort with multiple keys' '
> +	git checkout -b Branch1 &&
> +	echo foo >> one &&
> +	git commit -a -m "Branch1 commit" &&
> +	git checkout -b Branch2 &&
> +	echo foo >> one &&
> +	git commit -a -m "Branch2 commit"
> +'
> +
> +test_atom refs/heads/Branch1 objectname 32fca05e9f638021a123a84226acf17756acc18b
> +test_atom refs/heads/Branch2 objectname 194a5b89ac661a114566ba4374bc06c2797539f3

Do these need to be "Branch[12]", not "branch[12]" for the code to exhibit the bug? If not, please don't be creative in names like these. On case corrupting filesystems you may write Branch1 and they may come back as branch1, but that is not what we are testing here.

Also, style: redirection sticks to the target file, e.g.

	echo foo >>one &&

Show 43 quoted lines

> +
>  cat >expected <<\EOF
> +67a36f10722846e891fbada1ba48ed035de75581 commit	refs/heads/master
> +194a5b89ac661a114566ba4374bc06c2797539f3 commit	refs/heads/Branch2
> +32fca05e9f638021a123a84226acf17756acc18b commit	refs/heads/Branch1
> +EOF
> +
> +test_expect_failure 'Verify sort with multiple keys' '
> +	git for-each-ref --sort=objectname --sort=committerdate refs/heads > actual &&
> +	test_cmp expected actual
> +'
> +
> +cat >expected <<\EOF
> +'refs/heads/Branch1'
> +'refs/heads/Branch2'
>  'refs/heads/master'
>  'refs/remotes/origin/master'
>  'refs/tags/testtag'
> @@ -264,6 +289,8 @@ test_expect_success 'Quoting style: python' '
>  '
>  
>  cat >expected <<\EOF
> +"refs/heads/Branch1"
> +"refs/heads/Branch2"
>  "refs/heads/master"
>  "refs/remotes/origin/master"
>  "refs/tags/testtag"
> @@ -285,6 +312,8 @@ for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
>  done
>  
>  cat >expected <<\EOF
> +Branch1
> +Branch2
>  master
>  testtag
>  EOF
> @@ -296,6 +325,8 @@ test_expect_success 'Check short refname format' '
>  '
>  
>  cat >expected <<EOF
> +
> +
>  origin/master

What are these blank line outputs?

Show 10 quoted lines

>  EOF
>  
> @@ -309,7 +340,7 @@ cat >expected <<EOF
>  EOF
>  
>  test_expect_success 'Check short objectname format' '
> -	git for-each-ref --format="%(objectname:short)" refs/heads >actual &&
> +	git for-each-ref --format="%(objectname:short)" refs/heads/master >actual &&
>  	test_cmp expected actual
>  '

All in all, I have to wonder if you can limit the updates to other unrelated tests if you added a new test near the end. Also doesn't the existing test already create enough refs to let you sort with multiple keys and demonstrate the breakage already, without adding new refs and objects?