Re: [PATCH] Support pathspec magic :(exclude) and its short form :-
Duy Nguyen writes: > 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. >>> + 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" ;-) >>> + /* >>> + * # | 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Support pathspec magic :(exclude) and its short form :-
On Thu, Nov 21, 2013 at 6:48 AM, Junio C Hamano wrote: >> 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 " 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.. >> @@ -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 :) >> +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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Support pathspec magic :(exclude) and its short form :-
Nguyễn Thái Ngọc Duy writes: > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > This is yet another stab at the negative pathspec thing. It's not > ready yet (there are a few XXXs) but I could use some feedback > regarding the interface, or the behavior. It looks better this time > now that pathspec magic is supported (or maybe I'm just biased). > > For :(glob) or :(icase) you're more likely to enable it for all > pathspec, i.e. --glob-pathspecs. But I expect :(exclude) to be typed > more often (it does not make sense to add --exclude-pathspecs to > exclude everything), which is why I add the short form for it. > > 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 " option), so avoiding "!" and introducing an inconsistent "-" only to appease bash leaves somewhat a funny taste in my mouth. > Documentation/glossary-content.txt | 5 > builtin/add.c | 5 +++- > dir.c | 50 +++- > pathspec.c | 9 ++- > pathspec.h | 4 ++- > tree-walk.c| 52 > +++--- > 6 files changed, 112 insertions(+), 13 deletions(-) > > diff --git a/Documentation/glossary-content.txt > b/Documentation/glossary-content.txt > index e470661..f7d7d8c 100644 > --- a/Documentation/glossary-content.txt > +++ b/Documentation/glossary-content.txt > @@ -377,6 +377,11 @@ full pathname may have special meaning: > - Other consecutive asterisks are considered invalid. > + > Glob magic is incompatible with literal magic. > + > +exclude `-`;; > + After a path matches any non-exclude pathspec, it will be run > + through all exclude pathspec. If it matches, the path is > + ignored. > -- > + > Currently only the slash `/` is recognized as the "magic signature", No longer, no? "magic signature" is a non-alphanumeric that follows the ':' introducer, as opposed to "magic words" that are in ":(...)". > diff --git a/builtin/add.c b/builtin/add.c > index 226f758..0df73ae 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -540,10 +540,13 @@ int cmd_add(int argc, const char **argv, const char > *prefix) > PATHSPEC_FROMTOP | > PATHSPEC_LITERAL | > PATHSPEC_GLOB | > -PATHSPEC_ICASE); > +PATHSPEC_ICASE | > +PATHSPEC_EXCLUDE); > > for (i = 0; i < pathspec.nr; i++) { > const char *path = pathspec.items[i].match; > + if (pathspec.items[i].magic & PATHSPEC_EXCLUDE) > + continue; > if (!seen[i] && > ((pathspec.items[i].magic & > (PATHSPEC_GLOB | PATHSPEC_ICASE)) || So "git add ':(exclude)junk/' '*.c'" to add all .c files except for the ones in the 'junk/' directory may find that ':(exclude)junk/' matched nothing (because there is no .c file in there), and that is not an error. It makes sense to me. > diff --git a/dir.c b/dir.c > index 23b6de4..e2df82f 100644 > --- a/dir.c > +++ b/dir.c > @@ -126,10 +126,13 @@ static size_t common_prefix_len(const struct pathspec > *pathspec) > PATHSPEC_MAXDEPTH | > PATHSPEC_LITERAL | > PATHSPEC_GLOB | > -PATHSPEC_ICASE); > +PATHSPEC_ICASE | > +PATHSPEC_EXCLUDE); > > for (n = 0; n < pathspec->nr; n++) { > size_t i = 0, len = 0, item_len; > + if (pathspec->items[n].magic & PATHSPEC_EXCLUDE) > + continue; > if (pathspec->items[n].magic & PATHSPEC_ICASE) > item_len = pathspec->items[n].prefix; > else Likewise. Exclusion does not participate in the early culling with the common prefix. > @@ -1375,11 +1407,17 @@ int read_directory(struct dir_struct *dir, const char > *path, int len, const stru > PATHSPEC_MAXDEPTH | > PATHSPEC_LITERAL | > PATHSPEC_GLOB | > -PATHSPEC_ICASE); > +PATHSPEC_ICASE | > +PATHSPEC_EXCLUDE); > > if (has_symlink_leading_path(path, len)) > return dir->nr; > > + /* > + * XXX: exclude patterns are treated like positive ones in > + * create_simplify! This is not wrong, but may make path > + * filtering less efficient. > + */ True, but "git
[PATCH] Support pathspec magic :(exclude) and its short form :-
Signed-off-by: Nguyễn Thái Ngọc Duy --- This is yet another stab at the negative pathspec thing. It's not ready yet (there are a few XXXs) but I could use some feedback regarding the interface, or the behavior. It looks better this time now that pathspec magic is supported (or maybe I'm just biased). For :(glob) or :(icase) you're more likely to enable it for all pathspec, i.e. --glob-pathspecs. But I expect :(exclude) to be typed more often (it does not make sense to add --exclude-pathspecs to exclude everything), which is why I add the short form for it. 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 '-'. Documentation/glossary-content.txt | 5 builtin/add.c | 5 +++- dir.c | 50 +++- pathspec.c | 9 ++- pathspec.h | 4 ++- tree-walk.c| 52 +++--- 6 files changed, 112 insertions(+), 13 deletions(-) diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt index e470661..f7d7d8c 100644 --- a/Documentation/glossary-content.txt +++ b/Documentation/glossary-content.txt @@ -377,6 +377,11 @@ full pathname may have special meaning: - Other consecutive asterisks are considered invalid. + Glob magic is incompatible with literal magic. + +exclude `-`;; + After a path matches any non-exclude pathspec, it will be run + through all exclude pathspec. If it matches, the path is + ignored. -- + Currently only the slash `/` is recognized as the "magic signature", diff --git a/builtin/add.c b/builtin/add.c index 226f758..0df73ae 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -540,10 +540,13 @@ int cmd_add(int argc, const char **argv, const char *prefix) PATHSPEC_FROMTOP | PATHSPEC_LITERAL | PATHSPEC_GLOB | - PATHSPEC_ICASE); + PATHSPEC_ICASE | + PATHSPEC_EXCLUDE); for (i = 0; i < pathspec.nr; i++) { const char *path = pathspec.items[i].match; + if (pathspec.items[i].magic & PATHSPEC_EXCLUDE) + continue; if (!seen[i] && ((pathspec.items[i].magic & (PATHSPEC_GLOB | PATHSPEC_ICASE)) || diff --git a/dir.c b/dir.c index 23b6de4..e2df82f 100644 --- a/dir.c +++ b/dir.c @@ -126,10 +126,13 @@ static size_t common_prefix_len(const struct pathspec *pathspec) PATHSPEC_MAXDEPTH | PATHSPEC_LITERAL | PATHSPEC_GLOB | - PATHSPEC_ICASE); + PATHSPEC_ICASE | + PATHSPEC_EXCLUDE); for (n = 0; n < pathspec->nr; n++) { size_t i = 0, len = 0, item_len; + if (pathspec->items[n].magic & PATHSPEC_EXCLUDE) + continue; if (pathspec->items[n].magic & PATHSPEC_ICASE) item_len = pathspec->items[n].prefix; else @@ -279,9 +282,10 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix, * pathspec did not match any names, which could indicate that the * user mistyped the nth pathspec. */ -int match_pathspec_depth(const struct pathspec *ps, -const char *name, int namelen, -int prefix, char *seen) +static int match_pathspec_depth_1(const struct pathspec *ps, + const char *name, int namelen, + int prefix, char *seen, + int exclude) { int i, retval = 0; @@ -290,7 +294,8 @@ int match_pathspec_depth(const struct pathspec *ps, PATHSPEC_MAXDEPTH | PATHSPEC_LITERAL | PATHSPEC_GLOB | - PATHSPEC_ICASE); + PATHSPEC_ICASE | + PATHSPEC_EXCLUDE); if (!ps->nr) { if (!ps->recursive || @@ -309,6 +314,11 @@ int match_pathspec_depth(const struct pathspec *ps, for (i = ps->nr - 1; i >= 0; i--) { int how; + + if ((!exclude && ps->items[i].magic & PATHSPEC_EXCLUDE) || + ( exclude && !(ps->items[i].magic & PATHSPEC_EXCLUDE))) + continue; + if (seen && seen[i] == MATCHED_EXACTLY) continue; how = match_pathspec_item(ps->items+i, prefix, name, namelen); @@ -327,6 +337,16 @@ int match_pathspec_depth(const struct pat