[PATCHv2 0/5] pathspec labels [WAS: submodule groups]

[PATCHv2 0/5] pathspec labels [WAS: submodule groups]

[Stefan Beller]

I am happy with the results of the label experiments. (Mainly writing the tests)

>> Isn't the former be "label="

> I do not know what you mean by the latter.  I would understand
>"pretend this has all the labels under the sun", though.

See the new tests what I mean. It cleared up for me as well when writing them.

> I am wondering where we should sanity check (and die, pointing out
> an error in .gitattributes file).  Is this function a good place to
> reject TRUE and FALSE?

TRUE and FALSE are solved issues in this series. However we still want
funny characters not be used. What if we want to use them as 
special characters in the future? (e.g. 'label*' for 'labelA' or 'labelB'
or label{A,B} for the same)

I think it is too late in the matching, so ideally we would check label
names when they are assigned (i.e. while editing the .gitattributes file)
That is not in our control, though.

And during use of match_pathspec it is already to late to die(...)
for bad labels, because the labels may not be in control for the user
(She just cloned the repository and wants to inspect certain files,
which are labeled "$*&^".) So for now we just output a warning for
these cases, in the hope that content creatos upstream respect the
recommendation for labels.

Feedback welcome!

Thanks,
Stefan

Stefan Beller (5):
  Documentation: fix a typo
  Documentation: correct typo in example for querying attributes
  pathspec: move long magic parsing out of prefix_pathspec
  pathspec: move prefix check out of the inner loop
  pathspec: record labels

 Documentation/gitattributes.txt               |  14 ++-
 Documentation/glossary-content.txt            |   4 +
 Documentation/technical/api-gitattributes.txt |   2 +-
 dir.c                                         |  50 ++++++++
 pathspec.c                                    | 110 +++++++++++------
 pathspec.h                                    |   1 +
 t/t6134-pathspec-with-labels.sh               | 164 ++++++++++++++++++++++++++
 7 files changed, 305 insertions(+), 40 deletions(-)
 create mode 100755 t/t6134-pathspec-with-labels.sh

-- 
2.8.2.401.gb7348ac.dirty

[PATCH 1/5] 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.401.g6647087.dirty

[PATCH 2/5] Documentation: correct typo in example for querying attributes

[Stefan Beller]

The introductory text of the example has a typo in the
specification which makes it harder to follow that example.

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

diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 2602668..36fc9af 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -61,7 +61,7 @@ Querying Specific Attributes
 Example
 -------
 
-To see how attributes "crlf" and "indent" are set for different paths.
+To see how attributes "crlf" and "ident" are set for different paths.
 
 . Prepare an array of `struct git_attr_check` with two elements (because
   we are checking two attributes).  Initialize their `attr` member with
-- 
2.8.2.401.g6647087.dirty

[PATCH 3/5] 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.401.g6647087.dirty

[PATCH 4/5] 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.401.g6647087.dirty

[PATCH 5/5] pathspec: record labels

[Stefan Beller]

Labels were originally designed to manage large amount of
submodules, the discussion steered this in a more general
direction, such that other files can be labeled as well.

Labels are meant to describe arbitrary set of files, which
is not described via the tree layout.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/gitattributes.txt    |  12 +++
 Documentation/glossary-content.txt |   4 +
 dir.c                              |  50 +++++++++++
 pathspec.c                         |  64 ++++++++++++++-
 pathspec.h                         |   1 +
 t/t6134-pathspec-with-labels.sh    | 164 +++++++++++++++++++++++++++++++++++++
 6 files changed, 291 insertions(+), 4 deletions(-)
 create mode 100755 t/t6134-pathspec-with-labels.sh

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index af2c682..7fd764f 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -906,6 +906,18 @@ If this attribute is not set or has an invalid value, the value of the
 (See linkgit:git-config[1]).
 
 
+
+Attaching labels to path
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+`label`
+^^^^^^^^^^
+By the value of this attribute you can specify a list of arbitrary strings
+to be attached to a path as labels. These labels can be used for
+easier paths matching using pathspecs (linkgit:gitglossary[1]).
+It is recommended to use only alphanumeric characters, dashes and
+underscores for the labels.
+
 USING MACRO ATTRIBUTES
 ----------------------
 
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 8ad29e6..f990017 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -384,6 +384,10 @@ full pathname may have special meaning:
 +
 Glob magic is incompatible with literal magic.
 
+label:<white space separated list>;;
+	Additionally to matching the pathspec, the path must habe a 'label'
+	attribute having set all labels listed here.
+
 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 656f272..a86e3ff 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"
@@ -208,6 +209,52 @@ int within_depth(const char *name, int namelen,
 	return 1;
 }
 
