[PATCH 0/2] bringing attributes to pathspecs

[PATCH 0/2] bringing attributes to pathspecs

[Brandon Williams]

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

[PATCH 1/2] pathspec: allow querying for attributes

[Brandon Williams]

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

[PATCH 2/2] pathspec: allow escaped query values

[Brandon Williams]

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

Re: [PATCH 0/2] bringing attributes to pathspecs

[Stefan Beller]

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

Re: [PATCH 1/2] pathspec: allow querying for attributes

[Jonathan Tan]

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.

Re: [PATCH 2/2] pathspec: allow escaped query values

[Jonathan Tan]

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

Re: [PATCH 1/2] pathspec: allow querying for attributes

[Brandon Williams]

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

Re: [PATCH 2/2] pathspec: allow escaped query values

[Brandon Williams]

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

[PATCH v2 0/2] bringing attributes to pathspecs

[Brandon Williams]

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

[PATCH v2 1/2] pathspec: allow querying for attributes

[Brandon Williams]

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

[PATCH v2 2/2] pathspec: allow escaped query values

[Brandon Williams]

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

Re: [PATCH v2 1/2] pathspec: allow querying for attributes

[Jonathan Tan]

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;

Re: [PATCH v2 1/2] pathspec: allow querying for attributes

[Brandon Williams]

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

Re: [PATCH 1/2] pathspec: allow querying for attributes

[Junio C Hamano]

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.

[PATCH v3 0/2] bringing attributes to pathspecs

[Brandon Williams]

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

[PATCH v3 1/2] pathspec: allow querying for attributes

[Brandon Williams]

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

[PATCH v3 2/2] pathspec: allow escaped query values

[Brandon Williams]

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

Re: [PATCH 1/2] pathspec: allow querying for attributes

[Stefan Beller]

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

Re: [PATCH v3 0/2] bringing attributes to pathspecs

[Junio C Hamano]

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.

Re: [PATCH v3 0/2] bringing attributes to pathspecs

[Brandon Williams]

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

Re: [PATCH v3 0/2] bringing attributes to pathspecs

[Duy Nguyen]

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

Re: [PATCH v3 0/2] bringing attributes to pathspecs

[Junio C Hamano]

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.

Re: [PATCH v3 0/2] bringing attributes to pathspecs

[Brandon Williams]

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