Re: [PATCH] Support pathspec magic :(exclude) and its short form :-

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 21, 2013, 18:43 UTC
Message-ID: <xmqqpppt4mur.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACsJy8AeL+EVZme3BPocXDfRqPpqKDA8nCuwx3buiS66L7G4fA@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 8 quoted lines

> Btw I'm thinking of extending pathspec magic syntax a bit to allow
> path completion. Right now the user has to write
>
> git log -- :-Documentation
>
> which does not play well with path completion. I'm thinking of accepting
>
> git log -- :- Documentation

Please don't. That does not help our users, but actively harm them. They have to stop and wonder why a single pathspec is spelled as two tokens on the command line of some other people.

Doing that stupidity only to help those who polish the tool (namely, "bash completion") to be lazy is doubly wrong (in the meantime, the users can type your second variant and then edit the result).

For the same reason why I do not think rewriting

	echo "hello, world!"

to

	echo "hello, world-"

only to work around a pitfall of a particular tool (namely "bash") makes any sense, I do not think it makes sense to make _our_ tool inconsistent by using "!excluded" in the files (and --exclude) and "-not this pattern" only here.

Show 7 quoted lines

>>> +     if (nr_exclude == n)
>>> +             die(_("There is nothing to exclude from by :(exclude) patterns.\n"
>>> +                   "Perhaps you forgot to add either ':/' or '.' ?"));
>>
>> ;-).
>
> Hey it was originally not there,...

I am not objecting. I noticed it and was commending on it as "a nice touch" ;-)

Show 22 quoted lines

>>> +     /*
>>> +      *   #  | positive | negative | result
>>> +      * -----+----------+----------+-------
>>> +      * 1..4 |   -1     |    *     |  -1
>>> +      * 5..8 |    0     |    *     |   0
>>> +      *   9  |    1     |   -1     |   1
>>> +      *  10  |    1     |    0     |   1
>>> +      *  11  |    1     |    1     |   0
>>> +      *  12  |    1     |    2     |   0
>>> +      *  13  |    2     |   -1     |   2
>>> +      *  14  |    2     |    0     |   2
>>> +      *  15  |    2     |    1     |   0
>>> +      *  16  |    2     |    2     |  -1
>>> +      */
>>
>> Not sure what this case-table means...
>
> Sorry, because tree_entry_interesting_1() returns more than "match
> or not", we need to combine the result from positive pathspec with
> the negative one to correctly handle all_not_interesting and
> all_interesting. This table sums it up. I'll add more explanation
> in the next patch.

I managed to have guessed what the three columns on the right meant; I was wondering about the meaning of the "#" column and where it is defined/explained.