[RFC PATCH 0/4] pathspec labels [WAS: submodule groups]

STALE3748d

7 messages, 2 authors, 2016-06-16 · open the first message on its own page

[RFC PATCH 0/4] pathspec labels [WAS: submodule groups]

From: Stefan Beller <hidden>
Date: 2016-06-16 02:19:21

After some fruitful discussion[1] in which Junio suggested trying a very
different route[2] that is more general and not submodule related, I considered
doing a mock for this.

This lets you label arbitrary pathspecs, e.g. in git.git we may want to have:

    t/t[0-9]*.sh label=tests
    
such that

    $ git log --author=Beller ":(label=tests)"
    
would show all commits in which I touched tests.

This has suprisingly few lines of code, as the first 3 patches are refactoring.
The actual new feature is in the last patch.

This would solve the submodule issues I want to solve, as I can produce a
.gitattributes like:

    ./submodule1 label=default
    ./submodule2 label=default
    ./submodule3 label=optional-feature1
    
and then I'd instruct the users to clone like this:

    git clone .. superproject
    cd superproject
    git submodule update --init :(label:default)
    
The second part of the submodule series to collapse these three commands
will come as an extra series later, then.

What annoys me here:
Attributes can only be set once at the moment, there is no way to collect all
attributes. If we'd have such a collection feature we would be able to
configure this:

    *.[ch] label=C,code
    contrib/** label=oldstuff
    
and run this:

    git status ":(label:C oldstuff)"
    
which would be the equivalent to

    contrib/**.[ch]
    
as in this proposed implementation the labels which are given within one
pathspec item are logical AND. To get the logical OR, you'd have to give multiple
pathspec items, i.e. ":(label:C)" ":(label:oldstuff)"

Feedback welcome!

Thanks,
Stefan

[1] http://thread.gmane.org/gmane.comp.version-control.git/294212
[2] http://thread.gmane.org/gmane.comp.version-control.git/294212/focus=294391

Stefan Beller (4):
  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/glossary-content.txt            |   5 ++
 Documentation/technical/api-gitattributes.txt |   2 +-
 attr.h                                        |   1 +
 dir.c                                         |  31 ++++++++
 pathspec.c                                    | 109 +++++++++++++++++---------
 pathspec.h                                    |   1 +
 t/t6134-pathspec-with-labels.sh               |  91 +++++++++++++++++++++
 7 files changed, 201 insertions(+), 39 deletions(-)
 create mode 100755 t/t6134-pathspec-with-labels.sh

-- 
2.8.2.400.g66c4903.dirty

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

From: Stefan Beller <hidden>
Date: 2016-06-16 02:19:21

`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 <redacted>
---
 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.400.g66c4903.dirty

[PATCH 1/4] Documentation: correct typo in example for querying attributes

From: Stefan Beller <hidden>
Date: 2016-06-16 02:19:21

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 <redacted>
---
 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.400.g66c4903.dirty

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

From: Stefan Beller <hidden>
Date: 2016-06-16 02:19:21

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 <redacted>
---
 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.400.g66c4903.dirty

[PATCH 4/4] pathspec: record labels

From: Stefan Beller <hidden>
Date: 2016-06-16 02:19:21

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 <redacted>
---
 Documentation/glossary-content.txt |  5 +++
 attr.h                             |  1 +
 dir.c                              | 31 +++++++++++++
 pathspec.c                         | 24 +++++++++-
 pathspec.h                         |  1 +
 t/t6134-pathspec-with-labels.sh    | 91 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100755 t/t6134-pathspec-with-labels.sh
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 8ad29e6..a1fc9e0 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -362,6 +362,11 @@ glob;;
 	For example, "Documentation/{asterisk}.html" matches
 	"Documentation/git.html" but not "Documentation/ppc/ppc.html"
 	or "tools/perf/Documentation/perf.html".
+
+label:<white space separated list>;;
+	Labels can be assigned to pathspecs in the .gitattributes file.
+	By specifying a list of labels the pattern will match only
+	files which have all of the listed labels.
 +
 Two consecutive asterisks ("`**`") in patterns matched against
 full pathname may have special meaning:
diff --git a/attr.h b/attr.h
index 8b08d33..f6fc7c3 100644
--- a/attr.h
+++ b/attr.h
@@ -18,6 +18,7 @@ extern const char git_attr__false[];
 #define ATTR_TRUE(v) ((v) == git_attr__true)
 #define ATTR_FALSE(v) ((v) == git_attr__false)
 #define ATTR_UNSET(v) ((v) == NULL)
+#define ATTR_CUSTOM(v) (!(ATTR_UNSET(v) || ATTR_FALSE(v) || ATTR_TRUE(v)))
 
 /*
  * Send one or more git_attr_check to git_check_attr(), and
diff --git a/dir.c b/dir.c
index 656f272..51d5965 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,27 @@ int within_depth(const char *name, int namelen,
 	return 1;
 }
 
+void load_labels(const char *name, int namelen, struct string_list *list)
+{
+	static struct git_attr *attr;
+	struct git_attr_check check;
+	char *path = xmemdupz(name, namelen);
+
+	if (!attr)
+		attr = git_attr("label");
+	check.attr = attr;
+
+	if (git_check_attr(path, 1, &check))
+		die("git_check_attr died");
+
+	if (ATTR_CUSTOM(check.value)) {
+		string_list_split(list, check.value, ',', -1);
+		string_list_sort(list);
+	}
+
+	free(path);
+}
+
 #define DO_MATCH_EXCLUDE   1
 #define DO_MATCH_DIRECTORY 2
 
@@ -263,6 +285,15 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 	    strncmp(item->match, name - prefix, item->prefix))
 		return 0;
 
+	if (item->group) {
+		struct string_list has_labels = STRING_LIST_INIT_DUP;
+		struct string_list_item *si;
+		load_labels(name, namelen, &has_labels);
+		for_each_string_list_item(si, item->group)
+			if (!string_list_has_string(&has_labels, si->string))
+				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..c227c25 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -94,6 +94,7 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,
 {
 	int i;
 	const char *copyfrom = *copyfrom_;
+	const char *out;
 	/* longhand */
 	const char *nextat;
 	for (copyfrom = elt + 2;
@@ -117,6 +118,20 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,
 			continue;
 		}
 
