Re: [PATCH] Support pathspec magic :(exclude) and its short form :-
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Nov 21, 2013, 02:10 UTC
- Message-ID
- <CACsJy8AeL+EVZme3BPocXDfRqPpqKDA8nCuwx3buiS66L7G4fA@mail.gmail.com>
- In-Reply-To
- <xmqqhab663ef.fsf@gitster.dls.corp.google.com>
On Thu, Nov 21, 2013 at 6:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
>> We don't have many options that say "negative" in short form. >> Either '!', '-' or '~'. '!' is already used for bash history expansion. >> ~ looks more like $HOME expansion. Which left me '-'. > > I agree with your decision to reject ~, but "!not-this-pattern" is > very much consistent with the patterns used in .gitignore (and the > "--exclude <pattern>" option), so avoiding "!" and introducing an > inconsistent "-" only to appease bash leaves somewhat a funny taste > in my mouth.
The thing about '!' is it's history expansion in bash and I suspect not many people are aware of it. So "git log -- :!something" may recall the last command that has "something" in it, which is confusing for those new people and may potentially be dangerous (multiple command in one line, separated by semicolon). Compared to ":git log -- (exclude)somethign" the worst that could happen is a syntax error message from bash.
Other than that I'm fine with '!' being the shortcut.
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
In other words, if there's no path (or pattern) component after the magic, then the next argument must contain the path. This enables path completion and I haven't seen any drawbacks yet..
Show 9 quoted lines
>> @@ -427,6 +430,10 @@ void parse_pathspec(struct pathspec *pathspec,
>> pathspec->magic |= item[i].magic;
>> }
>>
>> + 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, then I made a mistake of typing "git log -- :-po" and wondered why it shows nothing. Intuitively, if "git log" shows every path, then "git log -- :-po" should show every path except 'po' and the user should not be required to type "git log -- :/ :-po". parse_pathspec() can do that, but it's more work and I'm lazy so I push that back to the user until they scream :)
Show 23 quoted lines
>> +enum interesting tree_entry_interesting(const struct name_entry *entry,
>> + struct strbuf *base, int base_offset,
>> + const struct pathspec *ps)
>> +{
>> + enum interesting positive, negative;
>> + positive = tree_entry_interesting_1(entry, base, base_offset, ps, 0);
>> +
>> + /*
>> + * # | 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.
-- Duy