* [PATCH 0/2] bringing attributes to pathspecs
@ 2017-03-09 21:07 Brandon Williams
2017-03-09 21:07 ` [PATCH 1/2] pathspec: allow querying for attributes Brandon Williams
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Brandon Williams @ 2017-03-09 21:07 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
This small series extends the pathspec magic to allow users to specify
attributes that files must have in order for a pathspec to 'match' a file.
One potential use for this is to allow a repository to specify attributes for a
set of files. The user can then specify that attribute as a pathspec to
perform various operations only on that set of files. One simple example:
git ls-files -- ":(attr:text)"
can be used to list all of the files with the 'text' attribute.
Brandon Williams (2):
pathspec: allow querying for attributes
pathspec: allow escaped query values
Documentation/glossary-content.txt | 20 ++++
attr.c | 17 ++++
attr.h | 1 +
dir.c | 43 ++++++++-
pathspec.c | 165 ++++++++++++++++++++++++++++++--
pathspec.h | 16 +++-
t/t6135-pathspec-with-attrs.sh | 190 +++++++++++++++++++++++++++++++++++++
7 files changed, 442 insertions(+), 10 deletions(-)
create mode 100755 t/t6135-pathspec-with-attrs.sh
--
2.12.0.246.ga2ecc84866-goog
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 1/2] pathspec: allow querying for attributes 2017-03-09 21:07 [PATCH 0/2] bringing attributes to pathspecs Brandon Williams @ 2017-03-09 21:07 ` Brandon Williams 2017-03-09 22:19 ` Jonathan Tan 2017-03-13 2:43 ` Junio C Hamano 2017-03-09 21:07 ` [PATCH 2/2] pathspec: allow escaped query values Brandon Williams ` (2 subsequent siblings) 3 siblings, 2 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-09 21:07 UTC (permalink / raw) To: git; +Cc: Brandon Williams, sbeller, pclouds The pathspec mechanism is extended via the new ":(attr:eol=input)pattern/to/match" syntax to filter paths so that it requires paths to not just match the given pattern but also have the specified attrs attached for them to be chosen. Based on a patch by Stefan Beller <sbeller@google.com> Signed-off-by: Brandon Williams <bmwill@google.com> --- Documentation/glossary-content.txt | 20 ++++ attr.c | 17 ++++ attr.h | 1 + dir.c | 43 ++++++++- pathspec.c | 119 +++++++++++++++++++++++- pathspec.h | 16 +++- t/t6135-pathspec-with-attrs.sh | 181 +++++++++++++++++++++++++++++++++++++ 7 files changed, 388 insertions(+), 9 deletions(-) create mode 100755 t/t6135-pathspec-with-attrs.sh diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt index fc9320e59..5c32d1905 100644 --- a/Documentation/glossary-content.txt +++ b/Documentation/glossary-content.txt @@ -384,6 +384,26 @@ full pathname may have special meaning: + Glob magic is incompatible with literal magic. +attr;; +After `attr:` comes a space separated list of "attribute +requirements", all of which must be met in order for the +path to be considered a match; this is in addition to the +usual non-magic pathspec pattern matching. ++ +Each of the attribute requirements for the path takes one of +these forms: + +- "`ATTR`" requires that the attribute `ATTR` must be set. + +- "`-ATTR`" requires that the attribute `ATTR` must be unset. + +- "`ATTR=VALUE`" requires that the attribute `ATTR` must be + set to the string `VALUE`. + +- "`!ATTR`" requires that the attribute `ATTR` must be + unspecified. ++ + exclude;; After a path matches any non-exclude pathspec, it will be run through all exclude pathspec (magic signature: `!` or its diff --git a/attr.c b/attr.c index 5493bff22..7e2134471 100644 --- a/attr.c +++ b/attr.c @@ -603,6 +603,23 @@ struct attr_check *attr_check_initl(const char *one, ...) return check; } +struct attr_check *attr_check_dup(const struct attr_check *check) +{ + struct attr_check *ret; + + if (!check) + return NULL; + + ret = attr_check_alloc(); + + ret->nr = check->nr; + ret->alloc = check->alloc; + ALLOC_ARRAY(ret->items, ret->nr); + COPY_ARRAY(ret->items, check->items, ret->nr); + + return ret; +} + struct attr_check_item *attr_check_append(struct attr_check *check, const struct git_attr *attr) { diff --git a/attr.h b/attr.h index 48ab3e1c2..442d464db 100644 --- a/attr.h +++ b/attr.h @@ -44,6 +44,7 @@ struct attr_check { extern struct attr_check *attr_check_alloc(void); extern struct attr_check *attr_check_initl(const char *, ...); +extern struct attr_check *attr_check_dup(const struct attr_check *check); extern struct attr_check_item *attr_check_append(struct attr_check *check, const struct git_attr *attr); diff --git a/dir.c b/dir.c index 4541f9e14..2fe7acbcf 100644 --- a/dir.c +++ b/dir.c @@ -9,6 +9,7 @@ */ #include "cache.h" #include "dir.h" +#include "attr.h" #include "refs.h" #include "wildmatch.h" #include "pathspec.h" @@ -134,7 +135,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec) PATHSPEC_LITERAL | PATHSPEC_GLOB | PATHSPEC_ICASE | - PATHSPEC_EXCLUDE); + PATHSPEC_EXCLUDE | + PATHSPEC_ATTR); for (n = 0; n < pathspec->nr; n++) { size_t i = 0, len = 0, item_len; @@ -209,6 +211,36 @@ int within_depth(const char *name, int namelen, #define DO_MATCH_DIRECTORY (1<<1) #define DO_MATCH_SUBMODULE (1<<2) +static int match_attrs(const char *name, int namelen, + const struct pathspec_item *item) +{ + int i; + + git_check_attr(name, item->attr_check); + for (i = 0; i < item->attr_match_nr; i++) { + const char *value; + int matched; + enum attr_match_mode match_mode; + + value = item->attr_check->items[i].value; + match_mode = item->attr_match[i].match_mode; + + if (ATTR_TRUE(value)) + matched = (match_mode == MATCH_SET); + else if (ATTR_FALSE(value)) + matched = (match_mode == MATCH_UNSET); + else if (ATTR_UNSET(value)) + matched = (match_mode == MATCH_UNSPECIFIED); + else + matched = (match_mode == MATCH_VALUE && + !strcmp(item->attr_match[i].value, value)); + if (!matched) + return 0; + } + + return 1; +} + /* * Does 'match' match the given name? * A match is found if @@ -261,6 +293,9 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix, strncmp(item->match, name - prefix, item->prefix)) return 0; + if (item->attr_match_nr && !match_attrs(name, namelen, item)) + return 0; + /* If the match was just the prefix, we matched */ if (!*match) return MATCHED_RECURSIVELY; @@ -339,7 +374,8 @@ static int do_match_pathspec(const struct pathspec *ps, PATHSPEC_LITERAL | PATHSPEC_GLOB | PATHSPEC_ICASE | - PATHSPEC_EXCLUDE); + PATHSPEC_EXCLUDE | + PATHSPEC_ATTR); if (!ps->nr) { if (!ps->recursive || @@ -1361,7 +1397,8 @@ static int simplify_away(const char *path, int pathlen, PATHSPEC_LITERAL | PATHSPEC_GLOB | PATHSPEC_ICASE | - PATHSPEC_EXCLUDE); + PATHSPEC_EXCLUDE | + PATHSPEC_ATTR); for (i = 0; i < pathspec->nr; i++) { const struct pathspec_item *item = &pathspec->items[i]; diff --git a/pathspec.c b/pathspec.c index b961f00c8..583ed5208 100644 --- a/pathspec.c +++ b/pathspec.c @@ -1,6 +1,7 @@ #include "cache.h" #include "dir.h" #include "pathspec.h" +#include "attr.h" /* * Finds which of the given pathspecs match items in the index. @@ -72,6 +73,7 @@ static struct pathspec_magic { { PATHSPEC_GLOB, '\0', "glob" }, { PATHSPEC_ICASE, '\0', "icase" }, { PATHSPEC_EXCLUDE, '!', "exclude" }, + { PATHSPEC_ATTR, '\0', "attr" }, }; static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) @@ -87,6 +89,72 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) strbuf_addf(sb, ",prefix:%d)", prefixlen); } +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value) +{ + struct string_list_item *si; + struct string_list list = STRING_LIST_INIT_DUP; + + if (item->attr_check) + die(_("Only one 'attr:' specification is allowed.")); + + if (!value || !strlen(value)) + die(_("attr spec must not be empty")); + + string_list_split(&list, value, ' ', -1); + string_list_remove_empty_items(&list, 0); + + item->attr_check = attr_check_alloc(); + ALLOC_GROW(item->attr_match, + item->attr_match_nr + list.nr, + item->attr_match_alloc); + + for_each_string_list_item(si, &list) { + size_t attr_len; + char *attr_name; + const struct git_attr *a; + + int j = item->attr_match_nr++; + const char *attr = si->string; + struct attr_match *am = &item->attr_match[j]; + + switch (*attr) { + case '!': + am->match_mode = MATCH_UNSPECIFIED; + attr++; + attr_len = strlen(attr); + break; + case '-': + am->match_mode = MATCH_UNSET; + attr++; + attr_len = strlen(attr); + break; + default: + attr_len = strcspn(attr, "="); + if (attr[attr_len] != '=') + am->match_mode = MATCH_SET; + else { + am->match_mode = MATCH_VALUE; + am->value = xstrdup(&attr[attr_len + 1]); + if (strchr(am->value, '\\')) + die(_("attr spec values must not contain backslashes")); + } + break; + } + + attr_name = xmemdupz(attr, attr_len); + a = git_attr(attr_name); + if (!a) + die(_("invalid attribute name %s"), attr_name); + + attr_check_append(item->attr_check, a); + + free(attr_name); + } + + string_list_clear(&list, 0); + return; +} + static inline int get_literal_global(void) { static int literal = -1; @@ -164,6 +232,7 @@ static int get_global_magic(int element_magic) * returns the position in 'elem' after all magic has been parsed */ static const char *parse_long_magic(unsigned *magic, int *prefix_len, + struct pathspec_item *item, const char *elem) { const char *pos; @@ -189,6 +258,14 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len, continue; } + if (starts_with(pos, "attr:")) { + char *attr_body = xmemdupz(pos + 5, len - 5); + parse_pathspec_attr_match(item, attr_body); + *magic |= PATHSPEC_ATTR; + free(attr_body); + continue; + } + for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) { if (strlen(pathspec_magic[i].name) == len && !strncmp(pathspec_magic[i].name, pos, len)) { @@ -252,13 +329,14 @@ static const char *parse_short_magic(unsigned *magic, const char *elem) } static const char *parse_element_magic(unsigned *magic, int *prefix_len, + struct pathspec_item *item, const char *elem) { if (elem[0] != ':' || get_literal_global()) return elem; /* nothing to do */ else if (elem[1] == '(') /* longhand */ - return parse_long_magic(magic, prefix_len, elem); + return parse_long_magic(magic, prefix_len, item, elem); else /* shorthand */ return parse_short_magic(magic, elem); @@ -335,12 +413,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags, char *match; int pathspec_prefix = -1; + item->attr_check = NULL; + item->attr_match = NULL; + item->attr_match_nr = 0; + item->attr_match_alloc = 0; + /* PATHSPEC_LITERAL_PATH ignores magic */ if (flags & PATHSPEC_LITERAL_PATH) { magic = PATHSPEC_LITERAL; } else { copyfrom = parse_element_magic(&element_magic, &pathspec_prefix, + item, elt); magic |= element_magic; magic |= get_global_magic(element_magic); @@ -544,6 +628,10 @@ void parse_pathspec(struct pathspec *pathspec, if (item[i].nowildcard_len < item[i].len) pathspec->has_wildcard = 1; pathspec->magic |= item[i].magic; + + if (item[i].attr_check && + item[i].attr_check->nr != item[i].attr_match_nr) + die("BUG: should have same number of entries"); } /* @@ -565,26 +653,47 @@ void parse_pathspec(struct pathspec *pathspec, void copy_pathspec(struct pathspec *dst, const struct pathspec *src) { - int i; + int i, j; *dst = *src; ALLOC_ARRAY(dst->items, dst->nr); COPY_ARRAY(dst->items, src->items, dst->nr); for (i = 0; i < dst->nr; i++) { - dst->items[i].match = xstrdup(src->items[i].match); - dst->items[i].original = xstrdup(src->items[i].original); + struct pathspec_item *d = &dst->items[i]; + struct pathspec_item *s = &src->items[i]; + + d->match = xstrdup(s->match); + d->original = xstrdup(s->original); + + ALLOC_ARRAY(d->attr_match, d->attr_match_nr); + COPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr); + for (j = 0; j < d->attr_match_nr; j++) { + const char *value = s->attr_match[j].value; + if (value) + d->attr_match[j].value = xstrdup(value); + } + + d->attr_check = attr_check_dup(s->attr_check); } } void clear_pathspec(struct pathspec *pathspec) { - int i; + int i, j; for (i = 0; i < pathspec->nr; i++) { free(pathspec->items[i].match); free(pathspec->items[i].original); + + for (j = 0; j < pathspec->items[j].attr_match_nr; j++) + free(pathspec->items[i].attr_match[j].value); + free(pathspec->items[i].attr_match); + + if (pathspec->items[i].attr_check) + attr_check_free(pathspec->items[i].attr_check); } + free(pathspec->items); pathspec->items = NULL; pathspec->nr = 0; diff --git a/pathspec.h b/pathspec.h index 49fd823dd..83625f006 100644 --- a/pathspec.h +++ b/pathspec.h @@ -8,13 +8,15 @@ #define PATHSPEC_GLOB (1<<3) #define PATHSPEC_ICASE (1<<4) #define PATHSPEC_EXCLUDE (1<<5) +#define PATHSPEC_ATTR (1<<6) #define PATHSPEC_ALL_MAGIC \ (PATHSPEC_FROMTOP | \ PATHSPEC_MAXDEPTH | \ PATHSPEC_LITERAL | \ PATHSPEC_GLOB | \ PATHSPEC_ICASE | \ - PATHSPEC_EXCLUDE) + PATHSPEC_EXCLUDE | \ + PATHSPEC_ATTR) #define PATHSPEC_ONESTAR 1 /* the pathspec pattern satisfies GFNM_ONESTAR */ @@ -31,6 +33,18 @@ struct pathspec { int len, prefix; int nowildcard_len; int flags; + int attr_match_nr; + int attr_match_alloc; + struct attr_match { + char *value; + enum attr_match_mode { + MATCH_SET, + MATCH_UNSET, + MATCH_VALUE, + MATCH_UNSPECIFIED + } match_mode; + } *attr_match; + struct attr_check *attr_check; } *items; }; diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh new file mode 100755 index 000000000..b5e5a0607 --- /dev/null +++ b/t/t6135-pathspec-with-attrs.sh @@ -0,0 +1,181 @@ +#!/bin/sh + +test_description='test labels in pathspecs' +. ./test-lib.sh + +test_expect_success 'setup a tree' ' + cat <<-EOF >expect && + fileA + fileAB + fileAC + fileB + fileBC + fileC + fileNoLabel + fileSetLabel + fileUnsetLabel + fileValue + fileWrongLabel + sub/fileA + sub/fileAB + sub/fileAC + sub/fileB + sub/fileBC + sub/fileC + sub/fileNoLabel + sub/fileSetLabel + sub/fileUnsetLabel + sub/fileValue + sub/fileWrongLabel + EOF + mkdir sub && + while read path + do + : >$path && + git add $path || return 1 + done <expect && + git commit -m "initial commit" && + git ls-files >actual && + test_cmp expect actual +' + +test_expect_success 'pathspec with no attr' ' + test_must_fail git ls-files ":(attr:)" +' + +test_expect_success 'pathspec with labels and non existent .gitattributes' ' + git ls-files ":(attr:label)" >actual && + test_must_be_empty actual +' + +test_expect_success 'setup .gitattributes' ' + cat <<-EOF >.gitattributes && + fileA labelA + fileB labelB + fileC labelC + fileAB labelA labelB + fileAC labelA labelC + fileBC labelB labelC + fileUnsetLabel -label + fileSetLabel label + fileValue label=foo + fileWrongLabel label☺ + EOF + git add .gitattributes && + git commit -m "add attributes" +' + +test_expect_success 'check specific set attr' ' + cat <<-EOF >expect && + fileSetLabel + sub/fileSetLabel + EOF + git ls-files ":(attr:label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check specific unset attr' ' + cat <<-EOF >expect && + fileUnsetLabel + sub/fileUnsetLabel + EOF + git ls-files ":(attr:-label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check specific value attr' ' + cat <<-EOF >expect && + fileValue + sub/fileValue + EOF + git ls-files ":(attr:label=foo)" >actual && + test_cmp expect actual && + git ls-files ":(attr:label=bar)" >actual && + test_must_be_empty actual +' + +test_expect_success 'check unspecified attr' ' + cat <<-EOF >expect && + .gitattributes + fileA + fileAB + fileAC + fileB + fileBC + fileC + fileNoLabel + fileWrongLabel + sub/fileA + sub/fileAB + sub/fileAC + sub/fileB + sub/fileBC + sub/fileC + sub/fileNoLabel + sub/fileWrongLabel + EOF + git ls-files ":(attr:!label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check multiple unspecified attr' ' + cat <<-EOF >expect && + .gitattributes + fileC + fileNoLabel + fileWrongLabel + sub/fileC + sub/fileNoLabel + sub/fileWrongLabel + EOF + git ls-files ":(attr:!labelB !labelA !label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check label with more labels but excluded path' ' + cat <<-EOF >expect && + fileAB + fileB + fileBC + EOF + git ls-files ":(attr:labelB)" ":(exclude)sub/" >actual && + test_cmp expect actual +' + +test_expect_success 'check label excluding other labels' ' + cat <<-EOF >expect && + fileAB + fileB + fileBC + sub/fileAB + sub/fileB + EOF + git ls-files ":(attr:labelB)" ":(exclude,attr:labelC)sub/" >actual && + test_cmp expect actual +' + +test_expect_success 'fail on multiple attr specifiers in one pathspec item' ' + test_must_fail git ls-files . ":(attr:labelB,attr:labelC)" 2>actual && + test_i18ngrep "Only one" actual +' + +test_expect_success 'fail if attr magic is used places not implemented' ' + # The main purpose of this test is to check that we actually fail + # when you attempt to use attr magic in commands that do not implement + # attr magic. This test does not advocate git-add to stay that way, + # though, but git-add is convenient as it has its own internal pathspec + # parsing. + test_must_fail git add ":(attr:labelB)" 2>actual && + test_i18ngrep "unsupported magic" actual +' + +test_expect_success 'abort on giving invalid label on the command line' ' + test_must_fail git ls-files . ":(attr:☺)" +' + +test_expect_success 'abort on asking for wrong magic' ' + test_must_fail git ls-files . ":(attr:-label=foo)" && + test_must_fail git ls-files . ":(attr:!label=foo)" +' + +test_done -- 2.12.0.246.ga2ecc84866-goog ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] pathspec: allow querying for attributes 2017-03-09 21:07 ` [PATCH 1/2] pathspec: allow querying for attributes Brandon Williams @ 2017-03-09 22:19 ` Jonathan Tan 2017-03-10 18:26 ` Brandon Williams 2017-03-13 2:43 ` Junio C Hamano 1 sibling, 1 reply; 23+ messages in thread From: Jonathan Tan @ 2017-03-09 22:19 UTC (permalink / raw) To: Brandon Williams, git; +Cc: sbeller, pclouds On 03/09/2017 01:07 PM, Brandon Williams wrote: > diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt > index fc9320e59..5c32d1905 100644 > --- a/Documentation/glossary-content.txt > +++ b/Documentation/glossary-content.txt > @@ -384,6 +384,26 @@ full pathname may have special meaning: > + > Glob magic is incompatible with literal magic. > > +attr;; > +After `attr:` comes a space separated list of "attribute > +requirements", all of which must be met in order for the > +path to be considered a match; this is in addition to the > +usual non-magic pathspec pattern matching. > ++ > +Each of the attribute requirements for the path takes one of > +these forms: > + > +- "`ATTR`" requires that the attribute `ATTR` must be set. As a relative newcomer to attributes, I was confused by the fact that "set" and "set to a value" is different (and likewise "unset" and "unspecified"). Maybe it's worthwhile including a link to "gitattributes" to explain the different (exclusive) states that an attribute can be in. > + > +- "`-ATTR`" requires that the attribute `ATTR` must be unset. > + > +- "`ATTR=VALUE`" requires that the attribute `ATTR` must be > + set to the string `VALUE`. > + > +- "`!ATTR`" requires that the attribute `ATTR` must be > + unspecified. It would read better to me if you omitted "must" in all 4 bullet points (and it is redundant anyway with "requires"), but I don't feel too strongly about this. > diff --git a/pathspec.c b/pathspec.c > index b961f00c8..583ed5208 100644 > --- a/pathspec.c > +++ b/pathspec.c > @@ -87,6 +89,72 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) > strbuf_addf(sb, ",prefix:%d)", prefixlen); > } > > +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value) > +{ > + struct string_list_item *si; > + struct string_list list = STRING_LIST_INIT_DUP; > + > + if (item->attr_check) > + die(_("Only one 'attr:' specification is allowed.")); > + > + if (!value || !strlen(value)) You can write `!*value` instead of `!strlen(value)`. > + die(_("attr spec must not be empty")); > + > + string_list_split(&list, value, ' ', -1); You could avoid some allocations by using the in-place variant (since value is a newly allocated string not used elsewhere) but it is probably not that important in this argument parsing case. > + string_list_remove_empty_items(&list, 0); > + > + item->attr_check = attr_check_alloc(); > + ALLOC_GROW(item->attr_match, > + item->attr_match_nr + list.nr, > + item->attr_match_alloc); Is there a time when this function is called while item->attr_match_nr is not zero? > + > + for_each_string_list_item(si, &list) { > + size_t attr_len; > + char *attr_name; > + const struct git_attr *a; > + > + int j = item->attr_match_nr++; > + const char *attr = si->string; > + struct attr_match *am = &item->attr_match[j]; > + > + switch (*attr) { > + case '!': > + am->match_mode = MATCH_UNSPECIFIED; > + attr++; > + attr_len = strlen(attr); > + break; > + case '-': > + am->match_mode = MATCH_UNSET; > + attr++; > + attr_len = strlen(attr); > + break; > + default: > + attr_len = strcspn(attr, "="); > + if (attr[attr_len] != '=') > + am->match_mode = MATCH_SET; > + else { > + am->match_mode = MATCH_VALUE; > + am->value = xstrdup(&attr[attr_len + 1]); > + if (strchr(am->value, '\\')) > + die(_("attr spec values must not contain backslashes")); > + } > + break; > + } > + > + attr_name = xmemdupz(attr, attr_len); > + a = git_attr(attr_name); > + if (!a) > + die(_("invalid attribute name %s"), attr_name); > + > + attr_check_append(item->attr_check, a); > + > + free(attr_name); > + } > + > + string_list_clear(&list, 0); > + return; Redundant return? > @@ -544,6 +628,10 @@ void parse_pathspec(struct pathspec *pathspec, > if (item[i].nowildcard_len < item[i].len) > pathspec->has_wildcard = 1; > pathspec->magic |= item[i].magic; > + > + if (item[i].attr_check && > + item[i].attr_check->nr != item[i].attr_match_nr) > + die("BUG: should have same number of entries"); I'm not sure if this check is giving us any benefit - I would expect this type of code before some other code that assumed that the numbers matched, and that will potentially segfault if not. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] pathspec: allow querying for attributes 2017-03-09 22:19 ` Jonathan Tan @ 2017-03-10 18:26 ` Brandon Williams 0 siblings, 0 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-10 18:26 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, sbeller, pclouds On 03/09, Jonathan Tan wrote: > On 03/09/2017 01:07 PM, Brandon Williams wrote: > >diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt > >index fc9320e59..5c32d1905 100644 > >--- a/Documentation/glossary-content.txt > >+++ b/Documentation/glossary-content.txt > >@@ -384,6 +384,26 @@ full pathname may have special meaning: > > + > > Glob magic is incompatible with literal magic. > > > >+attr;; > >+After `attr:` comes a space separated list of "attribute > >+requirements", all of which must be met in order for the > >+path to be considered a match; this is in addition to the > >+usual non-magic pathspec pattern matching. > >++ > >+Each of the attribute requirements for the path takes one of > >+these forms: > >+ > >+- "`ATTR`" requires that the attribute `ATTR` must be set. > > As a relative newcomer to attributes, I was confused by the fact > that "set" and "set to a value" is different (and likewise "unset" > and "unspecified"). Maybe it's worthwhile including a link to > "gitattributes" to explain the different (exclusive) states that an > attribute can be in. Good idea! I'll add in a link to gitattributes. > > >+ > >+- "`-ATTR`" requires that the attribute `ATTR` must be unset. > >+ > >+- "`ATTR=VALUE`" requires that the attribute `ATTR` must be > >+ set to the string `VALUE`. > >+ > >+- "`!ATTR`" requires that the attribute `ATTR` must be > >+ unspecified. > > It would read better to me if you omitted "must" in all 4 bullet > points (and it is redundant anyway with "requires"), but I don't > feel too strongly about this. I agree, the first paragraph already says "must" so it reads better without repeating must over and over again. > > >diff --git a/pathspec.c b/pathspec.c > >index b961f00c8..583ed5208 100644 > >--- a/pathspec.c > >+++ b/pathspec.c > >@@ -87,6 +89,72 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) > > strbuf_addf(sb, ",prefix:%d)", prefixlen); > > } > > > >+static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value) > >+{ > >+ struct string_list_item *si; > >+ struct string_list list = STRING_LIST_INIT_DUP; > >+ > >+ if (item->attr_check) > >+ die(_("Only one 'attr:' specification is allowed.")); > >+ > >+ if (!value || !strlen(value)) > > You can write `!*value` instead of `!strlen(value)`. > Done. > >+ string_list_remove_empty_items(&list, 0); > >+ > >+ item->attr_check = attr_check_alloc(); > >+ ALLOC_GROW(item->attr_match, > >+ item->attr_match_nr + list.nr, > >+ item->attr_match_alloc); > > Is there a time when this function is called while > item->attr_match_nr is not zero? Nope, it pretty much has to be zero. I'll change this to just use list.nr. item->attr_match_nr will be incremented up to list.nr over the course of the for loop and I'll move the equality check to the end of this function. > >+ string_list_clear(&list, 0); > >+ return; > > Redundant return? I'll remove it. > > >@@ -544,6 +628,10 @@ void parse_pathspec(struct pathspec *pathspec, > > if (item[i].nowildcard_len < item[i].len) > > pathspec->has_wildcard = 1; > > pathspec->magic |= item[i].magic; > >+ > >+ if (item[i].attr_check && > >+ item[i].attr_check->nr != item[i].attr_match_nr) > >+ die("BUG: should have same number of entries"); > > I'm not sure if this check is giving us any benefit - I would expect > this type of code before some other code that assumed that the > numbers matched, and that will potentially segfault if not. I'll push the check to right after the object creation (see comment above). -- Brandon Williams ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] pathspec: allow querying for attributes 2017-03-09 21:07 ` [PATCH 1/2] pathspec: allow querying for attributes Brandon Williams 2017-03-09 22:19 ` Jonathan Tan @ 2017-03-13 2:43 ` Junio C Hamano 2017-03-13 18:30 ` Stefan Beller 1 sibling, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2017-03-13 2:43 UTC (permalink / raw) To: Brandon Williams; +Cc: git, sbeller, pclouds Brandon Williams <bmwill@google.com> writes: > +struct attr_check *attr_check_dup(const struct attr_check *check) > +{ > + struct attr_check *ret; > + > + if (!check) > + return NULL; > + > + ret = attr_check_alloc(); > + > + ret->nr = check->nr; > + ret->alloc = check->alloc; > + ALLOC_ARRAY(ret->items, ret->nr); > + COPY_ARRAY(ret->items, check->items, ret->nr); > + > + return ret; > +} Because an attr_check instance cannot be shared and used by multiple threads, we expect that the callers that go multi-thread to copy pathspec to each worker, and preload_index(), which is an existing example of such a caller, already does so with copy_pathspec(). Makes sense. > @@ -565,26 +653,47 @@ void parse_pathspec(struct pathspec *pathspec, > > void copy_pathspec(struct pathspec *dst, const struct pathspec *src) > { > - int i; > + int i, j; > > *dst = *src; > ALLOC_ARRAY(dst->items, dst->nr); > COPY_ARRAY(dst->items, src->items, dst->nr); > > for (i = 0; i < dst->nr; i++) { > - dst->items[i].match = xstrdup(src->items[i].match); > - dst->items[i].original = xstrdup(src->items[i].original); > + struct pathspec_item *d = &dst->items[i]; > + struct pathspec_item *s = &src->items[i]; > + > + d->match = xstrdup(s->match); > + d->original = xstrdup(s->original); > + > + ALLOC_ARRAY(d->attr_match, d->attr_match_nr); > + COPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr); > + for (j = 0; j < d->attr_match_nr; j++) { > + const char *value = s->attr_match[j].value; > + if (value) > + d->attr_match[j].value = xstrdup(value); We have xstrdup_or_null(), which may help here. > + } > + > + d->attr_check = attr_check_dup(s->attr_check); > } > } > > void clear_pathspec(struct pathspec *pathspec) > { > - int i; > + int i, j; > > for (i = 0; i < pathspec->nr; i++) { > free(pathspec->items[i].match); > free(pathspec->items[i].original); > + > + for (j = 0; j < pathspec->items[j].attr_match_nr; j++) > + free(pathspec->items[i].attr_match[j].value); > + free(pathspec->items[i].attr_match); > + > + if (pathspec->items[i].attr_check) > + attr_check_free(pathspec->items[i].attr_check); > } > + > free(pathspec->items); > pathspec->items = NULL; > pathspec->nr = 0; OK, makes sense. > diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh > new file mode 100755 > index 000000000..b5e5a0607 > --- /dev/null > +++ b/t/t6135-pathspec-with-attrs.sh > @@ -0,0 +1,181 @@ > +#!/bin/sh > + > +test_description='test labels in pathspecs' > +. ./test-lib.sh > + > +test_expect_success 'setup a tree' ' > + cat <<-EOF >expect && Minor style nit. Quote the 'EOF' marker and you signal to readers that what they'll see are literally the values, and they do not have to worry about $variable_interpolation. I.e. cat <<-\EOF >expect && > +test_expect_success 'fail if attr magic is used places not implemented' ' > + # The main purpose of this test is to check that we actually fail > + # when you attempt to use attr magic in commands that do not implement > + # attr magic. This test does not advocate git-add to stay that way, > + # though, but git-add is convenient as it has its own internal pathspec > + # parsing. That's thought-provoking ;-) Would it help to add a test-pathspec helper, similar to test-config helper, that serves as a vehicle to test this? > + test_must_fail git add ":(attr:labelB)" 2>actual && > + test_i18ngrep "unsupported magic" actual > +' Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] pathspec: allow querying for attributes 2017-03-13 2:43 ` Junio C Hamano @ 2017-03-13 18:30 ` Stefan Beller 0 siblings, 0 replies; 23+ messages in thread From: Stefan Beller @ 2017-03-13 18:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org, Duy Nguyen >> +test_expect_success 'fail if attr magic is used places not implemented' ' >> + # The main purpose of this test is to check that we actually fail >> + # when you attempt to use attr magic in commands that do not implement >> + # attr magic. This test does not advocate git-add to stay that way, >> + # though, but git-add is convenient as it has its own internal pathspec >> + # parsing. > > That's thought-provoking ;-) Would it help to add a test-pathspec > helper, similar to test-config helper, that serves as a vehicle to > test this? I think that is a very good idea; looking through files mentioned by "ls t/ |grep pathspec" most of the tests for pathspecs are done with git-log currently, which also helps with DWIM checking, e.g. if the pathspec part can be interpreted as a ref instead. So I wonder if such a test-pathspec helper would first only cover this test case of not supporting pathspecs, which sounds a bit counter intuitive. Thanks, Stefan ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/2] pathspec: allow escaped query values 2017-03-09 21:07 [PATCH 0/2] bringing attributes to pathspecs Brandon Williams 2017-03-09 21:07 ` [PATCH 1/2] pathspec: allow querying for attributes Brandon Williams @ 2017-03-09 21:07 ` Brandon Williams 2017-03-09 22:31 ` Jonathan Tan 2017-03-09 21:22 ` [PATCH 0/2] bringing attributes to pathspecs Stefan Beller 2017-03-10 18:59 ` [PATCH v2 " Brandon Williams 3 siblings, 1 reply; 23+ messages in thread From: Brandon Williams @ 2017-03-09 21:07 UTC (permalink / raw) To: git; +Cc: Brandon Williams, sbeller, pclouds In our own .gitattributes file we have attributes such as: *.[ch] whitespace=indent,trail,space When querying for attributes we want to be able to ask for the exact value, i.e. git ls-files :(attr:whitespace=indent,trail,space) should work, but the commas are used in the attr magic to introduce the next attr, such that this query currently fails with fatal: Invalid pathspec magic 'trail' in ':(attr:whitespace=indent,trail,space)' This change allows escaping characters by a backslash, such that the query git ls-files :(attr:whitespace=indent\,trail\,space) will match all path that have the value "indent,trail,space" for the whitespace attribute. To accomplish this, we need to modify two places. First `parse_long_magic` needs to not stop early upon seeing a comma or closing paren that is escaped. As a second step we need to remove any escaping from the attr value. Based on a patch by Stefan Beller <sbeller@google.com> Signed-off-by: Brandon Williams <bmwill@google.com> --- pathspec.c | 52 ++++++++++++++++++++++++++++++++++++++---- t/t6135-pathspec-with-attrs.sh | 9 ++++++++ 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/pathspec.c b/pathspec.c index 583ed5208..f89f84a83 100644 --- a/pathspec.c +++ b/pathspec.c @@ -89,6 +89,51 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) strbuf_addf(sb, ",prefix:%d)", prefixlen); } +static size_t strcspn_escaped(const char *s, const char *stop) +{ + const char *i; + + for (i = s; *i; i++) { + /* skip the escaped character */ + if (i[0] == '\\' && i[1]) { + i++; + continue; + } + + if (strchr(stop, *i)) + break; + } + return i - s; +} + +static inline int invalid_value_char(const char ch) +{ + if (isalnum(ch) || strchr(",-_", ch)) + return 0; + return -1; +} + +static char *attr_value_unescape(const char *value) +{ + const char *src; + char *dst, *ret; + + ret = xmallocz(strlen(value)); + for (src = value, dst = ret; *src; src++, dst++) { + if (*src == '\\') { + if (!src[1]) + die(_("Escape character '\\' not allowed as " + "last character in attr value")); + src++; + } + if (invalid_value_char(*src)) + die("cannot use '%c' for value matching", *src); + *dst = *src; + } + *dst = '\0'; + return ret; +} + static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value) { struct string_list_item *si; @@ -133,10 +178,9 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va if (attr[attr_len] != '=') am->match_mode = MATCH_SET; else { + const char *v = &attr[attr_len + 1]; am->match_mode = MATCH_VALUE; - am->value = xstrdup(&attr[attr_len + 1]); - if (strchr(am->value, '\\')) - die(_("attr spec values must not contain backslashes")); + am->value = attr_value_unescape(v); } break; } @@ -239,7 +283,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len, const char *nextat; for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) { - size_t len = strcspn(pos, ",)"); + size_t len = strcspn_escaped(pos, ",)"); int i; if (pos[len] == ',') diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh index b5e5a0607..585d17bad 100755 --- a/t/t6135-pathspec-with-attrs.sh +++ b/t/t6135-pathspec-with-attrs.sh @@ -178,4 +178,13 @@ test_expect_success 'abort on asking for wrong magic' ' test_must_fail git ls-files . ":(attr:!label=foo)" ' +test_expect_success 'check attribute list' ' + cat <<-EOF >>.gitattributes && + * whitespace=indent,trail,space + EOF + git ls-files ":(attr:whitespace=indent\,trail\,space)" >actual && + git ls-files >expect && + test_cmp expect actual +' + test_done -- 2.12.0.246.ga2ecc84866-goog ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] pathspec: allow escaped query values 2017-03-09 21:07 ` [PATCH 2/2] pathspec: allow escaped query values Brandon Williams @ 2017-03-09 22:31 ` Jonathan Tan 2017-03-10 18:53 ` Brandon Williams 0 siblings, 1 reply; 23+ messages in thread From: Jonathan Tan @ 2017-03-09 22:31 UTC (permalink / raw) To: Brandon Williams, git; +Cc: sbeller, pclouds On 03/09/2017 01:07 PM, Brandon Williams wrote: > diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh > index b5e5a0607..585d17bad 100755 > --- a/t/t6135-pathspec-with-attrs.sh > +++ b/t/t6135-pathspec-with-attrs.sh > @@ -178,4 +178,13 @@ test_expect_success 'abort on asking for wrong magic' ' > test_must_fail git ls-files . ":(attr:!label=foo)" > ' > > +test_expect_success 'check attribute list' ' > + cat <<-EOF >>.gitattributes && > + * whitespace=indent,trail,space > + EOF > + git ls-files ":(attr:whitespace=indent\,trail\,space)" >actual && > + git ls-files >expect && > + test_cmp expect actual > +' > + > test_done Is there a way to verify that `\,` is not escaped by the shell into `,`? Maybe also add tests that show \ as the last character and \ escaping another \. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] pathspec: allow escaped query values 2017-03-09 22:31 ` Jonathan Tan @ 2017-03-10 18:53 ` Brandon Williams 0 siblings, 0 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-10 18:53 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, sbeller, pclouds On 03/09, Jonathan Tan wrote: > On 03/09/2017 01:07 PM, Brandon Williams wrote: > >diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh > >index b5e5a0607..585d17bad 100755 > >--- a/t/t6135-pathspec-with-attrs.sh > >+++ b/t/t6135-pathspec-with-attrs.sh > >@@ -178,4 +178,13 @@ test_expect_success 'abort on asking for wrong magic' ' > > test_must_fail git ls-files . ":(attr:!label=foo)" > > ' > > > >+test_expect_success 'check attribute list' ' > >+ cat <<-EOF >>.gitattributes && > >+ * whitespace=indent,trail,space > >+ EOF > >+ git ls-files ":(attr:whitespace=indent\,trail\,space)" >actual && > >+ git ls-files >expect && > >+ test_cmp expect actual > >+' > >+ > > test_done > > Is there a way to verify that `\,` is not escaped by the shell into `,`? You can run with GIT_TRACE=1 to see the actual string passed to git. '\,' is indeed passed to git with no problems. > > Maybe also add tests that show \ as the last character and \ > escaping another \. Done -- Brandon Williams ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2] bringing attributes to pathspecs 2017-03-09 21:07 [PATCH 0/2] bringing attributes to pathspecs Brandon Williams 2017-03-09 21:07 ` [PATCH 1/2] pathspec: allow querying for attributes Brandon Williams 2017-03-09 21:07 ` [PATCH 2/2] pathspec: allow escaped query values Brandon Williams @ 2017-03-09 21:22 ` Stefan Beller 2017-03-10 18:59 ` [PATCH v2 " Brandon Williams 3 siblings, 0 replies; 23+ messages in thread From: Stefan Beller @ 2017-03-09 21:22 UTC (permalink / raw) To: Brandon Williams; +Cc: git@vger.kernel.org, Duy Nguyen On Thu, Mar 9, 2017 at 1:07 PM, Brandon Williams <bmwill@google.com> wrote: > This small series extends the pathspec magic to allow users to specify > attributes that files must have in order for a pathspec to 'match' a file. > > One potential use for this is to allow a repository to specify attributes for a > set of files. The user can then specify that attribute as a pathspec to > perform various operations only on that set of files. One simple example: > > git ls-files -- ":(attr:text)" > > can be used to list all of the files with the 'text' attribute. Thanks for reviving these patches. I have read through both of them and they look good to me. Thanks, Stefan ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 0/2] bringing attributes to pathspecs 2017-03-09 21:07 [PATCH 0/2] bringing attributes to pathspecs Brandon Williams ` (2 preceding siblings ...) 2017-03-09 21:22 ` [PATCH 0/2] bringing attributes to pathspecs Stefan Beller @ 2017-03-10 18:59 ` Brandon Williams 2017-03-10 18:59 ` [PATCH v2 1/2] pathspec: allow querying for attributes Brandon Williams ` (2 more replies) 3 siblings, 3 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-10 18:59 UTC (permalink / raw) To: git; +Cc: Brandon Williams, sbeller, pclouds, jonathantanmy v2 addresses the comments made by Jonathan. Brandon Williams (2): pathspec: allow querying for attributes pathspec: allow escaped query values Documentation/glossary-content.txt | 21 ++++ attr.c | 17 ++++ attr.h | 1 + dir.c | 43 +++++++- pathspec.c | 163 ++++++++++++++++++++++++++++-- pathspec.h | 16 ++- t/t6135-pathspec-with-attrs.sh | 200 +++++++++++++++++++++++++++++++++++++ 7 files changed, 451 insertions(+), 10 deletions(-) create mode 100755 t/t6135-pathspec-with-attrs.sh -- 2.12.0.246.ga2ecc84866-goog ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/2] pathspec: allow querying for attributes 2017-03-10 18:59 ` [PATCH v2 " Brandon Williams @ 2017-03-10 18:59 ` Brandon Williams 2017-03-10 19:56 ` Jonathan Tan 2017-03-10 18:59 ` [PATCH v2 2/2] pathspec: allow escaped query values Brandon Williams 2017-03-13 18:23 ` [PATCH v3 0/2] bringing attributes to pathspecs Brandon Williams 2 siblings, 1 reply; 23+ messages in thread From: Brandon Williams @ 2017-03-10 18:59 UTC (permalink / raw) To: git; +Cc: Brandon Williams, sbeller, pclouds, jonathantanmy The pathspec mechanism is extended via the new ":(attr:eol=input)pattern/to/match" syntax to filter paths so that it requires paths to not just match the given pattern but also have the specified attrs attached for them to be chosen. Based on a patch by Stefan Beller <sbeller@google.com> Signed-off-by: Brandon Williams <bmwill@google.com> --- Documentation/glossary-content.txt | 21 +++++ attr.c | 17 ++++ attr.h | 1 + dir.c | 43 ++++++++- pathspec.c | 117 +++++++++++++++++++++++- pathspec.h | 16 +++- t/t6135-pathspec-with-attrs.sh | 181 +++++++++++++++++++++++++++++++++++++ 7 files changed, 387 insertions(+), 9 deletions(-) create mode 100755 t/t6135-pathspec-with-attrs.sh diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt index fc9320e59..6e991c246 100644 --- a/Documentation/glossary-content.txt +++ b/Documentation/glossary-content.txt @@ -384,6 +384,27 @@ full pathname may have special meaning: + Glob magic is incompatible with literal magic. +attr;; +After `attr:` comes a space separated list of "attribute +requirements", all of which must be met in order for the +path to be considered a match; this is in addition to the +usual non-magic pathspec pattern matching. +See linkgit:gitattributes[5]. ++ +Each of the attribute requirements for the path takes one of +these forms: + +- "`ATTR`" requires that the attribute `ATTR` be set. + +- "`-ATTR`" requires that the attribute `ATTR` be unset. + +- "`ATTR=VALUE`" requires that the attribute `ATTR` be + set to the string `VALUE`. + +- "`!ATTR`" requires that the attribute `ATTR` be + unspecified. ++ + exclude;; After a path matches any non-exclude pathspec, it will be run through all exclude pathspec (magic signature: `!` or its diff --git a/attr.c b/attr.c index 5493bff22..7e2134471 100644 --- a/attr.c +++ b/attr.c @@ -603,6 +603,23 @@ struct attr_check *attr_check_initl(const char *one, ...) return check; } +struct attr_check *attr_check_dup(const struct attr_check *check) +{ + struct attr_check *ret; + + if (!check) + return NULL; + + ret = attr_check_alloc(); + + ret->nr = check->nr; + ret->alloc = check->alloc; + ALLOC_ARRAY(ret->items, ret->nr); + COPY_ARRAY(ret->items, check->items, ret->nr); + + return ret; +} + struct attr_check_item *attr_check_append(struct attr_check *check, const struct git_attr *attr) { diff --git a/attr.h b/attr.h index 48ab3e1c2..442d464db 100644 --- a/attr.h +++ b/attr.h @@ -44,6 +44,7 @@ struct attr_check { extern struct attr_check *attr_check_alloc(void); extern struct attr_check *attr_check_initl(const char *, ...); +extern struct attr_check *attr_check_dup(const struct attr_check *check); extern struct attr_check_item *attr_check_append(struct attr_check *check, const struct git_attr *attr); diff --git a/dir.c b/dir.c index 4541f9e14..2fe7acbcf 100644 --- a/dir.c +++ b/dir.c @@ -9,6 +9,7 @@ */ #include "cache.h" #include "dir.h" +#include "attr.h" #include "refs.h" #include "wildmatch.h" #include "pathspec.h" @@ -134,7 +135,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec) PATHSPEC_LITERAL | PATHSPEC_GLOB | PATHSPEC_ICASE | - PATHSPEC_EXCLUDE); + PATHSPEC_EXCLUDE | + PATHSPEC_ATTR); for (n = 0; n < pathspec->nr; n++) { size_t i = 0, len = 0, item_len; @@ -209,6 +211,36 @@ int within_depth(const char *name, int namelen, #define DO_MATCH_DIRECTORY (1<<1) #define DO_MATCH_SUBMODULE (1<<2) +static int match_attrs(const char *name, int namelen, + const struct pathspec_item *item) +{ + int i; + + git_check_attr(name, item->attr_check); + for (i = 0; i < item->attr_match_nr; i++) { + const char *value; + int matched; + enum attr_match_mode match_mode; + + value = item->attr_check->items[i].value; + match_mode = item->attr_match[i].match_mode; + + if (ATTR_TRUE(value)) + matched = (match_mode == MATCH_SET); + else if (ATTR_FALSE(value)) + matched = (match_mode == MATCH_UNSET); + else if (ATTR_UNSET(value)) + matched = (match_mode == MATCH_UNSPECIFIED); + else + matched = (match_mode == MATCH_VALUE && + !strcmp(item->attr_match[i].value, value)); + if (!matched) + return 0; + } + + return 1; +} + /* * Does 'match' match the given name? * A match is found if @@ -261,6 +293,9 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix, strncmp(item->match, name - prefix, item->prefix)) return 0; + if (item->attr_match_nr && !match_attrs(name, namelen, item)) + return 0; + /* If the match was just the prefix, we matched */ if (!*match) return MATCHED_RECURSIVELY; @@ -339,7 +374,8 @@ static int do_match_pathspec(const struct pathspec *ps, PATHSPEC_LITERAL | PATHSPEC_GLOB | PATHSPEC_ICASE | - PATHSPEC_EXCLUDE); + PATHSPEC_EXCLUDE | + PATHSPEC_ATTR); if (!ps->nr) { if (!ps->recursive || @@ -1361,7 +1397,8 @@ static int simplify_away(const char *path, int pathlen, PATHSPEC_LITERAL | PATHSPEC_GLOB | PATHSPEC_ICASE | - PATHSPEC_EXCLUDE); + PATHSPEC_EXCLUDE | + PATHSPEC_ATTR); for (i = 0; i < pathspec->nr; i++) { const struct pathspec_item *item = &pathspec->items[i]; diff --git a/pathspec.c b/pathspec.c index b961f00c8..7cd5f6e3d 100644 --- a/pathspec.c +++ b/pathspec.c @@ -1,6 +1,7 @@ #include "cache.h" #include "dir.h" #include "pathspec.h" +#include "attr.h" /* * Finds which of the given pathspecs match items in the index. @@ -72,6 +73,7 @@ static struct pathspec_magic { { PATHSPEC_GLOB, '\0', "glob" }, { PATHSPEC_ICASE, '\0', "icase" }, { PATHSPEC_EXCLUDE, '!', "exclude" }, + { PATHSPEC_ATTR, '\0', "attr" }, }; static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) @@ -87,6 +89,74 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) strbuf_addf(sb, ",prefix:%d)", prefixlen); } +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value) +{ + struct string_list_item *si; + struct string_list list = STRING_LIST_INIT_DUP; + + if (item->attr_check) + die(_("Only one 'attr:' specification is allowed.")); + + if (!value || !*value) + die(_("attr spec must not be empty")); + + string_list_split(&list, value, ' ', -1); + string_list_remove_empty_items(&list, 0); + + item->attr_check = attr_check_alloc(); + ALLOC_GROW(item->attr_match, + list.nr, + item->attr_match_alloc); + + for_each_string_list_item(si, &list) { + size_t attr_len; + char *attr_name; + const struct git_attr *a; + + int j = item->attr_match_nr++; + const char *attr = si->string; + struct attr_match *am = &item->attr_match[j]; + + switch (*attr) { + case '!': + am->match_mode = MATCH_UNSPECIFIED; + attr++; + attr_len = strlen(attr); + break; + case '-': + am->match_mode = MATCH_UNSET; + attr++; + attr_len = strlen(attr); + break; + default: + attr_len = strcspn(attr, "="); + if (attr[attr_len] != '=') + am->match_mode = MATCH_SET; + else { + am->match_mode = MATCH_VALUE; + am->value = xstrdup(&attr[attr_len + 1]); + if (strchr(am->value, '\\')) + die(_("attr spec values must not contain backslashes")); + } + break; + } + + attr_name = xmemdupz(attr, attr_len); + a = git_attr(attr_name); + if (!a) + die(_("invalid attribute name %s"), attr_name); + + attr_check_append(item->attr_check, a); + + free(attr_name); + } + + if (item->attr_check->nr != item->attr_match_nr) + die("BUG: should have same number of entries"); + + string_list_clear(&list, 0); +} + static inline int get_literal_global(void) { static int literal = -1; @@ -164,6 +234,7 @@ static int get_global_magic(int element_magic) * returns the position in 'elem' after all magic has been parsed */ static const char *parse_long_magic(unsigned *magic, int *prefix_len, + struct pathspec_item *item, const char *elem) { const char *pos; @@ -189,6 +260,14 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len, continue; } + if (starts_with(pos, "attr:")) { + char *attr_body = xmemdupz(pos + 5, len - 5); + parse_pathspec_attr_match(item, attr_body); + *magic |= PATHSPEC_ATTR; + free(attr_body); + continue; + } + for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) { if (strlen(pathspec_magic[i].name) == len && !strncmp(pathspec_magic[i].name, pos, len)) { @@ -252,13 +331,14 @@ static const char *parse_short_magic(unsigned *magic, const char *elem) } static const char *parse_element_magic(unsigned *magic, int *prefix_len, + struct pathspec_item *item, const char *elem) { if (elem[0] != ':' || get_literal_global()) return elem; /* nothing to do */ else if (elem[1] == '(') /* longhand */ - return parse_long_magic(magic, prefix_len, elem); + return parse_long_magic(magic, prefix_len, item, elem); else /* shorthand */ return parse_short_magic(magic, elem); @@ -335,12 +415,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags, char *match; int pathspec_prefix = -1; + item->attr_check = NULL; + item->attr_match = NULL; + item->attr_match_nr = 0; + item->attr_match_alloc = 0; + /* PATHSPEC_LITERAL_PATH ignores magic */ if (flags & PATHSPEC_LITERAL_PATH) { magic = PATHSPEC_LITERAL; } else { copyfrom = parse_element_magic(&element_magic, &pathspec_prefix, + item, elt); magic |= element_magic; magic |= get_global_magic(element_magic); @@ -565,26 +651,47 @@ void parse_pathspec(struct pathspec *pathspec, void copy_pathspec(struct pathspec *dst, const struct pathspec *src) { - int i; + int i, j; *dst = *src; ALLOC_ARRAY(dst->items, dst->nr); COPY_ARRAY(dst->items, src->items, dst->nr); for (i = 0; i < dst->nr; i++) { - dst->items[i].match = xstrdup(src->items[i].match); - dst->items[i].original = xstrdup(src->items[i].original); + struct pathspec_item *d = &dst->items[i]; + struct pathspec_item *s = &src->items[i]; + + d->match = xstrdup(s->match); + d->original = xstrdup(s->original); + + ALLOC_ARRAY(d->attr_match, d->attr_match_nr); + COPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr); + for (j = 0; j < d->attr_match_nr; j++) { + const char *value = s->attr_match[j].value; + if (value) + d->attr_match[j].value = xstrdup(value); + } + + d->attr_check = attr_check_dup(s->attr_check); } } void clear_pathspec(struct pathspec *pathspec) { - int i; + int i, j; for (i = 0; i < pathspec->nr; i++) { free(pathspec->items[i].match); free(pathspec->items[i].original); + + for (j = 0; j < pathspec->items[j].attr_match_nr; j++) + free(pathspec->items[i].attr_match[j].value); + free(pathspec->items[i].attr_match); + + if (pathspec->items[i].attr_check) + attr_check_free(pathspec->items[i].attr_check); } + free(pathspec->items); pathspec->items = NULL; pathspec->nr = 0; diff --git a/pathspec.h b/pathspec.h index 49fd823dd..83625f006 100644 --- a/pathspec.h +++ b/pathspec.h @@ -8,13 +8,15 @@ #define PATHSPEC_GLOB (1<<3) #define PATHSPEC_ICASE (1<<4) #define PATHSPEC_EXCLUDE (1<<5) +#define PATHSPEC_ATTR (1<<6) #define PATHSPEC_ALL_MAGIC \ (PATHSPEC_FROMTOP | \ PATHSPEC_MAXDEPTH | \ PATHSPEC_LITERAL | \ PATHSPEC_GLOB | \ PATHSPEC_ICASE | \ - PATHSPEC_EXCLUDE) + PATHSPEC_EXCLUDE | \ + PATHSPEC_ATTR) #define PATHSPEC_ONESTAR 1 /* the pathspec pattern satisfies GFNM_ONESTAR */ @@ -31,6 +33,18 @@ struct pathspec { int len, prefix; int nowildcard_len; int flags; + int attr_match_nr; + int attr_match_alloc; + struct attr_match { + char *value; + enum attr_match_mode { + MATCH_SET, + MATCH_UNSET, + MATCH_VALUE, + MATCH_UNSPECIFIED + } match_mode; + } *attr_match; + struct attr_check *attr_check; } *items; }; diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh new file mode 100755 index 000000000..b5e5a0607 --- /dev/null +++ b/t/t6135-pathspec-with-attrs.sh @@ -0,0 +1,181 @@ +#!/bin/sh + +test_description='test labels in pathspecs' +. ./test-lib.sh + +test_expect_success 'setup a tree' ' + cat <<-EOF >expect && + fileA + fileAB + fileAC + fileB + fileBC + fileC + fileNoLabel + fileSetLabel + fileUnsetLabel + fileValue + fileWrongLabel + sub/fileA + sub/fileAB + sub/fileAC + sub/fileB + sub/fileBC + sub/fileC + sub/fileNoLabel + sub/fileSetLabel + sub/fileUnsetLabel + sub/fileValue + sub/fileWrongLabel + EOF + mkdir sub && + while read path + do + : >$path && + git add $path || return 1 + done <expect && + git commit -m "initial commit" && + git ls-files >actual && + test_cmp expect actual +' + +test_expect_success 'pathspec with no attr' ' + test_must_fail git ls-files ":(attr:)" +' + +test_expect_success 'pathspec with labels and non existent .gitattributes' ' + git ls-files ":(attr:label)" >actual && + test_must_be_empty actual +' + +test_expect_success 'setup .gitattributes' ' + cat <<-EOF >.gitattributes && + fileA labelA + fileB labelB + fileC labelC + fileAB labelA labelB + fileAC labelA labelC + fileBC labelB labelC + fileUnsetLabel -label + fileSetLabel label + fileValue label=foo + fileWrongLabel label☺ + EOF + git add .gitattributes && + git commit -m "add attributes" +' + +test_expect_success 'check specific set attr' ' + cat <<-EOF >expect && + fileSetLabel + sub/fileSetLabel + EOF + git ls-files ":(attr:label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check specific unset attr' ' + cat <<-EOF >expect && + fileUnsetLabel + sub/fileUnsetLabel + EOF + git ls-files ":(attr:-label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check specific value attr' ' + cat <<-EOF >expect && + fileValue + sub/fileValue + EOF + git ls-files ":(attr:label=foo)" >actual && + test_cmp expect actual && + git ls-files ":(attr:label=bar)" >actual && + test_must_be_empty actual +' + +test_expect_success 'check unspecified attr' ' + cat <<-EOF >expect && + .gitattributes + fileA + fileAB + fileAC + fileB + fileBC + fileC + fileNoLabel + fileWrongLabel + sub/fileA + sub/fileAB + sub/fileAC + sub/fileB + sub/fileBC + sub/fileC + sub/fileNoLabel + sub/fileWrongLabel + EOF + git ls-files ":(attr:!label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check multiple unspecified attr' ' + cat <<-EOF >expect && + .gitattributes + fileC + fileNoLabel + fileWrongLabel + sub/fileC + sub/fileNoLabel + sub/fileWrongLabel + EOF + git ls-files ":(attr:!labelB !labelA !label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check label with more labels but excluded path' ' + cat <<-EOF >expect && + fileAB + fileB + fileBC + EOF + git ls-files ":(attr:labelB)" ":(exclude)sub/" >actual && + test_cmp expect actual +' + +test_expect_success 'check label excluding other labels' ' + cat <<-EOF >expect && + fileAB + fileB + fileBC + sub/fileAB + sub/fileB + EOF + git ls-files ":(attr:labelB)" ":(exclude,attr:labelC)sub/" >actual && + test_cmp expect actual +' + +test_expect_success 'fail on multiple attr specifiers in one pathspec item' ' + test_must_fail git ls-files . ":(attr:labelB,attr:labelC)" 2>actual && + test_i18ngrep "Only one" actual +' + +test_expect_success 'fail if attr magic is used places not implemented' ' + # The main purpose of this test is to check that we actually fail + # when you attempt to use attr magic in commands that do not implement + # attr magic. This test does not advocate git-add to stay that way, + # though, but git-add is convenient as it has its own internal pathspec + # parsing. + test_must_fail git add ":(attr:labelB)" 2>actual && + test_i18ngrep "unsupported magic" actual +' + +test_expect_success 'abort on giving invalid label on the command line' ' + test_must_fail git ls-files . ":(attr:☺)" +' + +test_expect_success 'abort on asking for wrong magic' ' + test_must_fail git ls-files . ":(attr:-label=foo)" && + test_must_fail git ls-files . ":(attr:!label=foo)" +' + +test_done -- 2.12.0.246.ga2ecc84866-goog ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pathspec: allow querying for attributes 2017-03-10 18:59 ` [PATCH v2 1/2] pathspec: allow querying for attributes Brandon Williams @ 2017-03-10 19:56 ` Jonathan Tan 2017-03-11 0:28 ` Brandon Williams 0 siblings, 1 reply; 23+ messages in thread From: Jonathan Tan @ 2017-03-10 19:56 UTC (permalink / raw) To: Brandon Williams, git; +Cc: sbeller, pclouds Thanks - I don't think I have any more comments on this patch set after these. On 03/10/2017 10:59 AM, Brandon Williams wrote: > diff --git a/pathspec.c b/pathspec.c > index b961f00c8..7cd5f6e3d 100644 > --- a/pathspec.c > +++ b/pathspec.c > @@ -87,6 +89,74 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) > strbuf_addf(sb, ",prefix:%d)", prefixlen); > } > > +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value) > +{ > + struct string_list_item *si; > + struct string_list list = STRING_LIST_INIT_DUP; > + > + if (item->attr_check) > + die(_("Only one 'attr:' specification is allowed.")); > + > + if (!value || !*value) > + die(_("attr spec must not be empty")); > + > + string_list_split(&list, value, ' ', -1); > + string_list_remove_empty_items(&list, 0); > + > + item->attr_check = attr_check_alloc(); > + ALLOC_GROW(item->attr_match, > + list.nr, > + item->attr_match_alloc); If item->attr_match always starts empty, then I think an xmalloc or xcalloc suffices (and we don't need item->attr_match_alloc anymore). We should probably also check item->attr_match above - that is, `if (item->attr_check || item->attr_match)`. > + > + for_each_string_list_item(si, &list) { > + size_t attr_len; > + char *attr_name; > + const struct git_attr *a; > + > + int j = item->attr_match_nr++; > + const char *attr = si->string; > + struct attr_match *am = &item->attr_match[j]; > + > + switch (*attr) { > + case '!': > + am->match_mode = MATCH_UNSPECIFIED; > + attr++; > + attr_len = strlen(attr); > + break; > + case '-': > + am->match_mode = MATCH_UNSET; > + attr++; > + attr_len = strlen(attr); > + break; > + default: > + attr_len = strcspn(attr, "="); > + if (attr[attr_len] != '=') > + am->match_mode = MATCH_SET; > + else { > + am->match_mode = MATCH_VALUE; > + am->value = xstrdup(&attr[attr_len + 1]); > + if (strchr(am->value, '\\')) > + die(_("attr spec values must not contain backslashes")); > + } > + break; > + } > + > + attr_name = xmemdupz(attr, attr_len); > + a = git_attr(attr_name); > + if (!a) > + die(_("invalid attribute name %s"), attr_name); > + > + attr_check_append(item->attr_check, a); > + > + free(attr_name); > + } > + > + if (item->attr_check->nr != item->attr_match_nr) > + die("BUG: should have same number of entries"); I think such postcondition checks are usually not worth it, but others might differ. > + > + string_list_clear(&list, 0); > +} > + > static inline int get_literal_global(void) > { > static int literal = -1; ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pathspec: allow querying for attributes 2017-03-10 19:56 ` Jonathan Tan @ 2017-03-11 0:28 ` Brandon Williams 0 siblings, 0 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-11 0:28 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, sbeller, pclouds On 03/10, Jonathan Tan wrote: > Thanks - I don't think I have any more comments on this patch set > after these. > > On 03/10/2017 10:59 AM, Brandon Williams wrote: > >diff --git a/pathspec.c b/pathspec.c > >index b961f00c8..7cd5f6e3d 100644 > >--- a/pathspec.c > >+++ b/pathspec.c > >@@ -87,6 +89,74 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) > > strbuf_addf(sb, ",prefix:%d)", prefixlen); > > } > > > >+static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value) > >+{ > >+ struct string_list_item *si; > >+ struct string_list list = STRING_LIST_INIT_DUP; > >+ > >+ if (item->attr_check) > >+ die(_("Only one 'attr:' specification is allowed.")); > >+ > >+ if (!value || !*value) > >+ die(_("attr spec must not be empty")); > >+ > >+ string_list_split(&list, value, ' ', -1); > >+ string_list_remove_empty_items(&list, 0); > >+ > >+ item->attr_check = attr_check_alloc(); > >+ ALLOC_GROW(item->attr_match, > >+ list.nr, > >+ item->attr_match_alloc); > > If item->attr_match always starts empty, then I think an xmalloc or > xcalloc suffices (and we don't need item->attr_match_alloc anymore). > > We should probably also check item->attr_match above - that is, `if > (item->attr_check || item->attr_match)`. Correct, I'll make these changes. > > >+ > >+ for_each_string_list_item(si, &list) { > >+ size_t attr_len; > >+ char *attr_name; > >+ const struct git_attr *a; > >+ > >+ int j = item->attr_match_nr++; > >+ const char *attr = si->string; > >+ struct attr_match *am = &item->attr_match[j]; > >+ > >+ switch (*attr) { > >+ case '!': > >+ am->match_mode = MATCH_UNSPECIFIED; > >+ attr++; > >+ attr_len = strlen(attr); > >+ break; > >+ case '-': > >+ am->match_mode = MATCH_UNSET; > >+ attr++; > >+ attr_len = strlen(attr); > >+ break; > >+ default: > >+ attr_len = strcspn(attr, "="); > >+ if (attr[attr_len] != '=') > >+ am->match_mode = MATCH_SET; > >+ else { > >+ am->match_mode = MATCH_VALUE; > >+ am->value = xstrdup(&attr[attr_len + 1]); > >+ if (strchr(am->value, '\\')) > >+ die(_("attr spec values must not contain backslashes")); > >+ } > >+ break; > >+ } > >+ > >+ attr_name = xmemdupz(attr, attr_len); > >+ a = git_attr(attr_name); > >+ if (!a) > >+ die(_("invalid attribute name %s"), attr_name); > >+ > >+ attr_check_append(item->attr_check, a); > >+ > >+ free(attr_name); > >+ } > >+ > >+ if (item->attr_check->nr != item->attr_match_nr) > >+ die("BUG: should have same number of entries"); > > I think such postcondition checks are usually not worth it, but > others might differ. yeah probably not, but its just an assert check for just in case so I'll leave it in. > > >+ > >+ string_list_clear(&list, 0); > >+} > >+ > > static inline int get_literal_global(void) > > { > > static int literal = -1; -- Brandon Williams ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 2/2] pathspec: allow escaped query values 2017-03-10 18:59 ` [PATCH v2 " Brandon Williams 2017-03-10 18:59 ` [PATCH v2 1/2] pathspec: allow querying for attributes Brandon Williams @ 2017-03-10 18:59 ` Brandon Williams 2017-03-13 18:23 ` [PATCH v3 0/2] bringing attributes to pathspecs Brandon Williams 2 siblings, 0 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-10 18:59 UTC (permalink / raw) To: git; +Cc: Brandon Williams, sbeller, pclouds, jonathantanmy In our own .gitattributes file we have attributes such as: *.[ch] whitespace=indent,trail,space When querying for attributes we want to be able to ask for the exact value, i.e. git ls-files :(attr:whitespace=indent,trail,space) should work, but the commas are used in the attr magic to introduce the next attr, such that this query currently fails with fatal: Invalid pathspec magic 'trail' in ':(attr:whitespace=indent,trail,space)' This change allows escaping characters by a backslash, such that the query git ls-files :(attr:whitespace=indent\,trail\,space) will match all path that have the value "indent,trail,space" for the whitespace attribute. To accomplish this, we need to modify two places. First `parse_long_magic` needs to not stop early upon seeing a comma or closing paren that is escaped. As a second step we need to remove any escaping from the attr value. Based on a patch by Stefan Beller <sbeller@google.com> Signed-off-by: Brandon Williams <bmwill@google.com> --- pathspec.c | 52 ++++++++++++++++++++++++++++++++++++++---- t/t6135-pathspec-with-attrs.sh | 19 +++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/pathspec.c b/pathspec.c index 7cd5f6e3d..d7956f6bf 100644 --- a/pathspec.c +++ b/pathspec.c @@ -89,6 +89,51 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) strbuf_addf(sb, ",prefix:%d)", prefixlen); } +static size_t strcspn_escaped(const char *s, const char *stop) +{ + const char *i; + + for (i = s; *i; i++) { + /* skip the escaped character */ + if (i[0] == '\\' && i[1]) { + i++; + continue; + } + + if (strchr(stop, *i)) + break; + } + return i - s; +} + +static inline int invalid_value_char(const char ch) +{ + if (isalnum(ch) || strchr(",-_", ch)) + return 0; + return -1; +} + +static char *attr_value_unescape(const char *value) +{ + const char *src; + char *dst, *ret; + + ret = xmallocz(strlen(value)); + for (src = value, dst = ret; *src; src++, dst++) { + if (*src == '\\') { + if (!src[1]) + die(_("Escape character '\\' not allowed as " + "last character in attr value")); + src++; + } + if (invalid_value_char(*src)) + die("cannot use '%c' for value matching", *src); + *dst = *src; + } + *dst = '\0'; + return ret; +} + static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value) { struct string_list_item *si; @@ -133,10 +178,9 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va if (attr[attr_len] != '=') am->match_mode = MATCH_SET; else { + const char *v = &attr[attr_len + 1]; am->match_mode = MATCH_VALUE; - am->value = xstrdup(&attr[attr_len + 1]); - if (strchr(am->value, '\\')) - die(_("attr spec values must not contain backslashes")); + am->value = attr_value_unescape(v); } break; } @@ -241,7 +285,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len, const char *nextat; for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) { - size_t len = strcspn(pos, ",)"); + size_t len = strcspn_escaped(pos, ",)"); int i; if (pos[len] == ',') diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh index b5e5a0607..f60af29a9 100755 --- a/t/t6135-pathspec-with-attrs.sh +++ b/t/t6135-pathspec-with-attrs.sh @@ -178,4 +178,23 @@ test_expect_success 'abort on asking for wrong magic' ' test_must_fail git ls-files . ":(attr:!label=foo)" ' +test_expect_success 'check attribute list' ' + cat <<-EOF >>.gitattributes && + * whitespace=indent,trail,space + EOF + git ls-files ":(attr:whitespace=indent\,trail\,space)" >actual && + git ls-files >expect && + test_cmp expect actual +' + +test_expect_success 'backslash cannot be the last character' ' + test_must_fail git ls-files ":(attr:label=foo\\ labelA=bar)" 2>actual && + test_i18ngrep "not allowed as last character in attr value" actual +' + +test_expect_success 'backslash cannot be used as a value' ' + test_must_fail git ls-files ":(attr:label=f\\\oo)" 2>actual && + test_i18ngrep "for value matching" actual +' + test_done -- 2.12.0.246.ga2ecc84866-goog ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 0/2] bringing attributes to pathspecs 2017-03-10 18:59 ` [PATCH v2 " Brandon Williams 2017-03-10 18:59 ` [PATCH v2 1/2] pathspec: allow querying for attributes Brandon Williams 2017-03-10 18:59 ` [PATCH v2 2/2] pathspec: allow escaped query values Brandon Williams @ 2017-03-13 18:23 ` Brandon Williams 2017-03-13 18:23 ` [PATCH v3 1/2] pathspec: allow querying for attributes Brandon Williams ` (3 more replies) 2 siblings, 4 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-13 18:23 UTC (permalink / raw) To: git; +Cc: Brandon Williams, sbeller, pclouds, jonathantanmy v3 fixes some nits in style in the test script (using <<-\EOF instead of <<-EOF) as well as fixing a few other minor things reported by Junio and Jonathan. Brandon Williams (2): pathspec: allow querying for attributes pathspec: allow escaped query values Documentation/glossary-content.txt | 21 ++++ attr.c | 17 ++++ attr.h | 1 + dir.c | 43 +++++++- pathspec.c | 159 +++++++++++++++++++++++++++-- pathspec.h | 15 ++- t/t6135-pathspec-with-attrs.sh | 200 +++++++++++++++++++++++++++++++++++++ 7 files changed, 446 insertions(+), 10 deletions(-) create mode 100755 t/t6135-pathspec-with-attrs.sh --- interdiff with bw/attr-pathspec diff --git a/pathspec.c b/pathspec.c index d7956f6bf..303efda83 100644 --- a/pathspec.c +++ b/pathspec.c @@ -139,7 +139,7 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va struct string_list_item *si; struct string_list list = STRING_LIST_INIT_DUP; - if (item->attr_check) + if (item->attr_check || item->attr_match) die(_("Only one 'attr:' specification is allowed.")); if (!value || !*value) @@ -149,9 +149,7 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va string_list_remove_empty_items(&list, 0); item->attr_check = attr_check_alloc(); - ALLOC_GROW(item->attr_match, - list.nr, - item->attr_match_alloc); + item->attr_match = xcalloc(list.nr, sizeof(struct attr_match)); for_each_string_list_item(si, &list) { size_t attr_len; @@ -462,7 +460,6 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags, item->attr_check = NULL; item->attr_match = NULL; item->attr_match_nr = 0; - item->attr_match_alloc = 0; /* PATHSPEC_LITERAL_PATH ignores magic */ if (flags & PATHSPEC_LITERAL_PATH) { @@ -712,8 +709,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src) COPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr); for (j = 0; j < d->attr_match_nr; j++) { const char *value = s->attr_match[j].value; - if (value) - d->attr_match[j].value = xstrdup(value); + d->attr_match[j].value = xstrdup_or_null(value); } d->attr_check = attr_check_dup(s->attr_check); diff --git a/pathspec.h b/pathspec.h index 83625f006..55e976972 100644 --- a/pathspec.h +++ b/pathspec.h @@ -34,7 +34,6 @@ struct pathspec { int nowildcard_len; int flags; int attr_match_nr; - int attr_match_alloc; struct attr_match { char *value; enum attr_match_mode { diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh index f60af29a9..77b8cef66 100755 --- a/t/t6135-pathspec-with-attrs.sh +++ b/t/t6135-pathspec-with-attrs.sh @@ -4,7 +4,7 @@ test_description='test labels in pathspecs' . ./test-lib.sh test_expect_success 'setup a tree' ' - cat <<-EOF >expect && + cat <<-\EOF >expect && fileA fileAB fileAC @@ -49,7 +49,7 @@ test_expect_success 'pathspec with labels and non existent .gitattributes' ' ' test_expect_success 'setup .gitattributes' ' - cat <<-EOF >.gitattributes && + cat <<-\EOF >.gitattributes && fileA labelA fileB labelB fileC labelC @@ -66,7 +66,7 @@ test_expect_success 'setup .gitattributes' ' ' test_expect_success 'check specific set attr' ' - cat <<-EOF >expect && + cat <<-\EOF >expect && fileSetLabel sub/fileSetLabel EOF @@ -75,7 +75,7 @@ test_expect_success 'check specific set attr' ' ' test_expect_success 'check specific unset attr' ' - cat <<-EOF >expect && + cat <<-\EOF >expect && fileUnsetLabel sub/fileUnsetLabel EOF @@ -84,7 +84,7 @@ test_expect_success 'check specific unset attr' ' ' test_expect_success 'check specific value attr' ' - cat <<-EOF >expect && + cat <<-\EOF >expect && fileValue sub/fileValue EOF @@ -95,7 +95,7 @@ test_expect_success 'check specific value attr' ' ' test_expect_success 'check unspecified attr' ' - cat <<-EOF >expect && + cat <<-\EOF >expect && .gitattributes fileA fileAB @@ -119,7 +119,7 @@ test_expect_success 'check unspecified attr' ' ' test_expect_success 'check multiple unspecified attr' ' - cat <<-EOF >expect && + cat <<-\EOF >expect && .gitattributes fileC fileNoLabel @@ -133,7 +133,7 @@ test_expect_success 'check multiple unspecified attr' ' ' test_expect_success 'check label with more labels but excluded path' ' - cat <<-EOF >expect && + cat <<-\EOF >expect && fileAB fileB fileBC @@ -143,7 +143,7 @@ test_expect_success 'check label with more labels but excluded path' ' ' test_expect_success 'check label excluding other labels' ' - cat <<-EOF >expect && + cat <<-\EOF >expect && fileAB fileB fileBC -- 2.12.0.246.ga2ecc84866-goog ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 1/2] pathspec: allow querying for attributes 2017-03-13 18:23 ` [PATCH v3 0/2] bringing attributes to pathspecs Brandon Williams @ 2017-03-13 18:23 ` Brandon Williams 2017-03-13 18:23 ` [PATCH v3 2/2] pathspec: allow escaped query values Brandon Williams ` (2 subsequent siblings) 3 siblings, 0 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-13 18:23 UTC (permalink / raw) To: git; +Cc: Brandon Williams, sbeller, pclouds, jonathantanmy The pathspec mechanism is extended via the new ":(attr:eol=input)pattern/to/match" syntax to filter paths so that it requires paths to not just match the given pattern but also have the specified attrs attached for them to be chosen. Based on a patch by Stefan Beller <sbeller@google.com> Signed-off-by: Brandon Williams <bmwill@google.com> --- Documentation/glossary-content.txt | 21 +++++ attr.c | 17 ++++ attr.h | 1 + dir.c | 43 ++++++++- pathspec.c | 113 ++++++++++++++++++++++- pathspec.h | 15 ++- t/t6135-pathspec-with-attrs.sh | 181 +++++++++++++++++++++++++++++++++++++ 7 files changed, 382 insertions(+), 9 deletions(-) create mode 100755 t/t6135-pathspec-with-attrs.sh diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt index fc9320e59..6e991c246 100644 --- a/Documentation/glossary-content.txt +++ b/Documentation/glossary-content.txt @@ -384,6 +384,27 @@ full pathname may have special meaning: + Glob magic is incompatible with literal magic. +attr;; +After `attr:` comes a space separated list of "attribute +requirements", all of which must be met in order for the +path to be considered a match; this is in addition to the +usual non-magic pathspec pattern matching. +See linkgit:gitattributes[5]. ++ +Each of the attribute requirements for the path takes one of +these forms: + +- "`ATTR`" requires that the attribute `ATTR` be set. + +- "`-ATTR`" requires that the attribute `ATTR` be unset. + +- "`ATTR=VALUE`" requires that the attribute `ATTR` be + set to the string `VALUE`. + +- "`!ATTR`" requires that the attribute `ATTR` be + unspecified. ++ + exclude;; After a path matches any non-exclude pathspec, it will be run through all exclude pathspec (magic signature: `!` or its diff --git a/attr.c b/attr.c index 5493bff22..7e2134471 100644 --- a/attr.c +++ b/attr.c @@ -603,6 +603,23 @@ struct attr_check *attr_check_initl(const char *one, ...) return check; } +struct attr_check *attr_check_dup(const struct attr_check *check) +{ + struct attr_check *ret; + + if (!check) + return NULL; + + ret = attr_check_alloc(); + + ret->nr = check->nr; + ret->alloc = check->alloc; + ALLOC_ARRAY(ret->items, ret->nr); + COPY_ARRAY(ret->items, check->items, ret->nr); + + return ret; +} + struct attr_check_item *attr_check_append(struct attr_check *check, const struct git_attr *attr) { diff --git a/attr.h b/attr.h index 48ab3e1c2..442d464db 100644 --- a/attr.h +++ b/attr.h @@ -44,6 +44,7 @@ struct attr_check { extern struct attr_check *attr_check_alloc(void); extern struct attr_check *attr_check_initl(const char *, ...); +extern struct attr_check *attr_check_dup(const struct attr_check *check); extern struct attr_check_item *attr_check_append(struct attr_check *check, const struct git_attr *attr); diff --git a/dir.c b/dir.c index 4541f9e14..2fe7acbcf 100644 --- a/dir.c +++ b/dir.c @@ -9,6 +9,7 @@ */ #include "cache.h" #include "dir.h" +#include "attr.h" #include "refs.h" #include "wildmatch.h" #include "pathspec.h" @@ -134,7 +135,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec) PATHSPEC_LITERAL | PATHSPEC_GLOB | PATHSPEC_ICASE | - PATHSPEC_EXCLUDE); + PATHSPEC_EXCLUDE | + PATHSPEC_ATTR); for (n = 0; n < pathspec->nr; n++) { size_t i = 0, len = 0, item_len; @@ -209,6 +211,36 @@ int within_depth(const char *name, int namelen, #define DO_MATCH_DIRECTORY (1<<1) #define DO_MATCH_SUBMODULE (1<<2) +static int match_attrs(const char *name, int namelen, + const struct pathspec_item *item) +{ + int i; + + git_check_attr(name, item->attr_check); + for (i = 0; i < item->attr_match_nr; i++) { + const char *value; + int matched; + enum attr_match_mode match_mode; + + value = item->attr_check->items[i].value; + match_mode = item->attr_match[i].match_mode; + + if (ATTR_TRUE(value)) + matched = (match_mode == MATCH_SET); + else if (ATTR_FALSE(value)) + matched = (match_mode == MATCH_UNSET); + else if (ATTR_UNSET(value)) + matched = (match_mode == MATCH_UNSPECIFIED); + else + matched = (match_mode == MATCH_VALUE && + !strcmp(item->attr_match[i].value, value)); + if (!matched) + return 0; + } + + return 1; +} + /* * Does 'match' match the given name? * A match is found if @@ -261,6 +293,9 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix, strncmp(item->match, name - prefix, item->prefix)) return 0; + if (item->attr_match_nr && !match_attrs(name, namelen, item)) + return 0; + /* If the match was just the prefix, we matched */ if (!*match) return MATCHED_RECURSIVELY; @@ -339,7 +374,8 @@ static int do_match_pathspec(const struct pathspec *ps, PATHSPEC_LITERAL | PATHSPEC_GLOB | PATHSPEC_ICASE | - PATHSPEC_EXCLUDE); + PATHSPEC_EXCLUDE | + PATHSPEC_ATTR); if (!ps->nr) { if (!ps->recursive || @@ -1361,7 +1397,8 @@ static int simplify_away(const char *path, int pathlen, PATHSPEC_LITERAL | PATHSPEC_GLOB | PATHSPEC_ICASE | - PATHSPEC_EXCLUDE); + PATHSPEC_EXCLUDE | + PATHSPEC_ATTR); for (i = 0; i < pathspec->nr; i++) { const struct pathspec_item *item = &pathspec->items[i]; diff --git a/pathspec.c b/pathspec.c index b961f00c8..608511c52 100644 --- a/pathspec.c +++ b/pathspec.c @@ -1,6 +1,7 @@ #include "cache.h" #include "dir.h" #include "pathspec.h" +#include "attr.h" /* * Finds which of the given pathspecs match items in the index. @@ -72,6 +73,7 @@ static struct pathspec_magic { { PATHSPEC_GLOB, '\0', "glob" }, { PATHSPEC_ICASE, '\0', "icase" }, { PATHSPEC_EXCLUDE, '!', "exclude" }, + { PATHSPEC_ATTR, '\0', "attr" }, }; static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) @@ -87,6 +89,72 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) strbuf_addf(sb, ",prefix:%d)", prefixlen); } +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value) +{ + struct string_list_item *si; + struct string_list list = STRING_LIST_INIT_DUP; + + if (item->attr_check || item->attr_match) + die(_("Only one 'attr:' specification is allowed.")); + + if (!value || !*value) + die(_("attr spec must not be empty")); + + string_list_split(&list, value, ' ', -1); + string_list_remove_empty_items(&list, 0); + + item->attr_check = attr_check_alloc(); + item->attr_match = xcalloc(list.nr, sizeof(struct attr_match)); + + for_each_string_list_item(si, &list) { + size_t attr_len; + char *attr_name; + const struct git_attr *a; + + int j = item->attr_match_nr++; + const char *attr = si->string; + struct attr_match *am = &item->attr_match[j]; + + switch (*attr) { + case '!': + am->match_mode = MATCH_UNSPECIFIED; + attr++; + attr_len = strlen(attr); + break; + case '-': + am->match_mode = MATCH_UNSET; + attr++; + attr_len = strlen(attr); + break; + default: + attr_len = strcspn(attr, "="); + if (attr[attr_len] != '=') + am->match_mode = MATCH_SET; + else { + am->match_mode = MATCH_VALUE; + am->value = xstrdup(&attr[attr_len + 1]); + if (strchr(am->value, '\\')) + die(_("attr spec values must not contain backslashes")); + } + break; + } + + attr_name = xmemdupz(attr, attr_len); + a = git_attr(attr_name); + if (!a) + die(_("invalid attribute name %s"), attr_name); + + attr_check_append(item->attr_check, a); + + free(attr_name); + } + + if (item->attr_check->nr != item->attr_match_nr) + die("BUG: should have same number of entries"); + + string_list_clear(&list, 0); +} + static inline int get_literal_global(void) { static int literal = -1; @@ -164,6 +232,7 @@ static int get_global_magic(int element_magic) * returns the position in 'elem' after all magic has been parsed */ static const char *parse_long_magic(unsigned *magic, int *prefix_len, + struct pathspec_item *item, const char *elem) { const char *pos; @@ -189,6 +258,14 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len, continue; } + if (starts_with(pos, "attr:")) { + char *attr_body = xmemdupz(pos + 5, len - 5); + parse_pathspec_attr_match(item, attr_body); + *magic |= PATHSPEC_ATTR; + free(attr_body); + continue; + } + for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) { if (strlen(pathspec_magic[i].name) == len && !strncmp(pathspec_magic[i].name, pos, len)) { @@ -252,13 +329,14 @@ static const char *parse_short_magic(unsigned *magic, const char *elem) } static const char *parse_element_magic(unsigned *magic, int *prefix_len, + struct pathspec_item *item, const char *elem) { if (elem[0] != ':' || get_literal_global()) return elem; /* nothing to do */ else if (elem[1] == '(') /* longhand */ - return parse_long_magic(magic, prefix_len, elem); + return parse_long_magic(magic, prefix_len, item, elem); else /* shorthand */ return parse_short_magic(magic, elem); @@ -335,12 +413,17 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags, char *match; int pathspec_prefix = -1; + item->attr_check = NULL; + item->attr_match = NULL; + item->attr_match_nr = 0; + /* PATHSPEC_LITERAL_PATH ignores magic */ if (flags & PATHSPEC_LITERAL_PATH) { magic = PATHSPEC_LITERAL; } else { copyfrom = parse_element_magic(&element_magic, &pathspec_prefix, + item, elt); magic |= element_magic; magic |= get_global_magic(element_magic); @@ -565,26 +648,46 @@ void parse_pathspec(struct pathspec *pathspec, void copy_pathspec(struct pathspec *dst, const struct pathspec *src) { - int i; + int i, j; *dst = *src; ALLOC_ARRAY(dst->items, dst->nr); COPY_ARRAY(dst->items, src->items, dst->nr); for (i = 0; i < dst->nr; i++) { - dst->items[i].match = xstrdup(src->items[i].match); - dst->items[i].original = xstrdup(src->items[i].original); + struct pathspec_item *d = &dst->items[i]; + struct pathspec_item *s = &src->items[i]; + + d->match = xstrdup(s->match); + d->original = xstrdup(s->original); + + ALLOC_ARRAY(d->attr_match, d->attr_match_nr); + COPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr); + for (j = 0; j < d->attr_match_nr; j++) { + const char *value = s->attr_match[j].value; + d->attr_match[j].value = xstrdup_or_null(value); + } + + d->attr_check = attr_check_dup(s->attr_check); } } void clear_pathspec(struct pathspec *pathspec) { - int i; + int i, j; for (i = 0; i < pathspec->nr; i++) { free(pathspec->items[i].match); free(pathspec->items[i].original); + + for (j = 0; j < pathspec->items[j].attr_match_nr; j++) + free(pathspec->items[i].attr_match[j].value); + free(pathspec->items[i].attr_match); + + if (pathspec->items[i].attr_check) + attr_check_free(pathspec->items[i].attr_check); } + free(pathspec->items); pathspec->items = NULL; pathspec->nr = 0; diff --git a/pathspec.h b/pathspec.h index 49fd823dd..55e976972 100644 --- a/pathspec.h +++ b/pathspec.h @@ -8,13 +8,15 @@ #define PATHSPEC_GLOB (1<<3) #define PATHSPEC_ICASE (1<<4) #define PATHSPEC_EXCLUDE (1<<5) +#define PATHSPEC_ATTR (1<<6) #define PATHSPEC_ALL_MAGIC \ (PATHSPEC_FROMTOP | \ PATHSPEC_MAXDEPTH | \ PATHSPEC_LITERAL | \ PATHSPEC_GLOB | \ PATHSPEC_ICASE | \ - PATHSPEC_EXCLUDE) + PATHSPEC_EXCLUDE | \ + PATHSPEC_ATTR) #define PATHSPEC_ONESTAR 1 /* the pathspec pattern satisfies GFNM_ONESTAR */ @@ -31,6 +33,17 @@ struct pathspec { int len, prefix; int nowildcard_len; int flags; + int attr_match_nr; + struct attr_match { + char *value; + enum attr_match_mode { + MATCH_SET, + MATCH_UNSET, + MATCH_VALUE, + MATCH_UNSPECIFIED + } match_mode; + } *attr_match; + struct attr_check *attr_check; } *items; }; diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh new file mode 100755 index 000000000..3f06fcf8d --- /dev/null +++ b/t/t6135-pathspec-with-attrs.sh @@ -0,0 +1,181 @@ +#!/bin/sh + +test_description='test labels in pathspecs' +. ./test-lib.sh + +test_expect_success 'setup a tree' ' + cat <<-\EOF >expect && + fileA + fileAB + fileAC + fileB + fileBC + fileC + fileNoLabel + fileSetLabel + fileUnsetLabel + fileValue + fileWrongLabel + sub/fileA + sub/fileAB + sub/fileAC + sub/fileB + sub/fileBC + sub/fileC + sub/fileNoLabel + sub/fileSetLabel + sub/fileUnsetLabel + sub/fileValue + sub/fileWrongLabel + EOF + mkdir sub && + while read path + do + : >$path && + git add $path || return 1 + done <expect && + git commit -m "initial commit" && + git ls-files >actual && + test_cmp expect actual +' + +test_expect_success 'pathspec with no attr' ' + test_must_fail git ls-files ":(attr:)" +' + +test_expect_success 'pathspec with labels and non existent .gitattributes' ' + git ls-files ":(attr:label)" >actual && + test_must_be_empty actual +' + +test_expect_success 'setup .gitattributes' ' + cat <<-\EOF >.gitattributes && + fileA labelA + fileB labelB + fileC labelC + fileAB labelA labelB + fileAC labelA labelC + fileBC labelB labelC + fileUnsetLabel -label + fileSetLabel label + fileValue label=foo + fileWrongLabel label☺ + EOF + git add .gitattributes && + git commit -m "add attributes" +' + +test_expect_success 'check specific set attr' ' + cat <<-\EOF >expect && + fileSetLabel + sub/fileSetLabel + EOF + git ls-files ":(attr:label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check specific unset attr' ' + cat <<-\EOF >expect && + fileUnsetLabel + sub/fileUnsetLabel + EOF + git ls-files ":(attr:-label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check specific value attr' ' + cat <<-\EOF >expect && + fileValue + sub/fileValue + EOF + git ls-files ":(attr:label=foo)" >actual && + test_cmp expect actual && + git ls-files ":(attr:label=bar)" >actual && + test_must_be_empty actual +' + +test_expect_success 'check unspecified attr' ' + cat <<-\EOF >expect && + .gitattributes + fileA + fileAB + fileAC + fileB + fileBC + fileC + fileNoLabel + fileWrongLabel + sub/fileA + sub/fileAB + sub/fileAC + sub/fileB + sub/fileBC + sub/fileC + sub/fileNoLabel + sub/fileWrongLabel + EOF + git ls-files ":(attr:!label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check multiple unspecified attr' ' + cat <<-\EOF >expect && + .gitattributes + fileC + fileNoLabel + fileWrongLabel + sub/fileC + sub/fileNoLabel + sub/fileWrongLabel + EOF + git ls-files ":(attr:!labelB !labelA !label)" >actual && + test_cmp expect actual +' + +test_expect_success 'check label with more labels but excluded path' ' + cat <<-\EOF >expect && + fileAB + fileB + fileBC + EOF + git ls-files ":(attr:labelB)" ":(exclude)sub/" >actual && + test_cmp expect actual +' + +test_expect_success 'check label excluding other labels' ' + cat <<-\EOF >expect && + fileAB + fileB + fileBC + sub/fileAB + sub/fileB + EOF + git ls-files ":(attr:labelB)" ":(exclude,attr:labelC)sub/" >actual && + test_cmp expect actual +' + +test_expect_success 'fail on multiple attr specifiers in one pathspec item' ' + test_must_fail git ls-files . ":(attr:labelB,attr:labelC)" 2>actual && + test_i18ngrep "Only one" actual +' + +test_expect_success 'fail if attr magic is used places not implemented' ' + # The main purpose of this test is to check that we actually fail + # when you attempt to use attr magic in commands that do not implement + # attr magic. This test does not advocate git-add to stay that way, + # though, but git-add is convenient as it has its own internal pathspec + # parsing. + test_must_fail git add ":(attr:labelB)" 2>actual && + test_i18ngrep "unsupported magic" actual +' + +test_expect_success 'abort on giving invalid label on the command line' ' + test_must_fail git ls-files . ":(attr:☺)" +' + +test_expect_success 'abort on asking for wrong magic' ' + test_must_fail git ls-files . ":(attr:-label=foo)" && + test_must_fail git ls-files . ":(attr:!label=foo)" +' + +test_done -- 2.12.0.246.ga2ecc84866-goog ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 2/2] pathspec: allow escaped query values 2017-03-13 18:23 ` [PATCH v3 0/2] bringing attributes to pathspecs Brandon Williams 2017-03-13 18:23 ` [PATCH v3 1/2] pathspec: allow querying for attributes Brandon Williams @ 2017-03-13 18:23 ` Brandon Williams 2017-03-13 22:30 ` [PATCH v3 0/2] bringing attributes to pathspecs Junio C Hamano 2017-03-21 10:51 ` Duy Nguyen 3 siblings, 0 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-13 18:23 UTC (permalink / raw) To: git; +Cc: Brandon Williams, sbeller, pclouds, jonathantanmy In our own .gitattributes file we have attributes such as: *.[ch] whitespace=indent,trail,space When querying for attributes we want to be able to ask for the exact value, i.e. git ls-files :(attr:whitespace=indent,trail,space) should work, but the commas are used in the attr magic to introduce the next attr, such that this query currently fails with fatal: Invalid pathspec magic 'trail' in ':(attr:whitespace=indent,trail,space)' This change allows escaping characters by a backslash, such that the query git ls-files :(attr:whitespace=indent\,trail\,space) will match all path that have the value "indent,trail,space" for the whitespace attribute. To accomplish this, we need to modify two places. First `parse_long_magic` needs to not stop early upon seeing a comma or closing paren that is escaped. As a second step we need to remove any escaping from the attr value. Based on a patch by Stefan Beller <sbeller@google.com> Signed-off-by: Brandon Williams <bmwill@google.com> --- pathspec.c | 52 ++++++++++++++++++++++++++++++++++++++---- t/t6135-pathspec-with-attrs.sh | 19 +++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/pathspec.c b/pathspec.c index 608511c52..303efda83 100644 --- a/pathspec.c +++ b/pathspec.c @@ -89,6 +89,51 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic) strbuf_addf(sb, ",prefix:%d)", prefixlen); } +static size_t strcspn_escaped(const char *s, const char *stop) +{ + const char *i; + + for (i = s; *i; i++) { + /* skip the escaped character */ + if (i[0] == '\\' && i[1]) { + i++; + continue; + } + + if (strchr(stop, *i)) + break; + } + return i - s; +} + +static inline int invalid_value_char(const char ch) +{ + if (isalnum(ch) || strchr(",-_", ch)) + return 0; + return -1; +} + +static char *attr_value_unescape(const char *value) +{ + const char *src; + char *dst, *ret; + + ret = xmallocz(strlen(value)); + for (src = value, dst = ret; *src; src++, dst++) { + if (*src == '\\') { + if (!src[1]) + die(_("Escape character '\\' not allowed as " + "last character in attr value")); + src++; + } + if (invalid_value_char(*src)) + die("cannot use '%c' for value matching", *src); + *dst = *src; + } + *dst = '\0'; + return ret; +} + static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value) { struct string_list_item *si; @@ -131,10 +176,9 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va if (attr[attr_len] != '=') am->match_mode = MATCH_SET; else { + const char *v = &attr[attr_len + 1]; am->match_mode = MATCH_VALUE; - am->value = xstrdup(&attr[attr_len + 1]); - if (strchr(am->value, '\\')) - die(_("attr spec values must not contain backslashes")); + am->value = attr_value_unescape(v); } break; } @@ -239,7 +283,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len, const char *nextat; for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) { - size_t len = strcspn(pos, ",)"); + size_t len = strcspn_escaped(pos, ",)"); int i; if (pos[len] == ',') diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh index 3f06fcf8d..77b8cef66 100755 --- a/t/t6135-pathspec-with-attrs.sh +++ b/t/t6135-pathspec-with-attrs.sh @@ -178,4 +178,23 @@ test_expect_success 'abort on asking for wrong magic' ' test_must_fail git ls-files . ":(attr:!label=foo)" ' +test_expect_success 'check attribute list' ' + cat <<-EOF >>.gitattributes && + * whitespace=indent,trail,space + EOF + git ls-files ":(attr:whitespace=indent\,trail\,space)" >actual && + git ls-files >expect && + test_cmp expect actual +' + +test_expect_success 'backslash cannot be the last character' ' + test_must_fail git ls-files ":(attr:label=foo\\ labelA=bar)" 2>actual && + test_i18ngrep "not allowed as last character in attr value" actual +' + +test_expect_success 'backslash cannot be used as a value' ' + test_must_fail git ls-files ":(attr:label=f\\\oo)" 2>actual && + test_i18ngrep "for value matching" actual +' + test_done -- 2.12.0.246.ga2ecc84866-goog ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 0/2] bringing attributes to pathspecs 2017-03-13 18:23 ` [PATCH v3 0/2] bringing attributes to pathspecs Brandon Williams 2017-03-13 18:23 ` [PATCH v3 1/2] pathspec: allow querying for attributes Brandon Williams 2017-03-13 18:23 ` [PATCH v3 2/2] pathspec: allow escaped query values Brandon Williams @ 2017-03-13 22:30 ` Junio C Hamano 2017-03-13 22:38 ` Brandon Williams 2017-03-21 10:51 ` Duy Nguyen 3 siblings, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2017-03-13 22:30 UTC (permalink / raw) To: Brandon Williams; +Cc: git, sbeller, pclouds, jonathantanmy Brandon Williams <bmwill@google.com> writes: > v3 fixes some nits in style in the test script (using <<-\EOF instead of <<-EOF) > as well as fixing a few other minor things reported by Junio and Jonathan. Thanks. Will replace. I think this is ready for 'next', so let's ask reviewers to really pay attention to this round, wait for a few days and merge it by the end of the week at the latest. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 0/2] bringing attributes to pathspecs 2017-03-13 22:30 ` [PATCH v3 0/2] bringing attributes to pathspecs Junio C Hamano @ 2017-03-13 22:38 ` Brandon Williams 0 siblings, 0 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-13 22:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, sbeller, pclouds, jonathantanmy On 03/13, Junio C Hamano wrote: > Brandon Williams <bmwill@google.com> writes: > > > v3 fixes some nits in style in the test script (using <<-\EOF instead of <<-EOF) > > as well as fixing a few other minor things reported by Junio and Jonathan. > > Thanks. Will replace. > > I think this is ready for 'next', so let's ask reviewers to really > pay attention to this round, wait for a few days and merge it by the > end of the week at the latest. Sounds good to me, Thanks! -- Brandon Williams ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 0/2] bringing attributes to pathspecs 2017-03-13 18:23 ` [PATCH v3 0/2] bringing attributes to pathspecs Brandon Williams ` (2 preceding siblings ...) 2017-03-13 22:30 ` [PATCH v3 0/2] bringing attributes to pathspecs Junio C Hamano @ 2017-03-21 10:51 ` Duy Nguyen 2017-03-21 15:51 ` Junio C Hamano 2017-03-21 16:52 ` Brandon Williams 3 siblings, 2 replies; 23+ messages in thread From: Duy Nguyen @ 2017-03-21 10:51 UTC (permalink / raw) To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, jonathantanmy On Tue, Mar 14, 2017 at 1:23 AM, Brandon Williams <bmwill@google.com> wrote: > v3 fixes some nits in style in the test script (using <<-\EOF instead of <<-EOF) > as well as fixing a few other minor things reported by Junio and Jonathan. I'm slowly digging through the pile of mails in the past weeks... I know this has landed on 'master' (thanks!). Just wanted to check something. The series updated match_pathspec(), but that's only one of two pathspec filtering functions. The other is tree_entry_interesting() (e.g. for "git grep <tree>"). Do you have plans to support :(attr) there too? "No" is a perfectly fine answer (and it will end up in my forever growing backlog). The thing about tree_entry_interesting() is, we would want to stop traversing subtrees as soon as possible. Naively implemented, we would need to traverse all subtrees so we can call match_attrs(). That's not great. Oii I'm rambling.. I don't know yet how to implement this thing efficiently. -- Duy ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 0/2] bringing attributes to pathspecs 2017-03-21 10:51 ` Duy Nguyen @ 2017-03-21 15:51 ` Junio C Hamano 2017-03-21 16:52 ` Brandon Williams 1 sibling, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2017-03-21 15:51 UTC (permalink / raw) To: Duy Nguyen Cc: Brandon Williams, Git Mailing List, Stefan Beller, jonathantanmy Duy Nguyen <pclouds@gmail.com> writes: > The series updated match_pathspec(), but that's only one of two > pathspec filtering functions. The other is tree_entry_interesting() > (e.g. for "git grep <tree>"). Do you have plans to support :(attr) > there too? "No" is a perfectly fine answer (and it will end up in my > forever growing backlog). > > The thing about tree_entry_interesting() is, we would want to stop > traversing subtrees as soon as possible. Naively implemented, we would > need to traverse all subtrees so we can call match_attrs(). That's not > great. Oii I'm rambling.. I don't know yet how to implement this thing > efficiently. Thanks for great insights. It indeed will become issue when an overly broad pathspec pattern is combined with an attribute requirement, e.g. ".:(attr=X)", and we may have to devise a way to tell that there won't be any paths with that satisfy the attribute requirement before descending into a tree as an optimization. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 0/2] bringing attributes to pathspecs 2017-03-21 10:51 ` Duy Nguyen 2017-03-21 15:51 ` Junio C Hamano @ 2017-03-21 16:52 ` Brandon Williams 1 sibling, 0 replies; 23+ messages in thread From: Brandon Williams @ 2017-03-21 16:52 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, jonathantanmy On 03/21, Duy Nguyen wrote: > On Tue, Mar 14, 2017 at 1:23 AM, Brandon Williams <bmwill@google.com> wrote: > > v3 fixes some nits in style in the test script (using <<-\EOF instead of <<-EOF) > > as well as fixing a few other minor things reported by Junio and Jonathan. > > I'm slowly digging through the pile of mails in the past weeks... I > know this has landed on 'master' (thanks!). Just wanted to check > something. > > The series updated match_pathspec(), but that's only one of two > pathspec filtering functions. The other is tree_entry_interesting() > (e.g. for "git grep <tree>"). Do you have plans to support :(attr) > there too? "No" is a perfectly fine answer (and it will end up in my > forever growing backlog). > > The thing about tree_entry_interesting() is, we would want to stop > traversing subtrees as soon as possible. Naively implemented, we would > need to traverse all subtrees so we can call match_attrs(). That's not > great. Oii I'm rambling.. I don't know yet how to implement this thing > efficiently. I expect that this should be supported in tree_entry_interesting() at some point, though I didn't have any immediate plans to do that yet. One reason for that is I just haven't thought through all the cases to make it performant (as you've pointed out). So for now it'll probably be appended to the backlog (yours, mine, or both) and if there ends up being a larger demand for it then we can increase the priority for it. -- Brandon Williams ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2017-03-21 16:58 UTC | newest] Thread overview: 23+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-09 21:07 [PATCH 0/2] bringing attributes to pathspecs Brandon Williams 2017-03-09 21:07 ` [PATCH 1/2] pathspec: allow querying for attributes Brandon Williams 2017-03-09 22:19 ` Jonathan Tan 2017-03-10 18:26 ` Brandon Williams 2017-03-13 2:43 ` Junio C Hamano 2017-03-13 18:30 ` Stefan Beller 2017-03-09 21:07 ` [PATCH 2/2] pathspec: allow escaped query values Brandon Williams 2017-03-09 22:31 ` Jonathan Tan 2017-03-10 18:53 ` Brandon Williams 2017-03-09 21:22 ` [PATCH 0/2] bringing attributes to pathspecs Stefan Beller 2017-03-10 18:59 ` [PATCH v2 " Brandon Williams 2017-03-10 18:59 ` [PATCH v2 1/2] pathspec: allow querying for attributes Brandon Williams 2017-03-10 19:56 ` Jonathan Tan 2017-03-11 0:28 ` Brandon Williams 2017-03-10 18:59 ` [PATCH v2 2/2] pathspec: allow escaped query values Brandon Williams 2017-03-13 18:23 ` [PATCH v3 0/2] bringing attributes to pathspecs Brandon Williams 2017-03-13 18:23 ` [PATCH v3 1/2] pathspec: allow querying for attributes Brandon Williams 2017-03-13 18:23 ` [PATCH v3 2/2] pathspec: allow escaped query values Brandon Williams 2017-03-13 22:30 ` [PATCH v3 0/2] bringing attributes to pathspecs Junio C Hamano 2017-03-13 22:38 ` Brandon Williams 2017-03-21 10:51 ` Duy Nguyen 2017-03-21 15:51 ` Junio C Hamano 2017-03-21 16:52 ` Brandon Williams
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).