threads / patch / 45334

patch, 2 partsbringing attributes to pathspecs

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

## tl;dr

23 messages between Mar 9, 2017 and Mar 21, 2017. Diffs are folded; open one to read it.

replies: 22people: 5as markdown or json

Brandon Williams· Mar 9, 2017, 21:07 UTC · lore

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
Brandon Williams· Mar 9, 2017, 21:07 UTC · re: Brandon Williams · lore

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

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
Show changes to 7 files +388 −9

Documentation/glossary-content.txt, attr.c, attr.h, dir.c, pathspec.c, pathspec.h, 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
Jonathan Tan· Mar 9, 2017, 22:19 UTC · re: Brandon Williams · lore

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

On 03/09/2017 01:07 PM, Brandon Williams wrote:
Show 18 quoted lines
> 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.

Show 8 quoted lines
> +
> +- "`-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.

Show 17 quoted lines
> 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.

Show 6 quoted lines
> +	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?

Show 46 quoted lines
> +
> +	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?
Show 8 quoted lines
> @@ -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.

Brandon Williams· Mar 10, 2017, 18:26 UTC · re: Jonathan Tan · lore

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

On 03/09, Jonathan Tan wrote:
Show 25 quoted lines
> 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.
Show 13 quoted lines
> 
> >+
> >+- "`-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.

Show 21 quoted lines
> 
> >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.
Show 9 quoted lines
> >+	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.
Show 13 quoted lines
> 
> >@@ -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
Junio C Hamano· Mar 13, 2017, 02:43 UTC · re: Brandon Williams · lore

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

Brandon Williams <bmwill@google.com> writes:
Show 16 quoted lines
> +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.
Show 26 quoted lines
> @@ -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.
Show 26 quoted lines
> +		}
> +
> +		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.
Show 13 quoted lines
> 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 &&
Show 6 quoted lines
> +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.
Stefan Beller· Mar 13, 2017, 18:30 UTC · re: Junio C Hamano · lore

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

Show 10 quoted lines
>> +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

Brandon Williams· Mar 9, 2017, 21:07 UTC · re: Brandon Williams · lore

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

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(-)
Show changes to 2 files +57 −4

pathspec.c, t/t6135-pathspec-with-attrs.sh

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
Jonathan Tan· Mar 9, 2017, 22:31 UTC · re: Brandon Williams · lore

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

On 03/09/2017 01:07 PM, Brandon Williams wrote:
Show 18 quoted lines
> 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 \.

Brandon Williams· Mar 10, 2017, 18:53 UTC · re: Jonathan Tan · lore

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

On 03/09, Jonathan Tan wrote:
Show 21 quoted lines
> 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
Stefan Beller· Mar 9, 2017, 21:22 UTC · re: Brandon Williams · lore

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

On Thu, Mar 9, 2017 at 1:07 PM, Brandon Williams <bmwill@google.com> wrote:
Show 10 quoted lines
> 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

Brandon Williams· Mar 10, 2017, 18:59 UTC · re: Brandon Williams · lore

[PATCH v2 0/2] bringing attributes to pathspecs

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
Brandon Williams· Mar 10, 2017, 18:59 UTC · re: Brandon Williams · lore

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

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
Show changes to 7 files +387 −9

Documentation/glossary-content.txt, attr.c, attr.h, dir.c, pathspec.c, pathspec.h, 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
Jonathan Tan· Mar 10, 2017, 19:56 UTC · re: Brandon Williams · lore

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

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:
Show 26 quoted lines
> 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)`.

Show 46 quoted lines
> +
> +	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.

Show 7 quoted lines
> +
> +	string_list_clear(&list, 0);
> +}
> +
>  static inline int get_literal_global(void)
>  {
>  	static int literal = -1;
Brandon Williams· Mar 11, 2017, 00:28 UTC · re: Jonathan Tan · lore

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

On 03/10, Jonathan Tan wrote:
Show 36 quoted lines
> 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.
Show 50 quoted lines
> 
> >+
> >+	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.

Show 8 quoted lines
> 
> >+
> >+	string_list_clear(&list, 0);
> >+}
> >+
> > static inline int get_literal_global(void)
> > {
> > 	static int literal = -1;
-- 
Brandon Williams
Brandon Williams· Mar 10, 2017, 18:59 UTC · re: Brandon Williams · lore

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

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(-)
Show changes to 2 files +67 −4

pathspec.c, t/t6135-pathspec-with-attrs.sh

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
Brandon Williams· Mar 13, 2017, 18:23 UTC · re: Brandon Williams · lore

[PATCH v3 0/2] bringing attributes to pathspecs

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
Show changes to 3 files +12 −17

pathspec.c, pathspec.h, t/t6135-pathspec-with-attrs.sh

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
Brandon Williams· Mar 13, 2017, 18:23 UTC · re: Brandon Williams · lore

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

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
Show changes to 7 files +382 −9

Documentation/glossary-content.txt, attr.c, attr.h, dir.c, pathspec.c, pathspec.h, 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
Brandon Williams· Mar 13, 2017, 18:23 UTC · re: Brandon Williams · lore

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

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(-)
Show changes to 2 files +67 −4

pathspec.c, t/t6135-pathspec-with-attrs.sh

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
Junio C Hamano· Mar 13, 2017, 22:30 UTC · re: Brandon Williams · lore

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

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.
Brandon Williams· Mar 13, 2017, 22:38 UTC · re: Junio C Hamano · lore

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

On 03/13, Junio C Hamano wrote:
Show 10 quoted lines
> 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
Duy Nguyen· Mar 21, 2017, 10:51 UTC · re: Brandon Williams · lore

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

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
Junio C Hamano· Mar 21, 2017, 15:51 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 11 quoted lines
> 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.

Brandon Williams· Mar 21, 2017, 16:52 UTC · re: Duy Nguyen · lore

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

On 03/21, Duy Nguyen wrote:
Show 19 quoted lines
> 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

← back to recent threads