+static int has_all_labels(const char *name, int namelen,
+			   const struct string_list *required_labels)
+{
+	static struct git_attr *attr;
+
+	struct git_attr_check check;
+	char *path;
+	int ret;
+
+	if (!attr)
+		attr = git_attr("label");
+	check.attr = attr;
+
+	path = xmemdupz(name, namelen);
+	if (git_check_attr(path, 1, &check))
+		die("internal bug: git_check_attr died.");
+	free(path);
+
+	if (ATTR_TRUE(check.value))
+		ret = 1; /* has all the labels */
+	else if (ATTR_FALSE(check.value))
+		ret = 0; /* has no labels */
+	else if (ATTR_UNSET(check.value))
+		/*
+		 * If no label was specified this matches, otherwise
+		 * there is a missing label.
+		 */
+		ret = (required_labels->nr > 0) ? 0 : 1;
+	else {
+		struct string_list_item *si;
+		struct string_list attr_labels = STRING_LIST_INIT_DUP;
+		string_list_split(&attr_labels, check.value, ',', -1);
+		string_list_sort(&attr_labels);
+		ret = 1;
+		for_each_string_list_item(si, required_labels) {
+			if (!string_list_has_string(&attr_labels, si->string)) {
+				ret = 0;
+				break;
+			}
+		}
+		string_list_clear(&attr_labels, 0);
+	}
+
+	return ret;
+}
+
 #define DO_MATCH_EXCLUDE   1
 #define DO_MATCH_DIRECTORY 2
 
@@ -263,6 +310,9 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 	    strncmp(item->match, name - prefix, item->prefix))
 		return 0;
 
+	if (item->labels && !has_all_labels(name, namelen, item->labels))
+		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..c65428a 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -88,12 +88,40 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
 	strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
+static int check_valid_label_name(const char *label)
+{
+	if (!label || !strlen(label))
+		return -1;
+
+	if (!isalpha(*label))
+		return -1;
+
+	while (*label) {
+		if (!(isalnum(*label) ||
+			*label == '-' ||
+			*label == '_'))
+			return -1;
+		label++;
+	}
+
+	return 0;
+}
+
+static void validate_label_name(const char *label)
+{
+	if (check_valid_label_name(label))
+		warning(_("Label '%s' discouraged. Recommended label names start "
+			"with an alphabetic character and use only alphanumeric "
+			"characters, dashes and underscores."), label);
+}
+
 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 +136,30 @@ 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;
 		}
 
+		/* The label token may have no argument, so no trailing ':' */
+		if (skip_prefix(copyfrom, "label", &body)) {
+			struct strbuf sb = STRBUF_INIT;
+			skip_prefix(body, ":", &body);
+			strbuf_add(&sb, body, nextat - body);
+			if (!item->labels) {
+				item->labels = xmalloc(sizeof(*item->labels));
+				string_list_init(item->labels, 1);
+			} else
+				die(_("multiple labels not supported in pathspec magic"));
+			string_list_split(item->labels, sb.buf, ' ', -1);
+			string_list_remove_empty_items(item->labels, 0);
+			strbuf_release(&sb);
+			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 +468,7 @@ void parse_pathspec(struct pathspec *pathspec,
 	for (i = 0; i < n; i++) {
 		unsigned short_magic;
 		entry = argv[i];
-
+		item[i].labels = NULL;
 		item[i].magic = prefix_pathspec(item + i, &short_magic,
 						argv + i, flags,
 						prefix, prefixlen, entry);
@@ -447,6 +490,12 @@ 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].labels) {
+			struct string_list_item *si;
+			for_each_string_list_item(si, item[i].labels)
+				validate_label_name(si->string);
+		}
 	}
 
 	if (nr_exclude == n)
@@ -502,6 +551,13 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
 
 void free_pathspec(struct pathspec *pathspec)
 {
+	int i;
+	for (i = 0; i < pathspec->nr; i++) {
+		if (pathspec->items[i].labels)
+			string_list_clear(pathspec->items[i].labels, 1);
+		free(pathspec->items[i].labels);
+	}
+
 	free(pathspec->items);
 	pathspec->items = NULL;
 }
diff --git a/pathspec.h b/pathspec.h
index 0c11262..12ed2b8 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -32,6 +32,7 @@ struct pathspec {
 		int len, prefix;
 		int nowildcard_len;
 		int flags;
+		struct string_list *labels;
 	} *items;
 };
 
diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
new file mode 100755
index 0000000..1fe5ed7
--- /dev/null
+++ b/t/t6134-pathspec-with-labels.sh
@@ -0,0 +1,164 @@
+#!/bin/sh
+
+test_description='test labels in pathspecs'
+. ./test-lib.sh
+
+test_expect_success 'setup a tree' '
+	mkdir sub &&
+	for p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel fileUnsetLabel fileSetLabel; do
+		: >$p &&
+		git add $p &&
+		: >sub/$p
+		git add sub/$p
+	done &&
+	git commit -m $p &&
+	git ls-files >actual &&
+	cat <<EOF >expect &&
+fileA
+fileAB
+fileAC
+fileB
+fileBC
+fileC
+fileNoLabel
+fileSetLabel
+fileUnsetLabel
+sub/fileA
+sub/fileAB
+sub/fileAC
+sub/fileB
+sub/fileBC
+sub/fileC
+sub/fileNoLabel
+sub/fileSetLabel
+sub/fileUnsetLabel
+EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'pathspec with labels and non existent .gitattributes' '
+	git ls-files ":(label)" >actual &&
+	git ls-files >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pathspec with labels and non existent .gitattributes' '
+	git ls-files ":(label:a)" >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'setup .gitattributes' '
+	cat <<EOF >.gitattributes &&
+fileA label=labelA
+fileB label=labelB
+fileC label=labelC
+fileAB label=labelA,labelB
+fileAC label=labelA,labelC
+fileBC label=labelB,labelC
+fileUnsetLabel -label
+fileSetLabel label
+EOF
+	git add .gitattributes &&
+	git commit -m "add attributes"
+'
+
+test_expect_success 'check for any label' '
+	cat <<EOF >expect &&
+.gitattributes
+fileA
+fileAB
+fileAC
+fileB
+fileBC
+fileC
+fileNoLabel
+fileSetLabel
+sub/fileA
+sub/fileAB
+sub/fileAC
+sub/fileB
+sub/fileBC
+sub/fileC
+sub/fileNoLabel
+sub/fileSetLabel
+EOF
+	git ls-files ":(label)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check specific label' '
+	cat <<EOF >expect &&
+fileA
+fileAB
+fileAC
+fileSetLabel
+sub/fileA
+sub/fileAB
+sub/fileAC
+sub/fileSetLabel
+EOF
+	git ls-files ":(label:labelA)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check label with 2 labels' '
+	cat <<EOF >expect &&
+fileAB
+fileSetLabel
+sub/fileAB
+sub/fileSetLabel
+EOF
+	git ls-files ":(label:labelA labelB)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check label with more labels' '
+	test_must_fail git ls-files ":(label:labelA,label:labelB)" 2>actual &&
+	test_i18ngrep "not supported" actual
+'
+
+test_expect_success 'check label with more labels but excluded path' '
+	cat <<EOF >expect &&
+fileAB
+fileB
+fileBC
+fileSetLabel
+EOF
+	git ls-files ":(label:labelB)" ":(exclude)sub/" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check label excluding other labels' '
+	cat <<EOF >expect &&
+fileAB
+fileB
+fileBC
+fileSetLabel
+sub/fileAB
+sub/fileB
+EOF
+	git ls-files ":(label:labelB)" ":(exclude,label:labelC)sub/" >actual &&
+	test_cmp expect actual
+'
+
+
+test_expect_success 'check for explicit unlabeled paths' '
+	cat <<EOF >expect &&
+fileUnsetLabel
+sub/fileUnsetLabel
+EOF
+	git ls-files . ":(exclude,label)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check for paths that have no label' '
+	cat <<EOF >expect &&
+.gitattributes
+fileNoLabel
+sub/fileNoLabel
+EOF
+	git ls-files ":(label)" ":(exclude,label:labelA)" ":(exclude,label:labelB)"  ":(exclude,label:labelC)"  >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.8.2.401.g6647087.dirty

Re: [PATCH 5/5] pathspec: record labels

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Labels were originally designed to manage large amount of
> submodules, the discussion steered this in a more general
> direction, such that other files can be labeled as well.

s/other files/any path/.

> Labels are meant to describe arbitrary set of files, which
> is not described via the tree layout.

Let's avoid "are meant to", which is merely to give you an excuse to
say "it was meant to ... but the implementation did not quite achieve
that goal".

    The "label" attribute can be attached to paths, and the pathspec
    mechanism is extended via the new ":(label=X)pattern/to/match"
    syntax to filter paths so that it requires paths to not just
    match the given pattern but also have the specified labels
    attached for them to be chosen.

or something?

> +Attaching labels to path
> +~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +`label`
> +^^^^^^^^^^
> +By the value of this attribute you can specify a list of arbitrary strings
> +to be attached to a path as labels. These labels can be used for
> +easier paths matching using pathspecs (linkgit:gitglossary[1]).
> +It is recommended to use only alphanumeric characters, dashes and
> +underscores for the labels.

Make this recomendation stronger to requirement.  It is always
possible to loosen it later, but once we allow random things even
with a warning, it gets impossible to take them back. Users will say
"Even though we got a warning, it used to correctly filter; now Git
is broken and my setup does not work."

> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> index 8ad29e6..f990017 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -384,6 +384,10 @@ full pathname may have special meaning:
>  +
>  Glob magic is incompatible with literal magic.
>  
> +label:<white space separated list>;;
> +	Additionally to matching the pathspec, the path must habe a 'label'
> +	attribute having set all labels listed here.

s/habe/have/

> +	if (ATTR_TRUE(check.value))
> +		ret = 1; /* has all the labels */

So this is "pretend that I have all the labels under the sun".

> +	else if (ATTR_FALSE(check.value))
> +		ret = 0; /* has no labels */

I do not see a value in this.  What difference does it have compared
to having a "label=" that explicitly says "I do not have one"?  What
is your answer to an end-user question "When should I say 'label='
and when should I say '!label'?"

Just forbid it for now; we may find a better use for it later.

> +	else if (ATTR_UNSET(check.value))
> +		/*
> +		 * If no label was specified this matches, otherwise
> +		 * there is a missing label.
> +		 */
> +		ret = (required_labels->nr > 0) ? 0 : 1;

