[PATCH 0/7] Comments and refactoring in attr.c

DORMANTno replies

8 messages, 1 author, 2016-06-15 · open the first message on its own page

[PATCH 0/7] Comments and refactoring in attr.c

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:51:48

This patch series adds comments for some non-obvious things in attr.c,
simplifies the interface of parse_attr(), and unrolls the loop over
passes in parse_attr_line() (which IMO makes the code easier to
understand).  It does not change the observable behavior in any way.

The patch series is against master, but can be rebased to next without
conflicts.

Michael Haggerty (7):
  Add a file comment
  Document struct match_attr
  Increment num_attr in parse_attr_line(), not parse_attr()
  Change parse_attr() to take a pointer to struct attr_state
  Determine the start of the states outside of the pass loop
  Change while loop into for loop
  Unroll the loop over passes

 attr.c |  113 +++++++++++++++++++++++++++++++++++----------------------------
 1 files changed, 63 insertions(+), 50 deletions(-)

-- 
1.7.6.8.gd2879

[PATCH 1/7] Add a file comment

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:51:48

Consolidate here a few general comments plus links to other
documentation.  Delete a comment with an out-of-date description of
the .gitattributes file format.

Signed-off-by: Michael Haggerty <redacted>
---
 attr.c |   26 ++++++++++----------------
 1 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/attr.c b/attr.c
index f6b3f7e..6bc7ae9 100644
--- a/attr.c
+++ b/attr.c

@@ -1,3 +1,12 @@
+/*
+ * Handle git attributes.  See gitattributes(5) for a description of
+ * the file syntax, and Documentation/technical/api-gitattributes.txt
+ * for a description of the API.
+ *
+ * One basic design decision here is that we are not going to support
+ * an insanely large number of attributes.
+ */
+
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "exec_cmd.h"

@@ -13,12 +22,7 @@ static const char git_attr__unknown[] = "(builtin)unknown";
 
 static const char *attributes_file;
 
-/*
- * The basic design decision here is that we are not going to have
- * insanely large number of attributes.
- *
- * This is a randomly chosen prime.
- */
+/* This is a randomly chosen prime. */
 #define HASHSIZE 257
 
 #ifndef DEBUG_ATTR

@@ -103,16 +107,6 @@ struct git_attr *git_attr(const char *name)
 	return git_attr_internal(name, strlen(name));
 }
 