+		if (skip_prefix(copyfrom, "label:", &out)) {
+			struct strbuf sb = STRBUF_INIT;
+			size_t l = nextat - out;
+			strbuf_add(&sb, out, l);
+			if (!item->group) {
+				item->group = xmalloc(sizeof(*item->group));
+				string_list_init(item->group, 1);
+			}
+			string_list_split(item->group, sb.buf, ' ', -1);
+			string_list_remove_empty_items(item->group, 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 +440,7 @@ void parse_pathspec(struct pathspec *pathspec,
 	for (i = 0; i < n; i++) {
 		unsigned short_magic;
 		entry = argv[i];
-
+		item[i].group = NULL;
 		item[i].magic = prefix_pathspec(item + i, &short_magic,
 						argv + i, flags,
 						prefix, prefixlen, entry);
@@ -502,6 +517,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].group)
+			string_list_clear(pathspec->items[i].group, 1);
+		free(pathspec->items[i].group);
+	}
+
 	free(pathspec->items);
 	pathspec->items = NULL;
 }
diff --git a/pathspec.h b/pathspec.h
index 0c11262..e3f7ebf 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -32,6 +32,7 @@ struct pathspec {
 		int len, prefix;
 		int nowildcard_len;
 		int flags;
+		struct string_list *group;
 	} *items;
 };
 
diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
new file mode 100755
index 0000000..0c061ce
--- /dev/null
+++ b/t/t6134-pathspec-with-labels.sh
@@ -0,0 +1,91 @@
+#!/bin/sh
+
+test_description='test labels in pathspecs'
+. ./test-lib.sh
+
+test_expect_success 'setup a tree' '
+	for p in file sub/file sub/sub/file sub/file2 sub/sub/sub/file sub2/file; do
+		if echo $p | grep /; then
+			mkdir -p $(dirname $p)
+		fi &&
+		: >$p &&
+		git add $p &&
+		git commit -m $p
+	done &&
+	git log --oneline --format=%s >actual &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'pathspec with labels and no .gitattributes exists' '
+	git ls-files ":(label:a)" >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'setup .gitattributes' '
+	cat <<-EOF >.gitattributes &&
+	/file label=b
+	sub/file label=a
+	sub/sub/* label=b,c
+	EOF
+	git add .gitattributes &&
+	git commit -m "add attributes"
+'
+
+test_expect_success 'check label' '
+	cat <<-EOF >expect &&
+	sub/file
+	EOF
+	git ls-files ":(label:a)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check label from label list' '
+	cat <<-EOF >expect &&
+	sub/sub/file
+	EOF
+	git ls-files ":(label:c)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check label with more labels' '
+	cat <<-EOF >expect &&
+	file
+	sub/sub/file
+	EOF
+	git ls-files ":(label:b)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check label with more labels but excluded path' '
+	cat <<-EOF >expect &&
+	sub/sub/file
+	EOF
+	git ls-files ":(label:b)" ":(exclude)./file" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check label specifying more labels' '
+	cat <<-EOF >expect &&
+	sub/sub/file
+	EOF
+	git ls-files ":(label:b c)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check label specifying more labels' '
+	cat <<-EOF >expect &&
+	sub/file
+	sub/sub/file
+	EOF
+	git ls-files ":(label:b c)" ":(label:a)" >actual &&
+	test_cmp expect actual
+'
+test_done
-- 
2.8.2.400.g66c4903.dirty

Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]

From: Duy Nguyen <hidden>
Date: 2016-06-16 02:19:23

On Fri, May 13, 2016 at 7:19 AM, Stefan Beller [off-list ref] wrote:
After some fruitful discussion[1] in which Junio suggested trying a very
different route[2] that is more general and not submodule related, I considered
doing a mock for this.

This lets you label arbitrary pathspecs, e.g. in git.git we may want to have:

    t/t[0-9]*.sh label=tests

such that

    $ git log --author=Beller ":(label=tests)"

would show all commits in which I touched tests.

This has suprisingly few lines of code, as the first 3 patches are refactoring.
The actual new feature is in the last patch.

This would solve the submodule issues I want to solve, as I can produce a
.gitattributes like:

    ./submodule1 label=default
    ./submodule2 label=default
    ./submodule3 label=optional-feature1

and then I'd instruct the users to clone like this:

    git clone .. superproject
    cd superproject
    git submodule update --init :(label:default)

The second part of the submodule series to collapse these three commands
will come as an extra series later, then.
Yeah. I can see the use of this, even without submodules.
What annoys me here:
Attributes can only be set once at the moment, there is no way to collect all
attributes. If we'd have such a collection feature we would be able to
configure this:

    *.[ch] label=C,code
    contrib/** label=oldstuff
Instead of putting everything in under the same attribute name
"label", make one attribute per label? Would this work?

*.[ch] c-group code-group

And the pathspec magic name can be simply "attr", any git attribute
can be used with it, e.g. :(attr:c-group)
and run this:

    git status ":(label:C oldstuff)"

which would be the equivalent to

    contrib/**.[ch]

as in this proposed implementation the labels which are given within one
pathspec item are logical AND. To get the logical OR, you'd have to give multiple
pathspec items, i.e. ":(label:C)" ":(label:oldstuff)"
It's even better if pathspec does not have to deal with combination
logic. Can attr macros deal with that (or if it can't at the moment,
can we improve it)?
-- 
Duy

Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]

From: Stefan Beller <hidden>
Date: 2016-06-16 02:19:24

On Sun, May 15, 2016 at 3:06 AM, Duy Nguyen [off-list ref] wrote:
Instead of putting everything in under the same attribute name
"label", make one attribute per label? Would this work?

*.[ch] c-group code-group

And the pathspec magic name can be simply "attr", any git attribute
can be used with it, e.g. :(attr:c-group)
So you want to be able to query something like:

    git ls-files ":(crlf=auto)"

as well?
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help