Re: [PATCH] t3703, t4208: add test cases for magic pathspec

From: Junio C Hamano <gitster@pobox.com>
Date: May 8, 2011, 17:59 UTC
Message-ID: <7v7ha1t6o9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1304852906-29272-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

Show 22 quoted lines

> diff --git a/t/t3703-add-magic-pathspec.sh b/t/t3703-add-magic-pathspec.sh
> new file mode 100755
> index 0000000..3d8c6b8
> --- /dev/null
> +++ b/t/t3703-add-magic-pathspec.sh
> @@ -0,0 +1,78 @@
> +#!/bin/sh
> +
> +test_description='magic pathspec tests using git-add'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	mkdir sub anothersub &&
> +	: >sub/foo &&
> +	: >anothersub/foo
> +'
> +
> +test_expect_failure 'colon alone magic can only used alone' '
> +	test_must_fail git add -n sub/foo : &&
> +	test_must_fail git add -n : sub/foo
> +'

I don't care too much about this case (it is a user error), but if you promise you will turn this expect-failure to expect-success in a follow-up patch, why not ;-)?

Show 11 quoted lines

> +cat >expected <<EOF
> +add 'anothersub/foo'
> +add 'expected'
> +add 'sub/actual'
> +add 'sub/foo'
> +EOF
> +
> +test_expect_success 'add :' '
> +	(cd sub && git add -n : >actual) &&
> +	test_cmp expected sub/actual
> +'

Shouldn't

	$ git anycmd :

be equivalent to

	$ (cd $(git rev-parse --show-cdup)/. && git anycmd)

for any command? I doubt this test is expecting the right outcome. Shouldn't it result in "Nothing specified, nothing added."?

> +test_expect_success 'add :/' '
> +	(cd sub && git add -n :/ >actual) &&
> +	test_cmp expected sub/actual
> +'

This one is expecting the right thing.

> +test_expect_success 'add :/non-existent' '
> +	(cd sub && test_must_fail git add -n :/non-existent)
> +'

Just being curious. What should the error message say? Can we make it to say "fatal: pathspec 'non-ex' from root did not match any files"?

> +cat >expected <<EOF
> +fatal: pathspec ':(icase)ha' did not match any files
> +EOF

When writing a literal, make it a habit to quote EOF to reduce mental load on the reader, like this:

	cat >expected <<\EOF
        ...
        EOF

so that the reader does not have to scan for $var in the text to make sure.

> +test_expect_failure 'show pathspecs exactly what are typed in' '
> +	test_cmp expected error
> +'

Will this break under gettext-poison?

Show 26 quoted lines

> diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
> new file mode 100755
> index 0000000..b296a74
> --- /dev/null
> +++ b/t/t4208-log-magic-pathspec.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +
> +test_description='magic pathspec tests using git-log'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	test_commit initial &&
> +	test_tick &&
> +	git commit --allow-empty -m empty &&
> +	mkdir sub
> +'
> +
> +test_expect_failure 'git log :/ ambiguous with [ref]:/path' '
> +	test_must_fail git log :/ 2>error &&
> +	grep ambiguous error
> +'
> +test_expect_failure 'git log :' '
> +	git log :
> +'

These two should expect exactly the same error, I think. ':', ':/' or anything magic will not satisify verify_filename(), and needs a double-dash before it.

We could improve the disambiguation heuristics so that when we do not have a '--' on the command line:

 - make sure all the earlier ones are refs and they cannot be a path on
   the filesystem (otherwise we need a disambiguator "--").

 - the first non-ref argument and everything that follows must be either a
   ':' magic, a string with globbing character, or a path on the
   filesystem, and none of them can be a ref.

Do you want to take a stab at it?