-/*
- * .gitattributes file is one line per record, each of which is
- *
- * (1) glob pattern.
- * (2) whitespace
- * (3) whitespace separated list of attribute names, each of which
- *     could be prefixed with '-' to mean "set to false", '!' to mean
- *     "unset".
- */
-
 /* What does a matched pattern decide? */
 struct attr_state {
 	struct git_attr *attr;

-- 
1.7.6.8.gd2879

[PATCH 2/7] Document struct match_attr

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:51:48

Signed-off-by: Michael Haggerty <redacted>
---
 attr.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/attr.c b/attr.c
index 6bc7ae9..c33e413 100644
--- a/attr.c
+++ b/attr.c

@@ -113,6 +113,20 @@ struct attr_state {
 	const char *setto;
 };
 
+/*
+ * One rule, as from a .gitattributes file.
+ *
+ * If is_macro is true, then u.attr is a pointer to the git_attr being
+ * defined.
+ *
+ * If is_macro is false, then u.pattern points at the filename pattern
+ * to which the rule applies.  (The memory pointed to is part of the
+ * memory block allocated for the match_attr instance.)
+ *
+ * In either case, num_attr is the number of attributes affected by
+ * this rule, and state is an array listing them.  The attributes are
+ * listed as they appear in the file (macros unexpanded).
+ */
 struct match_attr {
 	union {
 		char *pattern;

-- 
1.7.6.8.gd2879

[PATCH 3/7] Increment num_attr in parse_attr_line(), not parse_attr()

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:51:48

num_attr is incremented iff parse_attr() returns non-NULL.  So do the
counting in parse_attr_line() instead of within parse_attr().  This
allows an integer rather than a pointer to an integer to be passed to
parse_attr().

Signed-off-by: Michael Haggerty <redacted>
---
 attr.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/attr.c b/attr.c
index c33e413..cac550d 100644
--- a/attr.c
+++ b/attr.c

@@ -140,7 +140,7 @@ struct match_attr {
 static const char blank[] = " \t\r\n";
 
 static const char *parse_attr(const char *src, int lineno, const char *cp,
-			      int *num_attr, struct match_attr *res)
+			      int num_attr, struct match_attr *res)
 {
 	const char *ep, *equals;
 	int len;

@@ -167,7 +167,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
 	} else {
 		struct attr_state *e;
 
-		e = &(res->state[*num_attr]);
+		e = &(res->state[num_attr]);
 		if (*cp == '-' || *cp == '!') {
 			e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
 			cp++;

@@ -180,7 +180,6 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
 		}
 		e->attr = git_attr_internal(cp, len);
 	}
-	(*num_attr)++;
 	return ep + strspn(ep, blank);
 }

@@ -226,9 +225,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 		cp = name + namelen;
 		cp = cp + strspn(cp, blank);
 		while (*cp) {
-			cp = parse_attr(src, lineno, cp, &num_attr, res);
+			cp = parse_attr(src, lineno, cp, num_attr, res);
 			if (!cp)
 				return NULL;
+			num_attr++;
 		}
 		if (pass)
 			break;

-- 
1.7.6.8.gd2879

[PATCH 4/7] Change parse_attr() to take a pointer to struct attr_state

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:51:48

parse_attr() only needs access to the attr_state to which it should
store its results, not to the whole match_attr structure.  This change
also removes the need for it to know num_attr.  Change its signature
accordingly and add a comment.

Signed-off-by: Michael Haggerty <redacted>
---
 attr.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/attr.c b/attr.c
index cac550d..f23f62a 100644
--- a/attr.c
+++ b/attr.c

@@ -139,8 +139,15 @@ struct match_attr {
 
 static const char blank[] = " \t\r\n";
 
+/*
+ * Parse a whitespace-delimited attribute state (i.e., "attr",
+ * "-attr", "!attr", or "attr=value") from the string starting at src.
+ * If e is not NULL, write the results to *e.  Return a pointer to the
+ * remainder of the string (with leading whitespace removed), or NULL
+ * if there was an error.
+ */
 static const char *parse_attr(const char *src, int lineno, const char *cp,
-			      int num_attr, struct match_attr *res)
+			      struct attr_state *e)
 {
 	const char *ep, *equals;
 	int len;

@@ -153,7 +160,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
 		len = equals - cp;
 	else
 		len = ep - cp;
-	if (!res) {
+	if (!e) {
 		if (*cp == '-' || *cp == '!') {
 			cp++;
 			len--;

@@ -165,9 +172,6 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
 			return NULL;
 		}
 	} else {
-		struct attr_state *e;
-
-		e = &(res->state[num_attr]);
 		if (*cp == '-' || *cp == '!') {
 			e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
 			cp++;

@@ -225,7 +229,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 		cp = name + namelen;
 		cp = cp + strspn(cp, blank);
 		while (*cp) {
-			cp = parse_attr(src, lineno, cp, num_attr, res);
+			cp = parse_attr(src, lineno, cp,
+				pass ? &(res->state[num_attr]) : NULL);
 			if (!cp)
 				return NULL;
 			num_attr++;

-- 
1.7.6.8.gd2879

[PATCH 5/7] Determine the start of the states outside of the pass loop

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:51:48

Signed-off-by: Michael Haggerty <redacted>
---
 attr.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/attr.c b/attr.c
index f23f62a..a7d1aa9 100644
--- a/attr.c
+++ b/attr.c

@@ -192,7 +192,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 {
 	int namelen;
 	int num_attr;
-	const char *cp, *name;
+	const char *cp, *name, *states;
 	struct match_attr *res = NULL;
 	int pass;
 	int is_macro;

@@ -223,11 +223,13 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 	else
 		is_macro = 0;
 
+	states = name + namelen;
+	states += strspn(states, blank);
+
 	for (pass = 0; pass < 2; pass++) {
 		/* pass 0 counts and allocates, pass 1 fills */
 		num_attr = 0;
-		cp = name + namelen;
-		cp = cp + strspn(cp, blank);
+		cp = states;
 		while (*cp) {
 			cp = parse_attr(src, lineno, cp,
 				pass ? &(res->state[num_attr]) : NULL);

-- 
1.7.6.8.gd2879

[PATCH 6/7] Change while loop into for loop

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:51:48

Signed-off-by: Michael Haggerty <redacted>
---
 attr.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/attr.c b/attr.c
index a7d1aa9..b56542e 100644
--- a/attr.c
+++ b/attr.c

@@ -228,14 +228,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 
 	for (pass = 0; pass < 2; pass++) {
 		/* pass 0 counts and allocates, pass 1 fills */
-		num_attr = 0;
-		cp = states;
-		while (*cp) {
+		for (cp = states, num_attr = 0; *cp; num_attr++) {
 			cp = parse_attr(src, lineno, cp,
 				pass ? &(res->state[num_attr]) : NULL);
 			if (!cp)
 				return NULL;
-			num_attr++;
 		}
 		if (pass)
 			break;

-- 
1.7.6.8.gd2879

[PATCH 7/7] Unroll the loop over passes

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:51:48

The passes no longer share much code, and the unrolled code is easier
to understand.

Use a new index variable instead of num_attr for the second loop, as
we are no longer counting attributes but rather indexing through them.

Signed-off-by: Michael Haggerty <redacted>
---
 attr.c |   51 ++++++++++++++++++++++++++-------------------------
 1 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/attr.c b/attr.c
index b56542e..6abaaec 100644
--- a/attr.c
+++ b/attr.c

@@ -191,10 +191,9 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 					  int lineno, int macro_ok)
 {
 	int namelen;
-	int num_attr;
+	int num_attr, i;
 	const char *cp, *name, *states;
 	struct match_attr *res = NULL;
-	int pass;
 	int is_macro;
 
 	cp = line + strspn(line, blank);

@@ -226,30 +225,32 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 	states = name + namelen;
 	states += strspn(states, blank);
 
-	for (pass = 0; pass < 2; pass++) {
-		/* pass 0 counts and allocates, pass 1 fills */
-		for (cp = states, num_attr = 0; *cp; num_attr++) {
-			cp = parse_attr(src, lineno, cp,
-				pass ? &(res->state[num_attr]) : NULL);
-			if (!cp)
-				return NULL;
-		}
-		if (pass)
-			break;
-		res = xcalloc(1,
-			      sizeof(*res) +
-			      sizeof(struct attr_state) * num_attr +
-			      (is_macro ? 0 : namelen + 1));
-		if (is_macro)
-			res->u.attr = git_attr_internal(name, namelen);
-		else {
-			res->u.pattern = (char *)&(res->state[num_attr]);
-			memcpy(res->u.pattern, name, namelen);
-			res->u.pattern[namelen] = 0;
-		}
-		res->is_macro = is_macro;
-		res->num_attr = num_attr;
+	/* First pass to count the attr_states */
+	for (cp = states, num_attr = 0; *cp; num_attr++) {
+		cp = parse_attr(src, lineno, cp, NULL);
+		if (!cp)
+			return NULL;
 	}
+
+	res = xcalloc(1,
+		      sizeof(*res) +
+		      sizeof(struct attr_state) * num_attr +
+		      (is_macro ? 0 : namelen + 1));
+	if (is_macro)
+		res->u.attr = git_attr_internal(name, namelen);
+	else {
+		res->u.pattern = (char *)&(res->state[num_attr]);
+		memcpy(res->u.pattern, name, namelen);
+		res->u.pattern[namelen] = 0;
+	}
+	res->is_macro = is_macro;
+	res->num_attr = num_attr;
+
+	/* Second pass to fill the attr_states */
+	for (cp = states, i = 0; *cp; i++) {
+		cp = parse_attr(src, lineno, cp, &(res->state[i]));
+	}
+
 	return res;
 }

-- 
1.7.6.8.gd2879

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help