From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Mon, 09 May 2011 12:33:45 GMT
Subject: Re: [PATCH] t3703, t4208: add test cases for magic pathspec
Message-ID: <BANLkTikONPUVvAEw+fSDva8RUp-YSm-Nxw@mail.gmail.com>
In-Reply-To: <7v7ha1t6o9.fsf@alter.siamese.dyndns.org>

2011/5/9 Junio C Hamano <gitster@pobox.com>:
>> +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 ;-)?

failed tests are (annoying enough) to be fixed, right? :)

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

It's gray area. Yes I'd rather see that behavior, but then a lot of
code needs to be audited. Perhaps we should delay introducing ":"
until get_pathspec() learns to modify argc. IOW die() for now when
users write ":".

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

My opinion is in the next test. It should show exactly what users type
in, ie. "pathspec ':/non-existent' did not match any files". "'non-ex'
from root" is nice when there's only one magic. If users specify a few
more in one pathspec, the text may become too verbose.

>> +test_expect_failure 'show pathspecs exactly what are typed in' '
>> +     test_cmp expected error
>> +'
>
> Will this break under gettext-poison?

Never really known what that poison is. Will grep ":/non-existent" instead.

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

Yes. Error from the latter "fatal: Path '' does not exist (neither on
disk nor in the index)." makes me wonder, why does log accept a sha1
syntax that may resolve to non-commit object?

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

A bit busy these days. If you're interested, go ahead.
-- 
Duy