Hmm, I can see that this is making things more complicated to
explain and understand, but I cannot see what the expected use case
is.

Unless there is any compelling use case, I'd say we should forbid it
for now.

> +	else {
> +		struct string_list_item *si;
> +		struct string_list attr_labels = STRING_LIST_INIT_DUP;
> +		string_list_split(&attr_labels, check.value, ',', -1);
> +		string_list_sort(&attr_labels);

Filter out a non-compliant label values here, so that they are
ignored from day one.  That way you would not have to deal with "I
know I got the warning, but it used to work and you broke it" later.

> +		ret = 1;
> +		for_each_string_list_item(si, required_labels) {
> +			if (!string_list_has_string(&attr_labels, si->string)) {
> +				ret = 0;
> +				break;
> +			}
> +		}
> +		string_list_clear(&attr_labels, 0);
> +	}
> +
> +	return ret;
> +}

> +static void validate_label_name(const char *label)
> +{
> +	if (check_valid_label_name(label))
> +		warning(_("Label '%s' discouraged. Recommended label names start "
> +			"with an alphabetic character and use only alphanumeric "
> +			"characters, dashes and underscores."), label);
> +}

Instead of returning void, parrot the return value from
check_valid_label_name().

> +		/* The label token may have no argument, so no trailing ':' */

Why close the door for future pathspec magic "labelfoo" by being
unnecessarily[*1*] lenient?

    [*1*] Does ":(label)" mean anything useful, and is there a good
          reason why it should behave differently from ":(label:)"?

Unless there is a good reason why users would want ":(label)", I'd
say we should make it a syntax error.

