{"thread":{"id":"28084","subject":"[PATCH 0/7] Comments and refactoring in attr.c","startedAt":"2011-08-12T21:43:03Z","lastAt":"2011-08-12T21:43:10Z","messageCount":8,"participants":["Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"173419","messageId":"1313185390-19724-1-git-send-email-mhagger@alum.mit.edu","threadId":"28084","inReplyTo":null,"subject":"[PATCH 0/7] Comments and refactoring in attr.c","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T21:43:03Z","receivedAt":"2011-08-12T21:43:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This patch series adds comments for some non-obvious things in attr.c,\nsimplifies the interface of parse_attr(), and unrolls the loop over\npasses in parse_attr_line() (which IMO makes the code easier to\nunderstand).  It does not change the observable behavior in any way.\n\nThe patch series is against master, but can be rebased to next without\nconflicts.\n\nMichael Haggerty (7):\n  Add a file comment\n  Document struct match_attr\n  Increment num_attr in parse_attr_line(), not parse_attr()\n  Change parse_attr() to take a pointer to struct attr_state\n  Determine the start of the states outside of the pass loop\n  Change while loop into for loop\n  Unroll the loop over passes\n\n attr.c |  113 +++++++++++++++++++++++++++++++++++----------------------------\n 1 files changed, 63 insertions(+), 50 deletions(-)\n\n-- \n1.7.6.8.gd2879\n"},{"id":"173420","messageId":"1313185390-19724-2-git-send-email-mhagger@alum.mit.edu","threadId":"28084","inReplyTo":"1313185390-19724-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/7] Add a file comment","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T21:43:04Z","receivedAt":"2011-08-12T21:43:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Consolidate here a few general comments plus links to other\ndocumentation.  Delete a comment with an out-of-date description of\nthe .gitattributes file format.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n attr.c |   26 ++++++++++----------------\n 1 files changed, 10 insertions(+), 16 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex f6b3f7e..6bc7ae9 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1,3 +1,12 @@\n+/*\n+ * Handle git attributes.  See gitattributes(5) for a description of\n+ * the file syntax, and Documentation/technical/api-gitattributes.txt\n+ * for a description of the API.\n+ *\n+ * One basic design decision here is that we are not going to support\n+ * an insanely large number of attributes.\n+ */\n+\n #define NO_THE_INDEX_COMPATIBILITY_MACROS\n #include \"cache.h\"\n #include \"exec_cmd.h\"\n@@ -13,12 +22,7 @@ static const char git_attr__unknown[] = \"(builtin)unknown\";\n \n static const char *attributes_file;\n \n-/*\n- * The basic design decision here is that we are not going to have\n- * insanely large number of attributes.\n- *\n- * This is a randomly chosen prime.\n- */\n+/* This is a randomly chosen prime. */\n #define HASHSIZE 257\n \n #ifndef DEBUG_ATTR\n@@ -103,16 +107,6 @@ struct git_attr *git_attr(const char *name)\n \treturn git_attr_internal(name, strlen(name));\n }\n \n-/*\n- * .gitattributes file is one line per record, each of which is\n- *\n- * (1) glob pattern.\n- * (2) whitespace\n- * (3) whitespace separated list of attribute names, each of which\n- *     could be prefixed with '-' to mean \"set to false\", '!' to mean\n- *     \"unset\".\n- */\n-\n /* What does a matched pattern decide? */\n struct attr_state {\n \tstruct git_attr *attr;\n-- \n1.7.6.8.gd2879\n"},{"id":"173421","messageId":"1313185390-19724-3-git-send-email-mhagger@alum.mit.edu","threadId":"28084","inReplyTo":"1313185390-19724-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/7] Document struct match_attr","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T21:43:05Z","receivedAt":"2011-08-12T21:43:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n attr.c |   14 ++++++++++++++\n 1 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 6bc7ae9..c33e413 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -113,6 +113,20 @@ struct attr_state {\n \tconst char *setto;\n };\n \n+/*\n+ * One rule, as from a .gitattributes file.\n+ *\n+ * If is_macro is true, then u.attr is a pointer to the git_attr being\n+ * defined.\n+ *\n+ * If is_macro is false, then u.pattern points at the filename pattern\n+ * to which the rule applies.  (The memory pointed to is part of the\n+ * memory block allocated for the match_attr instance.)\n+ *\n+ * In either case, num_attr is the number of attributes affected by\n+ * this rule, and state is an array listing them.  The attributes are\n+ * listed as they appear in the file (macros unexpanded).\n+ */\n struct match_attr {\n \tunion {\n \t\tchar *pattern;\n-- \n1.7.6.8.gd2879\n"},{"id":"173422","messageId":"1313185390-19724-4-git-send-email-mhagger@alum.mit.edu","threadId":"28084","inReplyTo":"1313185390-19724-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/7] Increment num_attr in parse_attr_line(), not parse_attr()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T21:43:06Z","receivedAt":"2011-08-12T21:43:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"num_attr is incremented iff parse_attr() returns non-NULL.  So do the\ncounting in parse_attr_line() instead of within parse_attr().  This\nallows an integer rather than a pointer to an integer to be passed to\nparse_attr().\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n attr.c |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex c33e413..cac550d 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -140,7 +140,7 @@ struct match_attr {\n static const char blank[] = \" \\t\\r\\n\";\n \n static const char *parse_attr(const char *src, int lineno, const char *cp,\n-\t\t\t      int *num_attr, struct match_attr *res)\n+\t\t\t      int num_attr, struct match_attr *res)\n {\n \tconst char *ep, *equals;\n \tint len;\n@@ -167,7 +167,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n \t} else {\n \t\tstruct attr_state *e;\n \n-\t\te = &(res->state[*num_attr]);\n+\t\te = &(res->state[num_attr]);\n \t\tif (*cp == '-' || *cp == '!') {\n \t\t\te->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;\n \t\t\tcp++;\n@@ -180,7 +180,6 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n \t\t}\n \t\te->attr = git_attr_internal(cp, len);\n \t}\n-\t(*num_attr)++;\n \treturn ep + strspn(ep, blank);\n }\n \n@@ -226,9 +225,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\tcp = name + namelen;\n \t\tcp = cp + strspn(cp, blank);\n \t\twhile (*cp) {\n-\t\t\tcp = parse_attr(src, lineno, cp, &num_attr, res);\n+\t\t\tcp = parse_attr(src, lineno, cp, num_attr, res);\n \t\t\tif (!cp)\n \t\t\t\treturn NULL;\n+\t\t\tnum_attr++;\n \t\t}\n \t\tif (pass)\n \t\t\tbreak;\n-- \n1.7.6.8.gd2879\n"},{"id":"173423","messageId":"1313185390-19724-5-git-send-email-mhagger@alum.mit.edu","threadId":"28084","inReplyTo":"1313185390-19724-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/7] Change parse_attr() to take a pointer to struct attr_state","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T21:43:07Z","receivedAt":"2011-08-12T21:43:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"parse_attr() only needs access to the attr_state to which it should\nstore its results, not to the whole match_attr structure.  This change\nalso removes the need for it to know num_attr.  Change its signature\naccordingly and add a comment.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n attr.c |   17 +++++++++++------\n 1 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex cac550d..f23f62a 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -139,8 +139,15 @@ struct match_attr {\n \n static const char blank[] = \" \\t\\r\\n\";\n \n+/*\n+ * Parse a whitespace-delimited attribute state (i.e., \"attr\",\n+ * \"-attr\", \"!attr\", or \"attr=value\") from the string starting at src.\n+ * If e is not NULL, write the results to *e.  Return a pointer to the\n+ * remainder of the string (with leading whitespace removed), or NULL\n+ * if there was an error.\n+ */\n static const char *parse_attr(const char *src, int lineno, const char *cp,\n-\t\t\t      int num_attr, struct match_attr *res)\n+\t\t\t      struct attr_state *e)\n {\n \tconst char *ep, *equals;\n \tint len;\n@@ -153,7 +160,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n \t\tlen = equals - cp;\n \telse\n \t\tlen = ep - cp;\n-\tif (!res) {\n+\tif (!e) {\n \t\tif (*cp == '-' || *cp == '!') {\n \t\t\tcp++;\n \t\t\tlen--;\n@@ -165,9 +172,6 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n \t\t\treturn NULL;\n \t\t}\n \t} else {\n-\t\tstruct attr_state *e;\n-\n-\t\te = &(res->state[num_attr]);\n \t\tif (*cp == '-' || *cp == '!') {\n \t\t\te->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;\n \t\t\tcp++;\n@@ -225,7 +229,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\tcp = name + namelen;\n \t\tcp = cp + strspn(cp, blank);\n \t\twhile (*cp) {\n-\t\t\tcp = parse_attr(src, lineno, cp, num_attr, res);\n+\t\t\tcp = parse_attr(src, lineno, cp,\n+\t\t\t\tpass ? &(res->state[num_attr]) : NULL);\n \t\t\tif (!cp)\n \t\t\t\treturn NULL;\n \t\t\tnum_attr++;\n-- \n1.7.6.8.gd2879\n"},{"id":"173424","messageId":"1313185390-19724-6-git-send-email-mhagger@alum.mit.edu","threadId":"28084","inReplyTo":"1313185390-19724-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/7] Determine the start of the states outside of the pass loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T21:43:08Z","receivedAt":"2011-08-12T21:43:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n attr.c |    8 +++++---\n 1 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex f23f62a..a7d1aa9 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -192,7 +192,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n {\n \tint namelen;\n \tint num_attr;\n-\tconst char *cp, *name;\n+\tconst char *cp, *name, *states;\n \tstruct match_attr *res = NULL;\n \tint pass;\n \tint is_macro;\n@@ -223,11 +223,13 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \telse\n \t\tis_macro = 0;\n \n+\tstates = name + namelen;\n+\tstates += strspn(states, blank);\n+\n \tfor (pass = 0; pass < 2; pass++) {\n \t\t/* pass 0 counts and allocates, pass 1 fills */\n \t\tnum_attr = 0;\n-\t\tcp = name + namelen;\n-\t\tcp = cp + strspn(cp, blank);\n+\t\tcp = states;\n \t\twhile (*cp) {\n \t\t\tcp = parse_attr(src, lineno, cp,\n \t\t\t\tpass ? &(res->state[num_attr]) : NULL);\n-- \n1.7.6.8.gd2879\n"},{"id":"173425","messageId":"1313185390-19724-7-git-send-email-mhagger@alum.mit.edu","threadId":"28084","inReplyTo":"1313185390-19724-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 6/7] Change while loop into for loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T21:43:09Z","receivedAt":"2011-08-12T21:43:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n attr.c |    5 +----\n 1 files changed, 1 insertions(+), 4 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex a7d1aa9..b56542e 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -228,14 +228,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \n \tfor (pass = 0; pass < 2; pass++) {\n \t\t/* pass 0 counts and allocates, pass 1 fills */\n-\t\tnum_attr = 0;\n-\t\tcp = states;\n-\t\twhile (*cp) {\n+\t\tfor (cp = states, num_attr = 0; *cp; num_attr++) {\n \t\t\tcp = parse_attr(src, lineno, cp,\n \t\t\t\tpass ? &(res->state[num_attr]) : NULL);\n \t\t\tif (!cp)\n \t\t\t\treturn NULL;\n-\t\t\tnum_attr++;\n \t\t}\n \t\tif (pass)\n \t\t\tbreak;\n-- \n1.7.6.8.gd2879\n"},{"id":"173426","messageId":"1313185390-19724-8-git-send-email-mhagger@alum.mit.edu","threadId":"28084","inReplyTo":"1313185390-19724-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 7/7] Unroll the loop over passes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T21:43:10Z","receivedAt":"2011-08-12T21:43:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The passes no longer share much code, and the unrolled code is easier\nto understand.\n\nUse a new index variable instead of num_attr for the second loop, as\nwe are no longer counting attributes but rather indexing through them.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n attr.c |   51 ++++++++++++++++++++++++++-------------------------\n 1 files changed, 26 insertions(+), 25 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex b56542e..6abaaec 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -191,10 +191,9 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\t\t\t\t  int lineno, int macro_ok)\n {\n \tint namelen;\n-\tint num_attr;\n+\tint num_attr, i;\n \tconst char *cp, *name, *states;\n \tstruct match_attr *res = NULL;\n-\tint pass;\n \tint is_macro;\n \n \tcp = line + strspn(line, blank);\n@@ -226,30 +225,32 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \tstates = name + namelen;\n \tstates += strspn(states, blank);\n \n-\tfor (pass = 0; pass < 2; pass++) {\n-\t\t/* pass 0 counts and allocates, pass 1 fills */\n-\t\tfor (cp = states, num_attr = 0; *cp; num_attr++) {\n-\t\t\tcp = parse_attr(src, lineno, cp,\n-\t\t\t\tpass ? &(res->state[num_attr]) : NULL);\n-\t\t\tif (!cp)\n-\t\t\t\treturn NULL;\n-\t\t}\n-\t\tif (pass)\n-\t\t\tbreak;\n-\t\tres = xcalloc(1,\n-\t\t\t      sizeof(*res) +\n-\t\t\t      sizeof(struct attr_state) * num_attr +\n-\t\t\t      (is_macro ? 0 : namelen + 1));\n-\t\tif (is_macro)\n-\t\t\tres->u.attr = git_attr_internal(name, namelen);\n-\t\telse {\n-\t\t\tres->u.pattern = (char *)&(res->state[num_attr]);\n-\t\t\tmemcpy(res->u.pattern, name, namelen);\n-\t\t\tres->u.pattern[namelen] = 0;\n-\t\t}\n-\t\tres->is_macro = is_macro;\n-\t\tres->num_attr = num_attr;\n+\t/* First pass to count the attr_states */\n+\tfor (cp = states, num_attr = 0; *cp; num_attr++) {\n+\t\tcp = parse_attr(src, lineno, cp, NULL);\n+\t\tif (!cp)\n+\t\t\treturn NULL;\n \t}\n+\n+\tres = xcalloc(1,\n+\t\t      sizeof(*res) +\n+\t\t      sizeof(struct attr_state) * num_attr +\n+\t\t      (is_macro ? 0 : namelen + 1));\n+\tif (is_macro)\n+\t\tres->u.attr = git_attr_internal(name, namelen);\n+\telse {\n+\t\tres->u.pattern = (char *)&(res->state[num_attr]);\n+\t\tmemcpy(res->u.pattern, name, namelen);\n+\t\tres->u.pattern[namelen] = 0;\n+\t}\n+\tres->is_macro = is_macro;\n+\tres->num_attr = num_attr;\n+\n+\t/* Second pass to fill the attr_states */\n+\tfor (cp = states, i = 0; *cp; i++) {\n+\t\tcp = parse_attr(src, lineno, cp, &(res->state[i]));\n+\t}\n+\n \treturn res;\n }\n \n-- \n1.7.6.8.gd2879\n"}]}