[PATCHv9 0/4] pathspec magic extension to search for attributes

[PATCHv9 0/4] pathspec magic extension to search for attributes

[Stefan Beller]

This goes on top of origin/jc/attr, (396bf756f95, attr: expose validity check
for attribute names)

Patches 1 is a small fix, which could go independently as well.
I dropped the patch "string list: improve comment"
Patches 3 and 4 are refactoring pathspec.c a little.
These did not change since v7

Patch 5 contains all of Junios suggestions.

Thanks,
Stefan

diff to v8:
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index aa9f220..e06520b 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -385,20 +385,23 @@ full pathname may have special meaning:
 Glob magic is incompatible with literal magic.
 
 attr;;
-	Additionally to matching the pathspec, the path must have the
-	attribute as specified. The syntax for specifying the required
-	attributes is "`attr: [mode] <attribute name> [=value]`"
-+
-Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and
-you can query each attribute for certain states. The "`[mode]`" is a special
-character to indicate which attribute states are looked for. The following
-modes are available:
-
- - an empty "`[mode]`" matches if the attribute is set
- - "`-`" the attribute must be unset
- - "`!`" the attribute must be unspecified
- - an empty "`[mode]`" combined with "`[=value]`" matches if the attribute has
-   the given value.
+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;;
diff --git a/dir.c b/dir.c
index f60a503..fc071af 100644
--- a/dir.c
+++ b/dir.c
@@ -231,11 +231,11 @@ static int match_attrs(const char *name, int namelen,
 		match_mode = item->attr_match[i].match_mode;
 
 		if (ATTR_TRUE(value))
-			matched = match_mode == MATCH_SET;
+			matched = (match_mode == MATCH_SET);
 		else if (ATTR_FALSE(value))
-			matched = match_mode == MATCH_UNSET;
+			matched = (match_mode == MATCH_UNSET);
 		else if (ATTR_UNSET(value))
-			matched = match_mode == MATCH_UNSPECIFIED;
+			matched = (match_mode == MATCH_UNSPECIFIED);
 		else
 			matched = (match_mode == MATCH_VALUE &&
 				   !strcmp(item->attr_match[i].value, value));
diff --git a/pathspec.c b/pathspec.c
index b795a9c..693a5e7 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -115,34 +115,38 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va
 		const char *attr = si->string;
 		struct attr_match *am = &item->attr_match[j];
 
-		if (attr[0] == '!')
+		attr_len = strcspn(attr, "=");
+		switch (*attr) {
+		case '!':
 			am->match_mode = MATCH_UNSPECIFIED;
-		else if (attr[0] == '-')
+			attr++;
+			attr_len--;
+			break;
+		case '-':
 			am->match_mode = MATCH_UNSET;
-		else
-			am->match_mode = MATCH_SET;
-
-		if (am->match_mode != MATCH_SET)
-			/* skip first character */
 			attr++;
+			attr_len--;
+			break;
+		default:
+			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_len = strcspn(attr, "=");
-		if (attr[attr_len] == '=') {
-			am->match_mode = MATCH_VALUE;
-			am->value = xstrdup(&attr[attr_len + 1]);
-			if (strchr(am->value, '\\'))
-				die(_("attr spec values must not contain backslashes"));
-		} else
-			am->value = NULL;
-
-		if (!attr_name_valid(attr, attr_len)) {
+		am->attr = git_attr_counted(attr, attr_len);
+		if (!am->attr) {
 			struct strbuf sb = STRBUF_INIT;
 			am->match_mode = INVALID_ATTR;
 			invalid_attr_name_message(&sb, attr, attr_len);
 			die(_("invalid attribute in '%s': '%s'"), value, sb.buf);
 		}
 
-		am->attr = git_attr_counted(attr, attr_len);
 		git_attr_check_append(item->attr_check, am->attr);
 	}
 
diff --git a/string-list.h b/string-list.h
index 465a1f0..d3809a1 100644
--- a/string-list.h
+++ b/string-list.h
@@ -106,7 +106,7 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_
  * list->strdup_strings must be set, as new memory needs to be
  * allocated to hold the substrings.  If maxsplit is non-negative,
  * then split at most maxsplit times.  Return the number of substrings
- * appended to list. The list may be non-empty already.
+ * appended to list.
  *
  * Examples:
  *   string_list_split(l, "foo:bar:baz", ':', -1) -> ["foo", "bar", "baz"]
diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
index c0d8cda..5da1a63 100755
--- a/t/t6134-pathspec-with-labels.sh
+++ b/t/t6134-pathspec-with-labels.sh
@@ -4,39 +4,38 @@ 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 &&
-	for p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel fileUnsetLabel fileSetLabel fileValue fileWrongLabel; do
-		: >$p &&
-		git add $p &&
-		: >sub/$p
-		git add sub/$p
-	done &&
-	git commit -m $p &&
+	while read path
+	do
+		: >$path &&
+		git add $path || return 1
+	done <expect &&
+	git commit -m "initial commit" &&
 	git ls-files >actual &&
-	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
 	test_cmp expect actual
 '
 
@@ -51,47 +50,45 @@ test_expect_success 'pathspec with labels and non existent .gitattributes' '
 '
 
 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
+	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"
 '
 
-sq="'"
-
 test_expect_success 'check specific set attr' '
-	cat <<EOF >expect &&
-fileSetLabel
-sub/fileSetLabel
-EOF
+	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
+	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
+	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 &&
@@ -99,61 +96,61 @@ EOF
 '
 
 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 &&
+	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 &&
+	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
+	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
+	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
 '

[PATCHv9 1/4] Documentation: fix a typo

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/gitattributes.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e3b1de8..af2c682 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -86,7 +86,7 @@ is either not set or empty, $HOME/.config/git/attributes is used instead.
 Attributes for all users on a system should be placed in the
 `$(prefix)/etc/gitattributes` file.
 
-Sometimes you would need to override an setting of an attribute
+Sometimes you would need to override a setting of an attribute
 for a path to `Unspecified` state.  This can be done by listing
 the name of the attribute prefixed with an exclamation point `!`.
 
-- 
2.8.2.123.gb4ad9b6.dirty

[PATCHv9 2/4] pathspec: move long magic parsing out of prefix_pathspec

[Stefan Beller]

`prefix_pathspec` is quite a lengthy function and we plan on adding more.
Split it up for better readability. As we want to add code into the
inner loop of the long magic parsing, we also benefit from lower
indentation.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 pathspec.c | 84 +++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 47 insertions(+), 37 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index c9e9b6c..eba37c2 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -88,6 +88,52 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
 	strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
+static void eat_long_magic(struct pathspec_item *item, const char *elt,
+		unsigned *magic, int *pathspec_prefix,
+		const char **copyfrom_, const char **long_magic_end)
+{
+	int i;
+	const char *copyfrom = *copyfrom_;
+	/* longhand */
+	const char *nextat;
+	for (copyfrom = elt + 2;
+	     *copyfrom && *copyfrom != ')';
+	     copyfrom = nextat) {
+		size_t len = strcspn(copyfrom, ",)");
+		if (copyfrom[len] == ',')
+			nextat = copyfrom + len + 1;
+		else
+			/* handle ')' and '\0' */
+			nextat = copyfrom + len;
+		if (!len)
+			continue;
+		for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+			if (strlen(pathspec_magic[i].name) == len &&
+			    !strncmp(pathspec_magic[i].name, copyfrom, len)) {
+				*magic |= pathspec_magic[i].bit;
+				break;
+			}
+			if (starts_with(copyfrom, "prefix:")) {
+				char *endptr;
+				*pathspec_prefix = strtol(copyfrom + 7,
+							  &endptr, 10);
+				if (endptr - copyfrom != len)
+					die(_("invalid parameter for pathspec magic 'prefix'"));
+				/* "i" would be wrong, but it does not matter */
+				break;
+			}
+		}
+		if (ARRAY_SIZE(pathspec_magic) <= i)
+			die(_("Invalid pathspec magic '%.*s' in '%s'"),
+			    (int) len, copyfrom, elt);
+	}
+	if (*copyfrom != ')')
+		die(_("Missing ')' at the end of pathspec magic in '%s'"), elt);
+	*long_magic_end = copyfrom;
+	copyfrom++;
+	*copyfrom_ = copyfrom;
+}
+
 /*
  * Take an element of a pathspec and check for magic signatures.
  * Append the result to the prefix. Return the magic bitmap.
@@ -150,43 +196,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 	    (flags & PATHSPEC_LITERAL_PATH)) {
 		; /* nothing to do */
 	} else if (elt[1] == '(') {
-		/* longhand */
-		const char *nextat;
-		for (copyfrom = elt + 2;
-		     *copyfrom && *copyfrom != ')';
-		     copyfrom = nextat) {
-			size_t len = strcspn(copyfrom, ",)");
-			if (copyfrom[len] == ',')
-				nextat = copyfrom + len + 1;
-			else
-				/* handle ')' and '\0' */
-				nextat = copyfrom + len;
-			if (!len)
-				continue;
-			for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
-				if (strlen(pathspec_magic[i].name) == len &&
-				    !strncmp(pathspec_magic[i].name, copyfrom, len)) {
-					magic |= pathspec_magic[i].bit;
-					break;
-				}
-				if (starts_with(copyfrom, "prefix:")) {
-					char *endptr;
-					pathspec_prefix = strtol(copyfrom + 7,
-								 &endptr, 10);
-					if (endptr - copyfrom != len)
-						die(_("invalid parameter for pathspec magic 'prefix'"));
-					/* "i" would be wrong, but it does not matter */
-					break;
-				}
-			}
-			if (ARRAY_SIZE(pathspec_magic) <= i)
-				die(_("Invalid pathspec magic '%.*s' in '%s'"),
-				    (int) len, copyfrom, elt);
-		}
-		if (*copyfrom != ')')
-			die(_("Missing ')' at the end of pathspec magic in '%s'"), elt);
-		long_magic_end = copyfrom;
-		copyfrom++;
+		eat_long_magic(item, elt, &magic, &pathspec_prefix, &copyfrom, &long_magic_end);
 	} else {
 		/* shorthand */
 		for (copyfrom = elt + 1;
-- 
2.8.2.123.gb4ad9b6.dirty

[PATCHv9 3/4] pathspec: move prefix check out of the inner loop

[Stefan Beller]

The prefix check is not related the check of pathspec magic; also there
is no code that is relevant after we'd break the loop on a match for
"prefix:". So move the check before the loop and shortcircuit the outer
loop.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 pathspec.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index eba37c2..4dff252 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -107,21 +107,22 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,
 			nextat = copyfrom + len;
 		if (!len)
 			continue;
+
+		if (starts_with(copyfrom, "prefix:")) {
+			char *endptr;
+			*pathspec_prefix = strtol(copyfrom + 7,
+						  &endptr, 10);
+			if (endptr - copyfrom != len)
+				die(_("invalid parameter for pathspec magic 'prefix'"));
+			continue;
+		}
+
 		for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
 			if (strlen(pathspec_magic[i].name) == len &&
 			    !strncmp(pathspec_magic[i].name, copyfrom, len)) {
 				*magic |= pathspec_magic[i].bit;
 				break;
 			}
-			if (starts_with(copyfrom, "prefix:")) {
-				char *endptr;
-				*pathspec_prefix = strtol(copyfrom + 7,
-							  &endptr, 10);
-				if (endptr - copyfrom != len)
-					die(_("invalid parameter for pathspec magic 'prefix'"));
-				/* "i" would be wrong, but it does not matter */
-				break;
-			}
 		}
 		if (ARRAY_SIZE(pathspec_magic) <= i)
 			die(_("Invalid pathspec magic '%.*s' in '%s'"),
-- 
2.8.2.123.gb4ad9b6.dirty

[PATCHv9 4/4] pathspec: allow querying for attributes

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/glossary-content.txt |  20 +++++
 dir.c                              |  35 ++++++++
 pathspec.c                         | 101 ++++++++++++++++++++++-
 pathspec.h                         |  16 ++++
 t/t6134-pathspec-with-labels.sh    | 163 +++++++++++++++++++++++++++++++++++++
 5 files changed, 331 insertions(+), 4 deletions(-)
 create mode 100755 t/t6134-pathspec-with-labels.sh

diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index cafc284..e06520b 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: `!`). If it
diff --git a/dir.c b/dir.c
index 996653b..fc071af 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"
@@ -215,6 +216,37 @@ int within_depth(const char *name, int namelen,
 	return 1;
 }
 
+static int match_attrs(const char *name, int namelen,
+		       const struct pathspec_item *item)
+{
+	int i;
+
+	git_check_attr_counted(name, namelen, 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->check[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;
+}
+
 #define DO_MATCH_EXCLUDE   1
 #define DO_MATCH_DIRECTORY 2
 
@@ -270,6 +302,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;
diff --git a/pathspec.c b/pathspec.c
index 4dff252..693a5e7 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.
@@ -88,12 +89,78 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
 	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 (!value || !strlen(value))
+		die(_("attr spec must not be empty"));
+
+	string_list_split(&list, value, ' ', -1);
+	string_list_remove_empty_items(&list, 0);
+
+	if (!item->attr_check)
+		item->attr_check = git_attr_check_alloc();
+	else
+		die(_("Only one 'attr:' specification is allowed."));
+
+	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;
+
+		int j = item->attr_match_nr++;
+		const char *attr = si->string;
+		struct attr_match *am = &item->attr_match[j];
+
+		attr_len = strcspn(attr, "=");
+		switch (*attr) {
+		case '!':
+			am->match_mode = MATCH_UNSPECIFIED;
+			attr++;
+			attr_len--;
+			break;
+		case '-':
+			am->match_mode = MATCH_UNSET;
+			attr++;
+			attr_len--;
+			break;
+		default:
+			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;
+		}
+
+		am->attr = git_attr_counted(attr, attr_len);
+		if (!am->attr) {
+			struct strbuf sb = STRBUF_INIT;
+			am->match_mode = INVALID_ATTR;
+			invalid_attr_name_message(&sb, attr, attr_len);
+			die(_("invalid attribute in '%s': '%s'"), value, sb.buf);
+		}
+
+		git_attr_check_append(item->attr_check, am->attr);
+	}
+
+	string_list_clear(&list, 0);
+	return;
+}
+
 static void eat_long_magic(struct pathspec_item *item, const char *elt,
 		unsigned *magic, int *pathspec_prefix,
 		const char **copyfrom_, const char **long_magic_end)
 {
 	int i;
 	const char *copyfrom = *copyfrom_;
+	const char *body;
 	/* longhand */
 	const char *nextat;
 	for (copyfrom = elt + 2;
@@ -108,15 +175,21 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,
 		if (!len)
 			continue;
 
-		if (starts_with(copyfrom, "prefix:")) {
+		if (skip_prefix(copyfrom, "prefix:", &body)) {
 			char *endptr;
-			*pathspec_prefix = strtol(copyfrom + 7,
-						  &endptr, 10);
+			*pathspec_prefix = strtol(body, &endptr, 10);
 			if (endptr - copyfrom != len)
 				die(_("invalid parameter for pathspec magic 'prefix'"));
 			continue;
 		}
 
+		if (skip_prefix(copyfrom, "attr:", &body)) {
+			char *attr_body = xmemdupz(body, len - strlen("attr:"));
+			parse_pathspec_attr_match(item, attr_body);
+			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, copyfrom, len)) {
@@ -425,7 +498,10 @@ void parse_pathspec(struct pathspec *pathspec,
 	for (i = 0; i < n; i++) {
 		unsigned short_magic;
 		entry = argv[i];
-
+		item[i].attr_check = NULL;
+		item[i].attr_match = NULL;
+		item[i].attr_match_nr = 0;
+		item[i].attr_match_alloc = 0;
 		item[i].magic = prefix_pathspec(item + i, &short_magic,
 						argv + i, flags,
 						prefix, prefixlen, entry);
@@ -447,6 +523,13 @@ 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_match_nr) {
+			int j;
+			for (j = 0; j < item[i].attr_match_nr; j++)
+				if (item[i].attr_match[j].match_mode == INVALID_ATTR)
+					die(_("attribute spec in the wrong syntax are prohibited."));
+		}
 	}
 
 	if (nr_exclude == n)
@@ -502,6 +585,16 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
 
 void free_pathspec(struct pathspec *pathspec)
 {
+	int i, j;
+	for (i = 0; i < pathspec->nr; i++) {
+		if (!pathspec->items[i].attr_match_nr)
+			continue;
+		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);
+		git_attr_check_free(pathspec->items[i].attr_check);
+	}
+
 	free(pathspec->items);
 	pathspec->items = NULL;
 }
diff --git a/pathspec.h b/pathspec.h
index 0c11262..5308137 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -32,6 +32,22 @@ struct pathspec {
 		int len, prefix;
 		int nowildcard_len;
 		int flags;
+		int attr_match_nr;
+		int attr_match_alloc;
+		struct attr_match {
+			struct git_attr *attr;
+			char *value;
+			enum attr_match_mode {
+				MATCH_SET,
+				MATCH_UNSET,
+				MATCH_VALUE,
+				MATCH_UNSPECIFIED,
+				MATCH_NOT_UNSPECIFIED,
+				MATCH_SET_OR_VALUE,
+				INVALID_ATTR
+			} match_mode;
+		} *attr_match;
+		struct git_attr_check *attr_check;
 	} *items;
 };
 
diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
new file mode 100755
index 0000000..5da1a63
--- /dev/null
+++ b/t/t6134-pathspec-with-labels.sh
@@ -0,0 +1,163 @@
+#!/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:)" 2>actual &&
+	test_i18ngrep fatal actual
+'
+
+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 'abort on giving invalid label on the command line' '
+	test_must_fail git ls-files . ":(attr:☺)" 2>actual &&
+	test_i18ngrep "fatal" actual
+'
+
+test_done
-- 
2.8.2.123.gb4ad9b6.dirty

Re: [PATCHv9 4/4] pathspec: allow querying for attributes

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +attr;;
> +After `attr:` comes a space separated list of "attribute
> +...
> ++

The text looks OK, but does it format well?

> +		attr_len = strcspn(attr, "=");

Scanning for '=' here retains the same bug from the previous
iteration where you take !VAR=VAL and silently ignore =VAL part
without diagnosing the error, doesn't it?

Perhaps strlen(attr) here, and...

> +		switch (*attr) {
> +		case '!':
> +			am->match_mode = MATCH_UNSPECIFIED;
> +			attr++;
> +			attr_len--;
> +			break;
> +		case '-':
> +			am->match_mode = MATCH_UNSET;
> +			attr++;
> +			attr_len--;
> +			break;
> +		default:
> +			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;
> +		}

... doing strcspn() only in default: part would be a quick fix.

Re: [PATCHv9 4/4] pathspec: allow querying for attributes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> +attr;;
>> +After `attr:` comes a space separated list of "attribute
>> +...
>> ++
>
> The text looks OK, but does it format well?

I didn't check this, but the remainder would look like this
squashable patch.

You seem to i18ngrep for "fatal" but we are using test_must_fail for
the exit status, so I am not sure if that adds much value, so the
additional tests here do nto use that pattern.

diff --git a/pathspec.c b/pathspec.c
index 693a5e7..0a02255 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -115,19 +115,19 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va
 		const char *attr = si->string;
 		struct attr_match *am = &item->attr_match[j];
 
-		attr_len = strcspn(attr, "=");
 		switch (*attr) {
 		case '!':
 			am->match_mode = MATCH_UNSPECIFIED;
 			attr++;
-			attr_len--;
+			attr_len = strlen(attr);
 			break;
 		case '-':
 			am->match_mode = MATCH_UNSET;
 			attr++;
-			attr_len--;
+			attr_len = strlen(attr);
 			break;
 		default:
+			attr_len = strcspn(attr, "=");
 			if (attr[attr_len] != '=')
 				am->match_mode = MATCH_SET;
 			else {
diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
index 5da1a63..060047a 100755
--- a/t/t6134-pathspec-with-labels.sh
+++ b/t/t6134-pathspec-with-labels.sh
@@ -160,4 +160,9 @@ test_expect_success 'abort on giving invalid label on the command line' '
 	test_i18ngrep "fatal" actual
 '
 
+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

Re: [PATCHv9 4/4] pathspec: allow querying for attributes

[Stefan Beller]

On Fri, May 20, 2016 at 11:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> +attr;;
>>> +After `attr:` comes a space separated list of "attribute
>>> +...
>>> ++
>>
>> The text looks OK, but does it format well?
>
> I didn't check this, but the remainder would look like this
> squashable patch.

I checked and it looks wrong. the "exclude" section is indented below
the new attr section

fix is:
--8<--
diff --git a/Documentation/glossary-content.txt
b/Documentation/glossary-content.txt
index e06520b..181f52e 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -389,7 +389,7 @@ 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:
--8<--

I can resend with your proposed fixes as well.

Thanks,
Stefan



>
> You seem to i18ngrep for "fatal" but we are using test_must_fail for
> the exit status, so I am not sure if that adds much value, so the
> additional tests here do nto use that pattern.
>
> diff --git a/pathspec.c b/pathspec.c
> index 693a5e7..0a02255 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -115,19 +115,19 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va
>                 const char *attr = si->string;
>                 struct attr_match *am = &item->attr_match[j];
>
> -               attr_len = strcspn(attr, "=");
>                 switch (*attr) {
>                 case '!':
>                         am->match_mode = MATCH_UNSPECIFIED;
>                         attr++;
> -                       attr_len--;
> +                       attr_len = strlen(attr);
>                         break;
>                 case '-':
>                         am->match_mode = MATCH_UNSET;
>                         attr++;
> -                       attr_len--;
> +                       attr_len = strlen(attr);
>                         break;
>                 default:
> +                       attr_len = strcspn(attr, "=");
>                         if (attr[attr_len] != '=')
>                                 am->match_mode = MATCH_SET;
>                         else {
> diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
> index 5da1a63..060047a 100755
> --- a/t/t6134-pathspec-with-labels.sh
> +++ b/t/t6134-pathspec-with-labels.sh
> @@ -160,4 +160,9 @@ test_expect_success 'abort on giving invalid label on the command line' '
>         test_i18ngrep "fatal" actual
>  '
>
> +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

Re: [PATCHv9 4/4] pathspec: allow querying for attributes

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I checked and it looks wrong. the "exclude" section is indented below
> the new attr section
> ...
> I can resend with your proposed fixes as well.

If you do so, please make sure that the way tests check for error
condition are consistent.  I personally do not think it adds any
value to grep for "fatal", but I do not mind if you adjusted them to
go the same test_i18ngrep route if you think it does (and if you
agree that grepping is not necessary, please do not forget to update
the tests you had that use test_i18ngrep).

Thanks.