> +		if (skip_prefix(copyfrom, "label", &body)) {
> +			struct strbuf sb = STRBUF_INIT;
> +			skip_prefix(body, ":", &body);
> +			strbuf_add(&sb, body, nextat - body);
> +			if (!item->labels) {
> +				item->labels = xmalloc(sizeof(*item->labels));
> +				string_list_init(item->labels, 1);
> +			} else
> +				die(_("multiple labels not supported in pathspec magic"));

I am NOT suggesting to make this enhancement in the prototype to
allow us experiment with submodule selection use case, but this is
an obvious place to allow

	:(label=A B):(label=C D)

to mean ((A & B) | (C & D)) by making item->labels an array of set
of labels.

And no, I do not think arbitrary boolean expression is too complex
to understand to the end-users, especially if we have to stay within
the pathspec magic syntax.  And my gut feeling is that it is not
worth it to support anything more complex than "OR of these ANDed
ones".

> +			string_list_split(item->labels, sb.buf, ' ', -1);
> +			string_list_remove_empty_items(item->labels, 0);
> +			strbuf_release(&sb);
> +			continue;

The data structure to record the "required labels" is shared
knowledge between this function and the has_all_labels() and nobody
else knows it is done with string_list, so I think this is a good
balance between expediency and future optimization possibilities (I
am anticipating that linear search of string list would be found as
performance bottleneck).

> @@ -447,6 +490,12 @@ 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].labels) {
> +			struct string_list_item *si;
> +			for_each_string_list_item(si, item[i].labels)
> +				validate_label_name(si->string);

Reject a request to use a label that does not validate here.

The series is surprisingly simple and was a pleasant read (I no
longer recall how this compares with the earlier change to do this
as [submodule "x"] label=X).  Especially, given that the first two
patches are unrelated clean-ups that can be split out, this now
consists of merely 3 patches, each of which is an easily digestable
size.

Re: [PATCH 5/5] pathspec: record labels

[Stefan Beller]

On Sat, May 14, 2016 at 12:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Labels were originally designed to manage large amount of
>> submodules, the discussion steered this in a more general
>> direction, such that other files can be labeled as well.
>
> s/other files/any path/.
>
>> Labels are meant to describe arbitrary set of files, which
>> is not described via the tree layout.
>
> Let's avoid "are meant to", which is merely to give you an excuse to
> say "it was meant to ... but the implementation did not quite achieve
> that goal".
>
>     The "label" attribute can be attached to paths, and the pathspec
>     mechanism is extended via the new ":(label=X)pattern/to/match"

I wasn't sure about whether we want to have '=' or ':' as the separator
of "label" and its values. The prefix which is passed around uses
a colon instead of equal sign, so I thought consistency would be neat
there. But as that is only internal, we can be inconsistent to a new
public feature, so '=' seems better for the labeling system.

>     syntax to filter paths so that it requires paths to not just
>     match the given pattern but also have the specified labels
>     attached for them to be chosen.
>
> or something?
>
>> +Attaching labels to path
>> +~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +`label`
>> +^^^^^^^^^^
>> +By the value of this attribute you can specify a list of arbitrary strings
>> +to be attached to a path as labels. These labels can be used for
>> +easier paths matching using pathspecs (linkgit:gitglossary[1]).
>> +It is recommended to use only alphanumeric characters, dashes and
>> +underscores for the labels.
>
> Make this recomendation stronger to requirement.  It is always
> possible to loosen it later, but once we allow random things even
> with a warning, it gets impossible to take them back. Users will say
> "Even though we got a warning, it used to correctly filter; now Git
> is broken and my setup does not work."

I'll change the text here with s/recommended/required/ and error out
for that.

But still we do not enforce it early enough. Some crazy upstream may
add some labels which do not follow the requirements and then
tell downstream to run a command like `git foo "(label=!@#$)".
and then downstream curses at upstream as well as Git.

>
> s/habe/have/

done

>
>> +     if (ATTR_TRUE(check.value))
>> +             ret = 1; /* has all the labels */
>
> So this is "pretend that I have all the labels under the sun".
>
>> +     else if (ATTR_FALSE(check.value))
>> +             ret = 0; /* has no labels */
>
> I do not see a value in this.  What difference does it have compared
> to having a "label=" that explicitly says "I do not have one"?  What
> is your answer to an end-user question "When should I say 'label='
> and when should I say '!label'?"
>
> Just forbid it for now; we may find a better use for it later.

I don't think we want to die in that code as it is in the data already.
If we were to die in that case, you cannot query any path with
labels any more if asking for any kind of label.

>
>> +     else if (ATTR_UNSET(check.value))
>> +             /*
>> +              * If no label was specified this matches, otherwise
>> +              * there is a missing label.
>> +              */
>> +             ret = (required_labels->nr > 0) ? 0 : 1;
>
> Hmm, I can see that this is making things more complicated to
> explain and understand, but I cannot see what the expected use case
> is.
>
> Unless there is any compelling use case, I'd say we should forbid it
> for now.

We need to allow the UNSET case, as otherwise you'd need to label
any path if using labels? In the next iteration I'll go with

        if (ATTR_TRUE(check.value))
                ret = 1; /* has all the labels */
        else if (ATTR_FALSE(check.value))
                die(_("Label attributes must not be unset"));
        else if (ATTR_UNSET(check.value))
                ret = 0; /* has no labels */
        else {
                ...
>
>> +     else {
>> +             struct string_list_item *si;
>> +             struct string_list attr_labels = STRING_LIST_INIT_DUP;
>> +             string_list_split(&attr_labels, check.value, ',', -1);
>> +             string_list_sort(&attr_labels);
>
> Filter out a non-compliant label values here, so that they are
> ignored from day one.  That way you would not have to deal with "I
> know I got the warning, but it used to work and you broke it" later.

So you're saying we should not die(...) but just ignore those labels?

>
>> +             ret = 1;
>> +             for_each_string_list_item(si, required_labels) {
>> +                     if (!string_list_has_string(&attr_labels, si->string)) {
>> +                             ret = 0;
>> +                             break;
>> +                     }
>> +             }
>> +             string_list_clear(&attr_labels, 0);
>> +     }
>> +
>> +     return ret;
>> +}
>
>> +static void validate_label_name(const char *label)
>> +{
>> +     if (check_valid_label_name(label))
>> +             warning(_("Label '%s' discouraged. Recommended label names start "
>> +                     "with an alphabetic character and use only alphanumeric "
>> +                     "characters, dashes and underscores."), label);
>> +}
>
> Instead of returning void, parrot the return value from
> check_valid_label_name().
>
>> +             /* The label token may have no argument, so no trailing ':' */
>
> Why close the door for future pathspec magic "labelfoo" by being
> unnecessarily[*1*] lenient?
>
>     [*1*] Does ":(label)" mean anything useful, and is there a good
>           reason why it should behave differently from ":(label:)"?

ok fixed.

>
> Unless there is a good reason why users would want ":(label)", I'd
> say we should make it a syntax error.
>
>> +             if (skip_prefix(copyfrom, "label", &body)) {
>> +                     struct strbuf sb = STRBUF_INIT;
>> +                     skip_prefix(body, ":", &body);
>> +                     strbuf_add(&sb, body, nextat - body);
>> +                     if (!item->labels) {
>> +                             item->labels = xmalloc(sizeof(*item->labels));
>> +                             string_list_init(item->labels, 1);
>> +                     } else
>> +                             die(_("multiple labels not supported in pathspec magic"));
>
> I am NOT suggesting to make this enhancement in the prototype to
> allow us experiment with submodule selection use case, but this is
> an obvious place to allow
>
>         :(label=A B):(label=C D)
>
> to mean ((A & B) | (C & D)) by making item->labels an array of set
> of labels.
>
> And no, I do not think arbitrary boolean expression is too complex
> to understand to the end-users, especially if we have to stay within
> the pathspec magic syntax.  And my gut feeling is that it is not
> worth it to support anything more complex than "OR of these ANDed
> ones".

That makes sense.

>
>> +                     string_list_split(item->labels, sb.buf, ' ', -1);
>> +                     string_list_remove_empty_items(item->labels, 0);
>> +                     strbuf_release(&sb);
>> +                     continue;
>
> The data structure to record the "required labels" is shared
> knowledge between this function and the has_all_labels() and nobody
> else knows it is done with string_list, so I think this is a good
> balance between expediency and future optimization possibilities (I
> am anticipating that linear search of string list would be found as
> performance bottleneck).
>
>> @@ -447,6 +490,12 @@ 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].labels) {
>> +                     struct string_list_item *si;
>> +                     for_each_string_list_item(si, item[i].labels)
>> +                             validate_label_name(si->string);
>
> Reject a request to use a label that does not validate here.

done.

Re: [PATCH 5/5] pathspec: record labels

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Let's avoid "are meant to", which is merely to give you an excuse to
>> say "it was meant to ... but the implementation did not quite achieve
>> that goal".
>>
>>     The "label" attribute can be attached to paths, and the pathspec
>>     mechanism is extended via the new ":(label=X)pattern/to/match"
>
> I wasn't sure about whether we want to have '=' or ':' as the separator
> of "label" and its values. ...

Oh, sorry, the above wasn't a suggestion to change : to = at all.  I
was merely being sloppy in the details that is irrelevant to my main
point (which is, it is better spend the bits you spent for "meant
to" instead for saying clearly what it does).

> ... But as that is only internal, we can be inconsistent to a new
> public feature, so '=' seems better for the labeling system.

I can buy that, too.  Good that my sloppy wording helped you think
about it further ;-).

> But still we do not enforce it early enough. Some crazy upstream may
> add some labels which do not follow the requirements and then
> tell downstream to run a command like `git foo "(label=!@#$)".
> and then downstream curses at upstream as well as Git.

That is why it is "warn and ignore", not "die".

>>> +     if (ATTR_TRUE(check.value))
>>> +             ret = 1; /* has all the labels */
>>
>> So this is "pretend that I have all the labels under the sun".
>>
>>> +     else if (ATTR_FALSE(check.value))
>>> +             ret = 0; /* has no labels */
>>
>> I do not see a value in this.  What difference does it have compared
>> to having a "label=" that explicitly says "I do not have one"?  What
>> is your answer to an end-user question "When should I say 'label='
>> and when should I say '!label'?"
>>
>> Just forbid it for now; we may find a better use for it later.
>
> I don't think we want to die in that code as it is in the data already.

Is it?  I think this is coming from the command line pathspec magic,
not from .gitattributes file recorded in the tree.

> We need to allow the UNSET case, as otherwise you'd need to label
> any path if using labels?

You do need UNSET (roughly, "no label is mentioned for the path").

If I want to say "Everything under Documentation/ and also things
with label=doc", I'd say

	git cmd "Documentation/" ":(label=doc)"

and no path in Documentation/ need any label, i.e. for them, the
labels are unset.  They will not be caught with ":(label=doc)"
because they are not set, but that is OK.

FALSE is different from UNSET.  It needs an explicit mention, i.e.

	*.doc	doc
        false.doc	-doc

What's the difference between saying "-doc" and not saying anything?
If you really want to explicitly say "doc attribute is unset for
this path (perhaps because you may have other patterns that set the
doc elsewhere), you would say "!doc", and you already have code for
that.

>         if (ATTR_TRUE(check.value))
>                 ret = 1; /* has all the labels */
>         else if (ATTR_FALSE(check.value))
>                 die(_("Label attributes must not be unset"));

The message is wrong.  You are dying because the user explicitly set
it to false, not because the user made it unset.

>         else if (ATTR_UNSET(check.value))
>                 ret = 0; /* has no labels */

>>> +             struct string_list_item *si;
>>> +             struct string_list attr_labels = STRING_LIST_INIT_DUP;
>>> +             string_list_split(&attr_labels, check.value, ',', -1);
>>> +             string_list_sort(&attr_labels);
>>
>> Filter out a non-compliant label values here, so that they are
>> ignored from day one.  That way you would not have to deal with "I
>> know I got the warning, but it used to work and you broke it" later.
>
> So you're saying we should not die(...) but just ignore those labels?

Do not die() but warn-and-ignore when you see funnies in
.gitattributes; do die() if you see funnies in pathspec magic.

>> I am NOT suggesting to make this enhancement in the prototype to
>> allow us experiment with submodule selection use case, but this is
>> an obvious place to allow
>>
>>         :(label=A B):(label=C D)
>>
>> to mean ((A & B) | (C & D)) by making item->labels an array of set
>> of labels.
>>
>> And no, I do not think arbitrary boolean expression is too complex

s/do not/do/

>> to understand to the end-users, especially if we have to stay within
>> the pathspec magic syntax.  And my gut feeling is that it is not
>> worth it to support anything more complex than "OR of these ANDed
>> ones".
>
> That makes sense.

Thanks.

Re: [PATCH 5/5] pathspec: record labels

[Stefan Beller]

On Mon, May 16, 2016 at 10:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> Let's avoid "are meant to", which is merely to give you an excuse to
>>> say "it was meant to ... but the implementation did not quite achieve
>>> that goal".
>>>
>>>     The "label" attribute can be attached to paths, and the pathspec
>>>     mechanism is extended via the new ":(label=X)pattern/to/match"
>>
>> I wasn't sure about whether we want to have '=' or ':' as the separator
>> of "label" and its values. ...
>
> Oh, sorry, the above wasn't a suggestion to change : to = at all.  I
> was merely being sloppy in the details that is irrelevant to my main
> point (which is, it is better spend the bits you spent for "meant
> to" instead for saying clearly what it does).
>
>> ... But as that is only internal, we can be inconsistent to a new
>> public feature, so '=' seems better for the labeling system.
>
> I can buy that, too.  Good that my sloppy wording helped you think
> about it further ;-).
>
>> But still we do not enforce it early enough. Some crazy upstream may
>> add some labels which do not follow the requirements and then
>> tell downstream to run a command like `git foo "(label=!@#$)".
>> and then downstream curses at upstream as well as Git.
>
> That is why it is "warn and ignore", not "die".

So "warn and ignore" for data from .gitattributes and die for
commandline arguments? That makes sense.

>
>>>> +     if (ATTR_TRUE(check.value))
>>>> +             ret = 1; /* has all the labels */
>>>
>>> So this is "pretend that I have all the labels under the sun".
>>>
>>>> +     else if (ATTR_FALSE(check.value))
>>>> +             ret = 0; /* has no labels */
>>>
>>> I do not see a value in this.  What difference does it have compared
>>> to having a "label=" that explicitly says "I do not have one"?  What
>>> is your answer to an end-user question "When should I say 'label='
>>> and when should I say '!label'?"
>>>
>>> Just forbid it for now; we may find a better use for it later.
>>
>> I don't think we want to die in that code as it is in the data already.
>
> Is it?  I think this is coming from the command line pathspec magic,
> not from .gitattributes file recorded in the tree.

The 'check.value' is coming from the .gitattributes.

The 'required_labels' string list comes from the command line.
("The command line requires these labels")

>
>> We need to allow the UNSET case, as otherwise you'd need to label
>> any path if using labels?
>
> You do need UNSET (roughly, "no label is mentioned for the path").
>
> If I want to say "Everything under Documentation/ and also things
> with label=doc", I'd say
>
>         git cmd "Documentation/" ":(label=doc)"
>
> and no path in Documentation/ need any label, i.e. for them, the
> labels are unset.  They will not be caught with ":(label=doc)"
> because they are not set, but that is OK.

right. that is what the end user should be able to do. That also means,
we need to handle files with UNSET labels (all under Documentation/)

>
> FALSE is different from UNSET.  It needs an explicit mention, i.e.
>
>         *.doc   doc
>         false.doc       -doc

and this is in the .gitattribues, so warn-and-ignore and by ignore
we mean treat it as UNSET?

>
> What's the difference between saying "-doc" and not saying anything?
> If you really want to explicitly say "doc attribute is unset for
> this path (perhaps because you may have other patterns that set the
> doc elsewhere), you would say "!doc", and you already have code for
> that.
>
>>         if (ATTR_TRUE(check.value))
>>                 ret = 1; /* has all the labels */
>>         else if (ATTR_FALSE(check.value))
>>                 die(_("Label attributes must not be unset"));
>
> The message is wrong.  You are dying because the user explicitly set
> it to false, not because the user made it unset.

Ok, so here is the warn-and-ignore code:


        if (ATTR_TRUE(check.value))
                ret = 1; /* has all the labels */
        else if (ATTR_FALSE(check.value)) {
                warning(_("Path '%s': Label must not be false. Treat
as if no label was set"), path);
                ret = 0;
        else if (ATTR_UNSET(check.value))
                ret = 0; /* has no labels */
        else {

Thanks,
Stefan

Re: [PATCH 5/5] pathspec: record labels

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So "warn and ignore" for data from .gitattributes and die for
> commandline arguments? That makes sense.

Yes.

On the "command line" front, because we may want to give different
meanings to these two entries in the future:

        :(label=-doc)Documentation/
	:(label=!doc)Documentation/

we should diagnose -doc (FALSE) as an error, not treating it as the
same as !doc (UNSET).  And we should warn and ignore -doc (FALSE) in
.gitattributes.  Yes, ignoring it would be more or less equivalent
to treating it as UNSET, but because we may use -doc (FALSE) for a
better purpose later, we should still warn.

> Ok, so here is the warn-and-ignore code:
>
>
>         if (ATTR_TRUE(check.value))
>                 ret = 1; /* has all the labels */
>         else if (ATTR_FALSE(check.value)) {
>                 warning(_("Path '%s': Label must not be false. Treat
> as if no label was set"), path);
>                 ret = 0;

s/Treat as if .../The -label may be used differently in future
versions of Git, so do not use it/;

But if we are going in the direction of :(attr:crlf=auto), all this
discussion is moot, isn't it?  I haven't formed a firm opinion on
this, but it sure does sound tempting, doesn't it?

Re: [PATCH 5/5] pathspec: record labels

[Stefan Beller]

On Mon, May 16, 2016 at 11:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So "warn and ignore" for data from .gitattributes and die for
>> commandline arguments? That makes sense.
>
> Yes.
>
> On the "command line" front, because we may want to give different
> meanings to these two entries in the future:
>
>         :(label=-doc)Documentation/
>         :(label=!doc)Documentation/

Yes but both of these already errors out with:

    Label -doc': Label names must start with an
    alphabetic character and use only alphanumeric
    characters, dashes and underscores.
    fatal: Labels in the wrong syntax are prohibited.

The command line arguments are not parsed via the attr system.
Only data from .gitattributes are parsed via the attr system.

I can add tests for those to fail though.

>
> we should diagnose -doc (FALSE) as an error, not treating it as the
> same as !doc (UNSET).  And we should warn and ignore -doc (FALSE) in
> .gitattributes.  Yes, ignoring it would be more or less equivalent
> to treating it as UNSET, but because we may use -doc (FALSE) for a
> better purpose later, we should still warn.
>
>> Ok, so here is the warn-and-ignore code:
>>
>>
>>         if (ATTR_TRUE(check.value))
>>                 ret = 1; /* has all the labels */
>>         else if (ATTR_FALSE(check.value)) {
>>                 warning(_("Path '%s': Label must not be false. Treat
>> as if no label was set"), path);
>>                 ret = 0;
>
> s/Treat as if .../The -label may be used differently in future
> versions of Git, so do not use it/;

"... but for now Git treats it as if it is not set at all" is a valuable
information to the user, still?

That code path is only used for data coming from .gitattributes,
so we can bike shed the best warning message.

>
> But if we are going in the direction of :(attr:crlf=auto), all this
> discussion is moot, isn't it?  I haven't formed a firm opinion on
> this, but it sure does sound tempting, doesn't it?
>

That direction sounds scary as people may use any sort of labels, so
we can no longer add labels later on sanely.

    :(attr:random-attr=foo)

should also fall into the "warn and ignore" space. We only want to allow

    :(attr:known-attr)
    :(attr:label=..)

instead of label= we may want to allow a little bit more, such as
abbreviated  'l='
or 'group=', but not any sort of labeling.

Re: [PATCH 5/5] pathspec: record labels

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> "... but for now Git treats it as if it is not set at all" is a valuable
> information to the user, still?

Not at all.  "What you are using is wrong and there is no guarantee
what behaviour you would get" is the message we need to convey.

Re: [PATCH 5/5] pathspec: record labels

[Stefan Beller]

On Mon, May 16, 2016 at 12:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> "... but for now Git treats it as if it is not set at all" is a valuable
>> information to the user, still?
>
> Not at all.  "What you are using is wrong and there is no guarantee
> what behaviour you would get" is the message we need to convey.

    "Path '%s': Ignoring label set to false; This may behave
differently in future versions of Git."

Maybe even with s/may/will/

I think that conveys both the message about the future as well as
"what happened just now in that command"

Re: [PATCH 5/5] pathspec: record labels

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>     "Path '%s': Ignoring label set to false; This may behave
> differently in future versions of Git."

I agree that mentioning "ignoring" is good enough.  I think our
recent messages begin with lowercase, though.

Re: [PATCH 5/5] pathspec: record labels

[Stefan Beller]

> I am NOT suggesting to make this enhancement in the prototype to
> allow us experiment with submodule selection use case, but this is
> an obvious place to allow
>
>         :(label=A B):(label=C D)
>
> to mean ((A & B) | (C & D)) by making item->labels an array of set
> of labels.

This is what already works with the series. Or rather:

    ":(label=A B)" ":(label=C D)"

works as you would expect for (A&B) | (C&D).

So I am a bit hesitant to replace the string list by an array
of stringlists or such, as the future enhancement can also do that?

The enhancement may bring in more expressions into the label string,
so it may even parse that string into a tree for Lexicographical order
instead of just using an array of lists.

>
> And no, I do not think arbitrary boolean expression is too complex
> to understand to the end-users, especially if we have to stay within
> the pathspec magic syntax.  And my gut feeling is that it is not
> worth it to support anything more complex than "OR of these ANDed
> ones".
>
>> +                     string_list_split(item->labels, sb.buf, ' ', -1);
>> +                     string_list_remove_empty_items(item->labels, 0);
>> +                     strbuf_release(&sb);
>> +                     continue;
>
> The data structure to record the "required labels" is shared
> knowledge between this function and the has_all_labels() and nobody
> else knows it is done with string_list, so I think this is a good
> balance between expediency and future optimization possibilities (I
> am anticipating that linear search of string list would be found as
> performance bottleneck).
>

Re: [PATCH 5/5] pathspec: record labels

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> I am NOT suggesting to make this enhancement in the prototype to
>> allow us experiment with submodule selection use case, but this is
>> an obvious place to allow
>>
>>         :(label=A B):(label=C D)
>>
>> to mean ((A & B) | (C & D)) by making item->labels an array of set
>> of labels.
>
> This is what already works with the series. Or rather:
>
>     ":(label=A B)" ":(label=C D)"
>
> works as you would expect for (A&B) | (C&D).

That is "duplicationg path".  I was envisioning a single

	":(label=A B):(label=C D)tediously/long/path/because/java/"

a shorter and sweeter way to say your two pathspec variant, i.e.

	":(label=A B)tediously/long/path/because/java/" \
        ":(label=C D)tediously/long/path/because/java/"