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

[Junio C Hamano <gitster@pobox.com>]

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

> 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 ;-)?

> +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?

> 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?