[PATCH v2 00/12] revamping git_check_attr() API

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 00/12] revamping git_check_attr() API

Junio C Hamano
A common pattern to check N attributes for many paths is to

 (1) prepare an array A of N git_attr_check_elem items;
 (2) call git_attr() to intern the N attribute names and fill A;
 (3) repeatedly call git_check_attrs() for path with N and A;

A look-up for these N attributes for a single path P scans the
entire attr_stack, starting from the .git/info/attributes file and
then .gitattributes file in the directory the path P is in, going
upwards to find .gitattributes file found in parent directories.

An earlier commit 06a604e6 (attr: avoid heavy work when we know the
specified attr is not defined, 2014-12-28) tried to optimize out
this scanning for one trivial special case: when the attribute being
sought is known not to exist, we do not have to scan for it.  While
this may be a cheap and effective heuristic, it would not work well
when N is (much) more than 1.

What we would want is a more customized way to skip irrelevant
entries in the attribute stack, and the definition of irrelevance
is tied to the set of attributes passed to git_check_attrs() call,
i.e. the set of attributes being sought.  The data necessary for
this optimization needs to live alongside the set of attributes, but
a simple array of git_attr_check_elem simply does not have any place
for that.

Introduce "struct git_attr_check" that contains N, the number of
attributes being sought, and A, the array that holds N
git_attr_check_elem items, and a function git_check_attr() that
takes a path P and this structure as its parameters.  This structure
can later be extended to hold extra data necessary for optimization.

    Side Note: While in this series, I am not interested in
    specifying the exact optimization, it would help readers to give
    what is envisioned.  Even when the caller is interested in only
    a few attributes (say, "diff", "text" and "group-doc"), we'd walk
    the attribute entries in various .gitattributes files, perform
    the pattern match and collect attributes for the path.  An
    earlier commit 06a604e6 (attr: avoid heavy work when we know the
    specified attr is not defined, 2014-12-28) tried to optimize out
    this scanning for one trivial special case: where an asked-for
    attribute does not appear anywhere in these files, we know we do
    not have to scan at all.

    We can do much better.  We can scan these attribute entries that
    came from .gitattributes files once and mark the ones that affect
    one or more of the attributes we know the caller is interested
    in.  Then we only go through the marked entries.  E.g. if the
    caller is interested in "diff", "text" and "group-doc"
    attributes, we can mark entries that talks about any of "diff",
    "text", "group-doc" and "binary" attributes.  The last one is
    because it is an attribute macro that expands to affect "diff"
    and "text" (these attribute are unset by having "binary").

    The way git_attr_check() API is structured expects that the
    caller expresses the set of attributes it is interested in
    upfront, and reuses to ask the same question about many paths,
    and it is already optimized for the case where it does so in
    in-order recursive descent of a tree hierarchy by having an
    attr_stack that reads, pre-parses and caches contents of the
    .gitattributes files found in each directory hierarchy.  The
    cached optimization data that sits in "struct git_attr_check"
    would be effective for this expected access pattern.

    The optimization data cannot be file-scope static to attr.c,
    even though that might be easier to implement, in anticipation
    of having more than one "struct git_attr_check" working
    alternatingly, e.g. having more than one pathspec with
    ":(attr=X)" pathspec magic, i.e.

        git cmd ":(attr=X)dir/" ":(attr=Y)dir/ectory/"

    Each pathspec element is expected to keep its own "struct
    git_attr_check" in the custom data to support pathspec magic
    to match with attributes, and the optimization data kept there
    would survive repeated calls to git_check_attr() for the same
    path with different set of attributes.


The patches in the earliest part of the series have been sent to the
list already; there is no substantial change (I think I made a
typofix in the commit log message found by Eric).

  commit.c: use strchrnul() to scan for one line
  attr.c: use strchrnul() to scan for one line
  attr.c: update a stale comment on "struct match_attr"
  attr.c: explain the lack of attr-name syntax check in parse_attr()
  attr.c: complete a sentence in a comment
  attr.c: mark where #if DEBUG ends more clearly
  attr.c: simplify macroexpand_one()

Step 8 is to make sure I can catch all the existing callers and
update the API incrementally.

  attr: rename function and struct related to checking attributes

Step 9 is the most important one.  By updating the API to allow the
callers hold piece of information more than just a simple array of
<attr, value> pair, it paves the way to add data structures that
will help optimizing the lookup.  As a demonstration of the new API,
it converts one caller.  The immediate effect to the callers is that
they can lose moderate amount of code, but that is not the point of
this change.

  attr: (re)introduce git_check_attr() and struct git_attr_check

The two patches that follow converts remaining callers to the enw
API.

  attr: convert git_all_attrs() to use "struct git_attr_check"
  attr: convert git_check_attrs() callers to use the new API

The last step retires the old one and updates the document.

  attr: retire git_check_attrs() API

 Documentation/technical/api-gitattributes.txt |  62 ++++++-----
 archive.c                                     |  24 ++---
 attr.c                                        | 143 +++++++++++++++++++-------
 attr.h                                        |  31 ++++--
 builtin/check-attr.c                          |  55 +++++-----
 builtin/pack-objects.c                        |  19 +---
 commit.c                                      |   3 +-
 convert.c                                     |  26 ++---
 ll-merge.c                                    |  33 +++---
 userdiff.c                                    |  19 ++--
 ws.c                                          |  19 ++--
 11 files changed, 236 insertions(+), 198 deletions(-)

--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 01/12] commit.c: use strchrnul() to scan for one line

Junio C Hamano
Signed-off-by: Junio C Hamano <[hidden email]>
---
 commit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 3f4f371..1f9ee8a 100644
--- a/commit.c
+++ b/commit.c
@@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
  p++;
  if (*p) {
  p += 2;
- for (eol = p; *eol && *eol != '\n'; eol++)
- ; /* do nothing */
+ eol = strchrnul(p, '\n');
  } else
  eol = p;
 
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 02/12] attr.c: use strchrnul() to scan for one line

Junio C Hamano
In reply to this post by Junio C Hamano
Signed-off-by: Junio C Hamano <[hidden email]>
---
 attr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/attr.c b/attr.c
index eec5d7d..45aec1b 100644
--- a/attr.c
+++ b/attr.c
@@ -402,8 +402,8 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
  for (sp = buf; *sp; ) {
  char *ep;
  int more;
- for (ep = sp; *ep && *ep != '\n'; ep++)
- ;
+
+ ep = strchrnul(sp, '\n');
  more = (*ep == '\n');
  *ep = '\0';
  handle_attr_line(res, sp, path, ++lineno, macro_ok);
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 03/12] attr.c: update a stale comment on "struct match_attr"

Junio C Hamano
In reply to this post by Junio C Hamano
When 82dce998 (attr: more matching optimizations from .gitignore,
2012-10-15) changed a pointer to a string "*pattern" into an
embedded "struct pattern" in struct match_attr, it forgot to update
the comment that describes the structure.

Signed-off-by: Junio C Hamano <[hidden email]>
---
 attr.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/attr.c b/attr.c
index 45aec1b..4ae7801 100644
--- a/attr.c
+++ b/attr.c
@@ -131,9 +131,8 @@ struct pattern {
  * 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.)
+ * If is_macro is false, then u.pat is the filename pattern to which the
+ * rule applies.
  *
  * In either case, num_attr is the number of attributes affected by
  * this rule, and state is an array listing them.  The attributes are
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 04/12] attr.c: explain the lack of attr-name syntax check in parse_attr()

Junio C Hamano
In reply to this post by Junio C Hamano
Signed-off-by: Junio C Hamano <[hidden email]>
---
 attr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/attr.c b/attr.c
index 4ae7801..05db667 100644
--- a/attr.c
+++ b/attr.c
@@ -183,6 +183,12 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
  return NULL;
  }
  } else {
+ /*
+ * As this function is always called twice, once with
+ * e == NULL in the first pass and then e != NULL in
+ * the second pass, no need for invalid_attr_name()
+ * check here.
+ */
  if (*cp == '-' || *cp == '!') {
  e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
  cp++;
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 05/12] attr.c: complete a sentence in a comment

Junio C Hamano
In reply to this post by Junio C Hamano
Signed-off-by: Junio C Hamano <[hidden email]>
---
 attr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/attr.c b/attr.c
index 05db667..a7f2c3f 100644
--- a/attr.c
+++ b/attr.c
@@ -300,7 +300,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
  * directory (again, reading the file from top to bottom) down to the
  * current directory, and then scan the list backwards to find the first match.
  * This is exactly the same as what is_excluded() does in dir.c to deal with
- * .gitignore
+ * .gitignore file and info/excludes file as a fallback.
  */
 
 static struct attr_stack {
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 06/12] attr.c: mark where #if DEBUG ends more clearly

Junio C Hamano
In reply to this post by Junio C Hamano
Signed-off-by: Junio C Hamano <[hidden email]>
---
 attr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/attr.c b/attr.c
index a7f2c3f..95416d3 100644
--- a/attr.c
+++ b/attr.c
@@ -469,7 +469,7 @@ static void debug_set(const char *what, const char *match, struct git_attr *attr
 #define debug_push(a) do { ; } while (0)
 #define debug_pop(a) do { ; } while (0)
 #define debug_set(a,b,c,d) do { ; } while (0)
-#endif
+#endif /* DEBUG_ATTR */
 
 static void drop_attr_stack(void)
 {
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 07/12] attr.c: simplify macroexpand_one()

Junio C Hamano
In reply to this post by Junio C Hamano
The double-loop wants to do an early return immediately when one
matching macro is found.  Eliminate the extra variable 'a' used for
that purpose and rewrite the "assign the found item to 'a' to make
it non-NULL and force the loop(s) to terminate" with a direct return
from there.

Signed-off-by: Junio C Hamano <[hidden email]>
---
 attr.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/attr.c b/attr.c
index 95416d3..7bfeef3 100644
--- a/attr.c
+++ b/attr.c
@@ -701,24 +701,21 @@ static int fill(const char *path, int pathlen, int basename_offset,
 static int macroexpand_one(int nr, int rem)
 {
  struct attr_stack *stk;
- struct match_attr *a = NULL;
  int i;
 
  if (check_all_attr[nr].value != ATTR__TRUE ||
     !check_all_attr[nr].attr->maybe_macro)
  return rem;
 
- for (stk = attr_stack; !a && stk; stk = stk->prev)
- for (i = stk->num_matches - 1; !a && 0 <= i; i--) {
+ for (stk = attr_stack; stk; stk = stk->prev) {
+ for (i = stk->num_matches - 1; 0 <= i; i--) {
  struct match_attr *ma = stk->attrs[i];
  if (!ma->is_macro)
  continue;
  if (ma->u.attr->attr_nr == nr)
- a = ma;
+ return fill_one("expand", ma, rem);
  }
-
- if (a)
- rem = fill_one("expand", a, rem);
+ }
 
  return rem;
 }
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 08/12] attr: rename function and struct related to checking attributes

Junio C Hamano
In reply to this post by Junio C Hamano
The traditional API to check attributes is to prepare an N-element
array of "struct git_attr_check" and pass N and the array to the
function "git_check_attr()" as arguments.

In preparation to revamp the API to pass a single structure, in
which these N elements are held, rename the type used for these
individual array elements to "struct git_attr_check_elem" and rename
the function to "git_check_attrs()".

Signed-off-by: Junio C Hamano <[hidden email]>
---
 archive.c              |  6 +++---
 attr.c                 | 12 ++++++------
 attr.h                 |  8 ++++----
 builtin/check-attr.c   | 19 ++++++++++---------
 builtin/pack-objects.c |  6 +++---
 convert.c              | 12 ++++++------
 ll-merge.c             | 10 +++++-----
 userdiff.c             |  4 ++--
 ws.c                   |  6 +++---
 9 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/archive.c b/archive.c
index 5d735ae..0f6acc5 100644
--- a/archive.c
+++ b/archive.c
@@ -87,7 +87,7 @@ void *sha1_file_to_archive(const struct archiver_args *args,
  return buffer;
 }
 
-static void setup_archive_check(struct git_attr_check *check)
+static void setup_archive_check(struct git_attr_check_elem *check)
 {
  static struct git_attr *attr_export_ignore;
  static struct git_attr *attr_export_subst;
@@ -123,7 +123,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
  struct archiver_context *c = context;
  struct archiver_args *args = c->args;
  write_archive_entry_fn_t write_entry = c->write_entry;
- struct git_attr_check check[2];
+ struct git_attr_check_elem check[2];
  const char *path_without_prefix;
  int err;
 
@@ -138,7 +138,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
  path_without_prefix = path.buf + args->baselen;
 
  setup_archive_check(check);
- if (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {
+ if (!git_check_attrs(path_without_prefix, ARRAY_SIZE(check), check)) {
  if (ATTR_TRUE(check[0].value))
  return 0;
  args->convert = ATTR_TRUE(check[1].value);
diff --git a/attr.c b/attr.c
index 7bfeef3..8aa346c 100644
--- a/attr.c
+++ b/attr.c
@@ -40,7 +40,7 @@ struct git_attr {
 static int attr_nr;
 static int cannot_trust_maybe_real;
 
-static struct git_attr_check *check_all_attr;
+static struct git_attr_check_elem *check_all_attr;
 static struct git_attr *(git_attr_hash[HASHSIZE]);
 
 char *git_attr_name(struct git_attr *attr)
@@ -661,7 +661,7 @@ static int macroexpand_one(int attr_nr, int rem);
 
 static int fill_one(const char *what, struct match_attr *a, int rem)
 {
- struct git_attr_check *check = check_all_attr;
+ struct git_attr_check_elem *check = check_all_attr;
  int i;
 
  for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
@@ -726,7 +726,7 @@ static int macroexpand_one(int nr, int rem)
  * collected. Otherwise all attributes are collected.
  */
 static void collect_some_attrs(const char *path, int num,
-       struct git_attr_check *check)
+       struct git_attr_check_elem *check)
 
 {
  struct attr_stack *stk;
@@ -754,7 +754,7 @@ static void collect_some_attrs(const char *path, int num,
  rem = 0;
  for (i = 0; i < num; i++) {
  if (!check[i].attr->maybe_real) {
- struct git_attr_check *c;
+ struct git_attr_check_elem *c;
  c = check_all_attr + check[i].attr->attr_nr;
  c->value = ATTR__UNSET;
  rem++;
@@ -769,7 +769,7 @@ static void collect_some_attrs(const char *path, int num,
  rem = fill(path, pathlen, basename_offset, stk, rem);
 }
 
-int git_check_attr(const char *path, int num, struct git_attr_check *check)
+int git_check_attrs(const char *path, int num, struct git_attr_check_elem *check)
 {
  int i;
 
@@ -785,7 +785,7 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)
  return 0;
 }
 
-int git_all_attrs(const char *path, int *num, struct git_attr_check **check)
+int git_all_attrs(const char *path, int *num, struct git_attr_check_elem **check)
 {
  int i, count, j;
 
diff --git a/attr.h b/attr.h
index 8b08d33..cab82ec 100644
--- a/attr.h
+++ b/attr.h
@@ -20,11 +20,11 @@ extern const char git_attr__false[];
 #define ATTR_UNSET(v) ((v) == NULL)
 
 /*
- * Send one or more git_attr_check to git_check_attr(), and
+ * Send one or more git_attr_check to git_check_attrs(), and
  * each 'value' member tells what its value is.
  * Unset one is returned as NULL.
  */
-struct git_attr_check {
+struct git_attr_check_elem {
  struct git_attr *attr;
  const char *value;
 };
@@ -36,7 +36,7 @@ struct git_attr_check {
  */
 char *git_attr_name(struct git_attr *);
 
-int git_check_attr(const char *path, int, struct git_attr_check *);
+int git_check_attrs(const char *path, int, struct git_attr_check_elem *);
 
 /*
  * Retrieve all attributes that apply to the specified path.  *num
@@ -45,7 +45,7 @@ int git_check_attr(const char *path, int, struct git_attr_check *);
  * objects describing the attributes and their values.  *check must be
  * free()ed by the caller.
  */
-int git_all_attrs(const char *path, int *num, struct git_attr_check **check);
+int git_all_attrs(const char *path, int *num, struct git_attr_check_elem **check);
 
 enum git_attr_direction {
  GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 53a5a18..97e3837 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,8 +24,8 @@ static const struct option check_attr_options[] = {
  OPT_END()
 };
 
-static void output_attr(int cnt, struct git_attr_check *check,
- const char *file)
+static void output_attr(int cnt, struct git_attr_check_elem *check,
+ const char *file)
 {
  int j;
  for (j = 0; j < cnt; j++) {
@@ -51,14 +51,15 @@ static void output_attr(int cnt, struct git_attr_check *check,
  }
 }
 
-static void check_attr(const char *prefix, int cnt,
- struct git_attr_check *check, const char *file)
+static void check_attr(const char *prefix,
+       int cnt, struct git_attr_check_elem *check,
+       const char *file)
 {
  char *full_path =
  prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
  if (check != NULL) {
- if (git_check_attr(full_path, cnt, check))
- die("git_check_attr died");
+ if (git_check_attrs(full_path, cnt, check))
+ die("git_check_attrs died");
  output_attr(cnt, check, file);
  } else {
  if (git_all_attrs(full_path, &cnt, &check))
@@ -69,8 +70,8 @@ static void check_attr(const char *prefix, int cnt,
  free(full_path);
 }
 
-static void check_attr_stdin_paths(const char *prefix, int cnt,
- struct git_attr_check *check)
+static void check_attr_stdin_paths(const char *prefix,
+   int cnt, struct git_attr_check_elem *check)
 {
  struct strbuf buf = STRBUF_INIT;
  struct strbuf unquoted = STRBUF_INIT;
@@ -99,7 +100,7 @@ static NORETURN void error_with_usage(const char *msg)
 
 int cmd_check_attr(int argc, const char **argv, const char *prefix)
 {
- struct git_attr_check *check;
+ struct git_attr_check_elem *check;
  int cnt, i, doubledash, filei;
 
  if (!is_bare_repository())
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a27de5b..167c301 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -886,7 +886,7 @@ static void write_pack_file(void)
  written, nr_result);
 }
 
-static void setup_delta_attr_check(struct git_attr_check *check)
+static void setup_delta_attr_check(struct git_attr_check_elem *check)
 {
  static struct git_attr *attr_delta;
 
@@ -898,10 +898,10 @@ static void setup_delta_attr_check(struct git_attr_check *check)
 
 static int no_try_delta(const char *path)
 {
- struct git_attr_check check[1];
+ struct git_attr_check_elem check[1];
 
  setup_delta_attr_check(check);
- if (git_check_attr(path, ARRAY_SIZE(check), check))
+ if (git_check_attrs(path, ARRAY_SIZE(check), check))
  return 0;
  if (ATTR_FALSE(check->value))
  return 1;
diff --git a/convert.c b/convert.c
index f524b8d..058da86 100644
--- a/convert.c
+++ b/convert.c
@@ -703,7 +703,7 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
  return 1;
 }
 
-static enum crlf_action git_path_check_crlf(struct git_attr_check *check)
+static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)
 {
  const char *value = check->value;
 
@@ -720,7 +720,7 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check *check)
  return CRLF_UNDEFINED;
 }
 
-static enum eol git_path_check_eol(struct git_attr_check *check)
+static enum eol git_path_check_eol(struct git_attr_check_elem *check)
 {
  const char *value = check->value;
 
@@ -733,7 +733,7 @@ static enum eol git_path_check_eol(struct git_attr_check *check)
  return EOL_UNSET;
 }
 
-static struct convert_driver *git_path_check_convert(struct git_attr_check *check)
+static struct convert_driver *git_path_check_convert(struct git_attr_check_elem *check)
 {
  const char *value = check->value;
  struct convert_driver *drv;
@@ -746,7 +746,7 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check *chec
  return NULL;
 }
 
-static int git_path_check_ident(struct git_attr_check *check)
+static int git_path_check_ident(struct git_attr_check_elem *check)
 {
  const char *value = check->value;
 
@@ -768,7 +768,7 @@ static const char *conv_attr_name[] = {
 static void convert_attrs(struct conv_attrs *ca, const char *path)
 {
  int i;
- static struct git_attr_check ccheck[NUM_CONV_ATTRS];
+ static struct git_attr_check_elem ccheck[NUM_CONV_ATTRS];
 
  if (!ccheck[0].attr) {
  for (i = 0; i < NUM_CONV_ATTRS; i++)
@@ -777,7 +777,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
  git_config(read_convert_config, NULL);
  }
 
- if (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {
+ if (!git_check_attrs(path, NUM_CONV_ATTRS, ccheck)) {
  ca->crlf_action = git_path_check_crlf(ccheck + 4);
  if (ca->crlf_action == CRLF_UNDEFINED)
  ca->crlf_action = git_path_check_crlf(ccheck + 0);
diff --git a/ll-merge.c b/ll-merge.c
index ff4a43a..772b14e 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -334,13 +334,13 @@ static const struct ll_merge_driver *find_ll_merge_driver(const char *merge_attr
  return &ll_merge_drv[LL_TEXT_MERGE];
 }
 
-static int git_path_check_merge(const char *path, struct git_attr_check check[2])
+static int git_path_check_merge(const char *path, struct git_attr_check_elem check[2])
 {
  if (!check[0].attr) {
  check[0].attr = git_attr("merge");
  check[1].attr = git_attr("conflict-marker-size");
  }
- return git_check_attr(path, 2, check);
+ return git_check_attrs(path, 2, check);
 }
 
 static void normalize_file(mmfile_t *mm, const char *path)
@@ -360,7 +360,7 @@ int ll_merge(mmbuffer_t *result_buf,
      mmfile_t *theirs, const char *their_label,
      const struct ll_merge_options *opts)
 {
- static struct git_attr_check check[2];
+ static struct git_attr_check_elem check[2];
  static const struct ll_merge_options default_opts;
  const char *ll_driver_name = NULL;
  int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
@@ -392,12 +392,12 @@ int ll_merge(mmbuffer_t *result_buf,
 
 int ll_merge_marker_size(const char *path)
 {
- static struct git_attr_check check;
+ static struct git_attr_check_elem check;
  int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 
  if (!check.attr)
  check.attr = git_attr("conflict-marker-size");
- if (!git_check_attr(path, 1, &check) && check.value) {
+ if (!git_check_attrs(path, 1, &check) && check.value) {
  marker_size = atoi(check.value);
  if (marker_size <= 0)
  marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
diff --git a/userdiff.c b/userdiff.c
index 6bf2505..3560024 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -251,7 +251,7 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {
 struct userdiff_driver *userdiff_find_by_path(const char *path)
 {
  static struct git_attr *attr;
- struct git_attr_check check;
+ struct git_attr_check_elem check;
 
  if (!attr)
  attr = git_attr("diff");
@@ -259,7 +259,7 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)
 
  if (!path)
  return NULL;
- if (git_check_attr(path, 1, &check))
+ if (git_check_attrs(path, 1, &check))
  return NULL;
 
  if (ATTR_TRUE(check.value))
diff --git a/ws.c b/ws.c
index ea4b2b1..7350905 100644
--- a/ws.c
+++ b/ws.c
@@ -71,7 +71,7 @@ unsigned parse_whitespace_rule(const char *string)
  return rule;
 }
 
-static void setup_whitespace_attr_check(struct git_attr_check *check)
+static void setup_whitespace_attr_check(struct git_attr_check_elem *check)
 {
  static struct git_attr *attr_whitespace;
 
@@ -82,10 +82,10 @@ static void setup_whitespace_attr_check(struct git_attr_check *check)
 
 unsigned whitespace_rule(const char *pathname)
 {
- struct git_attr_check attr_whitespace_rule;
+ struct git_attr_check_elem attr_whitespace_rule;
 
  setup_whitespace_attr_check(&attr_whitespace_rule);
- if (!git_check_attr(pathname, 1, &attr_whitespace_rule)) {
+ if (!git_check_attrs(pathname, 1, &attr_whitespace_rule)) {
  const char *value;
 
  value = attr_whitespace_rule.value;
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 09/12] attr: (re)introduce git_check_attr() and struct git_attr_check

Junio C Hamano
In reply to this post by Junio C Hamano
A common pattern to check N attributes for many paths is to

 (1) prepare an array A of N git_attr_check_elem items;
 (2) call git_attr() to intern the N attribute names and fill A;
 (3) repeatedly call git_check_attrs() for path with N and A;

A look-up for these N attributes for a single path P scans the
entire attr_stack, starting from the .git/info/attributes file and
then .gitattributes file in the directory the path P is in, going
upwards to find .gitattributes file found in parent directories.

An earlier commit 06a604e6 (attr: avoid heavy work when we know the
specified attr is not defined, 2014-12-28) tried to optimize out
this scanning for one trivial special case: when the attribute being
sought is known not to exist, we do not have to scan for it.  While
this may be a cheap and effective heuristic, it would not work well
when N is (much) more than 1.

What we would want is a more customized way to skip irrelevant
entries in the attribute stack, and the definition of irrelevance
is tied to the set of attributes passed to git_check_attrs() call,
i.e. the set of attributes being sought.  The data necessary for
this optimization needs to live alongside the set of attributes, but
a simple array of git_attr_check_elem simply does not have any place
for that.

Introduce "struct git_attr_check" that contains N, the number of
attributes being sought, and A, the array that holds N
git_attr_check_elem items, and a function git_check_attr() that
takes a path P and this structure as its parameters.  This structure
can later be extended to hold extra data necessary for optimization.

Also, to make it easier to write the first two steps in common
cases, introduce git_attr_check_initl() helper function, which takes
a NULL-terminated list of attribute names and initialize this
structure.

As an illustration of this new API, convert archive.c that asks for
export-subst and export-ignore attributes for each paths.

Signed-off-by: Junio C Hamano <[hidden email]>
---
 archive.c | 24 ++++++------------------
 attr.c    | 34 ++++++++++++++++++++++++++++++++++
 attr.h    |  9 +++++++++
 3 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/archive.c b/archive.c
index 0f6acc5..7779af1 100644
--- a/archive.c
+++ b/archive.c
@@ -87,19 +87,6 @@ void *sha1_file_to_archive(const struct archiver_args *args,
  return buffer;
 }
 
-static void setup_archive_check(struct git_attr_check_elem *check)
-{
- static struct git_attr *attr_export_ignore;
- static struct git_attr *attr_export_subst;
-
- if (!attr_export_ignore) {
- attr_export_ignore = git_attr("export-ignore");
- attr_export_subst = git_attr("export-subst");
- }
- check[0].attr = attr_export_ignore;
- check[1].attr = attr_export_subst;
-}
-
 struct directory {
  struct directory *up;
  struct object_id oid;
@@ -123,7 +110,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
  struct archiver_context *c = context;
  struct archiver_args *args = c->args;
  write_archive_entry_fn_t write_entry = c->write_entry;
- struct git_attr_check_elem check[2];
+ static struct git_attr_check *check;
  const char *path_without_prefix;
  int err;
 
@@ -137,11 +124,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
  strbuf_addch(&path, '/');
  path_without_prefix = path.buf + args->baselen;
 
- setup_archive_check(check);
- if (!git_check_attrs(path_without_prefix, ARRAY_SIZE(check), check)) {
- if (ATTR_TRUE(check[0].value))
+ if (!check)
+ check = git_attr_check_initl("export-ignore", "export-subst", NULL);
+ if (!git_check_attr(path_without_prefix, check)) {
+ if (ATTR_TRUE(check->check[0].value))
  return 0;
- args->convert = ATTR_TRUE(check[1].value);
+ args->convert = ATTR_TRUE(check->check[1].value);
  }
 
  if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
diff --git a/attr.c b/attr.c
index 8aa346c..285fc58 100644
--- a/attr.c
+++ b/attr.c
@@ -825,3 +825,37 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
  drop_attr_stack();
  use_index = istate;
 }
+
+int git_check_attr(const char *path, struct git_attr_check *check)
+{
+ return git_check_attrs(path, check->check_nr, check->check);
+}
+
+struct git_attr_check *git_attr_check_initl(const char *one, ...)
+{
+ struct git_attr_check *check;
+ int cnt;
+ va_list params;
+ const char *param;
+
+ va_start(params, one);
+ for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
+ ;
+ va_end(params);
+ check = xcalloc(1,
+ sizeof(*check) + cnt * sizeof(*(check->check)));
+ check->check_nr = cnt;
+ check->check = (struct git_attr_check_elem *)(check + 1);
+
+ check->check[0].attr = git_attr(one);
+ va_start(params, one);
+ for (cnt = 1; cnt < check->check_nr; cnt++) {
+ param = va_arg(params, const char *);
+ if (!param)
+ die("BUG: counted %d != ended at %d",
+    check->check_nr, cnt);
+ check->check[cnt].attr = git_attr(param);
+ }
+ va_end(params);
+ return check;
+}
diff --git a/attr.h b/attr.h
index cab82ec..3ed89e5 100644
--- a/attr.h
+++ b/attr.h
@@ -29,6 +29,15 @@ struct git_attr_check_elem {
  const char *value;
 };
 
+struct git_attr_check {
+ int check_nr;
+ int check_alloc;
+ struct git_attr_check_elem *check;
+};
+
+extern struct git_attr_check *git_attr_check_initl(const char *, ...);
+extern int git_check_attr(const char *path, struct git_attr_check *);
+
 /*
  * Return the name of the attribute represented by the argument.  The
  * return value is a pointer to a null-delimited string that is part
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 10/12] attr: convert git_all_attrs() to use "struct git_attr_check"

Junio C Hamano
In reply to this post by Junio C Hamano
This updates the other two ways the attribute check is done via an
array of "struct git_attr_check_elem" elements.  These two niches
appear only in "git check-attr".

 * The caller does not know offhand what attributes it wants to ask
   about and cannot use git_attr_check_initl() to prepare the
   git_attr_check structure.

 * The caller may not know what attributes it wants to ask at all,
   and instead wants to learn everything that the given path has.

Such a caller can call git_attr_check_alloc() to allocate an empty
git_attr_check, and then call git_attr_check_append() to add
attribute names one by one.  A new attribute can be appended until
git_attr_check structure is "finalized", which happens when it is
used to ask for attributes for any path by calling git_check_attr()
or git_all_attrs().  A git_attr_check structure that is initialized
by git_attr_check_initl() is already finalized when it is returned.

I am not at all happy with the way git_all_attrs() API turned out to
be, but it is only to support one niche caller ("check-attr --all"),
so I'll stop here for now.

Signed-off-by: Junio C Hamano <[hidden email]>
---
 attr.c               | 70 +++++++++++++++++++++++++++++++++++++---------------
 attr.h               | 16 +++++++-----
 builtin/check-attr.c | 54 ++++++++++++++++++----------------------
 3 files changed, 84 insertions(+), 56 deletions(-)

diff --git a/attr.c b/attr.c
index 285fc58..9c187bc 100644
--- a/attr.c
+++ b/attr.c
@@ -785,32 +785,21 @@ int git_check_attrs(const char *path, int num, struct git_attr_check_elem *check
  return 0;
 }
 
-int git_all_attrs(const char *path, int *num, struct git_attr_check_elem **check)
+void git_all_attrs(const char *path, struct git_attr_check *check)
 {
- int i, count, j;
+ int i;
 
+ git_attr_check_clear(check);
  collect_some_attrs(path, 0, NULL);
 
- /* Count the number of attributes that are set. */
- count = 0;
- for (i = 0; i < attr_nr; i++) {
- const char *value = check_all_attr[i].value;
- if (value != ATTR__UNSET && value != ATTR__UNKNOWN)
- ++count;
- }
- *num = count;
- ALLOC_ARRAY(*check, count);
- j = 0;
  for (i = 0; i < attr_nr; i++) {
+ const char *name = check_all_attr[i].attr->name;
  const char *value = check_all_attr[i].value;
- if (value != ATTR__UNSET && value != ATTR__UNKNOWN) {
- (*check)[j].attr = check_all_attr[i].attr;
- (*check)[j].value = value;
- ++j;
- }
+ if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+ continue;
+ git_attr_check_append(check, name);
+ check->check[check->check_nr - 1].value = value;
  }
-
- return 0;
 }
 
 void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
@@ -828,6 +817,7 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
 
 int git_check_attr(const char *path, struct git_attr_check *check)
 {
+ check->finalized = 1;
  return git_check_attrs(path, check->check_nr, check->check);
 }
 
@@ -845,17 +835,57 @@ struct git_attr_check *git_attr_check_initl(const char *one, ...)
  check = xcalloc(1,
  sizeof(*check) + cnt * sizeof(*(check->check)));
  check->check_nr = cnt;
+ check->finalized = 1;
  check->check = (struct git_attr_check_elem *)(check + 1);
 
  check->check[0].attr = git_attr(one);
  va_start(params, one);
  for (cnt = 1; cnt < check->check_nr; cnt++) {
+ struct git_attr *attr;
  param = va_arg(params, const char *);
  if (!param)
  die("BUG: counted %d != ended at %d",
     check->check_nr, cnt);
- check->check[cnt].attr = git_attr(param);
+ attr = git_attr(param);
+ if (!attr)
+ die("BUG: %s: not a valid attribute name", param);
+ check->check[cnt].attr = attr;
  }
  va_end(params);
  return check;
 }
+
+struct git_attr_check *git_attr_check_alloc(void)
+{
+ return xcalloc(1, sizeof(struct git_attr_check));
+}
+
+void git_attr_check_append(struct git_attr_check *check, const char *name)
+{
+ struct git_attr *attr;
+
+ if (check->finalized)
+ die("BUG: append after git_attr_check structure is finalized");
+
+ attr = git_attr(name);
+ if (!attr)
+ die("%s: not a valid attribute name", name);
+ ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
+ check->check[check->check_nr++].attr = attr;
+}
+
+void git_attr_check_clear(struct git_attr_check *check)
+{
+ if ((void *)(check->check) == (void *)(check + 1))
+ die("BUG: clearing a statically initialized git_attr_check");
+ free(check->check);
+ check->check_nr = 0;
+ check->check_alloc = 0;
+ check->finalized = 0;
+}
+
+void git_attr_check_free(struct git_attr_check *check)
+{
+ git_attr_check_clear(check);
+ free(check);
+}
diff --git a/attr.h b/attr.h
index 3ed89e5..b5557ae 100644
--- a/attr.h
+++ b/attr.h
@@ -30,6 +30,7 @@ struct git_attr_check_elem {
 };
 
 struct git_attr_check {
+ int finalized;
  int check_nr;
  int check_alloc;
  struct git_attr_check_elem *check;
@@ -38,6 +39,12 @@ struct git_attr_check {
 extern struct git_attr_check *git_attr_check_initl(const char *, ...);
 extern int git_check_attr(const char *path, struct git_attr_check *);
 
+extern struct git_attr_check *git_attr_check_alloc(void);
+extern void git_attr_check_append(struct git_attr_check *, const char *);
+
+extern void git_attr_check_clear(struct git_attr_check *);
+extern void git_attr_check_free(struct git_attr_check *);
+
 /*
  * Return the name of the attribute represented by the argument.  The
  * return value is a pointer to a null-delimited string that is part
@@ -48,13 +55,10 @@ char *git_attr_name(struct git_attr *);
 int git_check_attrs(const char *path, int, struct git_attr_check_elem *);
 
 /*
- * Retrieve all attributes that apply to the specified path.  *num
- * will be set to the number of attributes on the path; **check will
- * be set to point at a newly-allocated array of git_attr_check
- * objects describing the attributes and their values.  *check must be
- * free()ed by the caller.
+ * Retrieve all attributes that apply to the specified path.
+ * check holds the attributes and their values.
  */
-int git_all_attrs(const char *path, int *num, struct git_attr_check_elem **check);
+void git_all_attrs(const char *path, struct git_attr_check *check);
 
 enum git_attr_direction {
  GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 97e3837..700700f 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,12 +24,13 @@ static const struct option check_attr_options[] = {
  OPT_END()
 };
 
-static void output_attr(int cnt, struct git_attr_check_elem *check,
- const char *file)
+static void output_attr(struct git_attr_check *check, const char *file)
 {
  int j;
+ int cnt = check->check_nr;
+
  for (j = 0; j < cnt; j++) {
- const char *value = check[j].value;
+ const char *value = check->check[j].value;
 
  if (ATTR_TRUE(value))
  value = "set";
@@ -42,36 +43,37 @@ static void output_attr(int cnt, struct git_attr_check_elem *check,
  printf("%s%c" /* path */
        "%s%c" /* attrname */
        "%s%c" /* attrvalue */,
-       file, 0, git_attr_name(check[j].attr), 0, value, 0);
+       file, 0,
+       git_attr_name(check->check[j].attr), 0, value, 0);
  } else {
  quote_c_style(file, NULL, stdout, 0);
- printf(": %s: %s\n", git_attr_name(check[j].attr), value);
+ printf(": %s: %s\n",
+       git_attr_name(check->check[j].attr), value);
  }
-
  }
 }
 
 static void check_attr(const char *prefix,
-       int cnt, struct git_attr_check_elem *check,
+       struct git_attr_check *check,
        const char *file)
 {
  char *full_path =
  prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
  if (check != NULL) {
- if (git_check_attrs(full_path, cnt, check))
- die("git_check_attrs died");
- output_attr(cnt, check, file);
+ if (git_check_attr(full_path, check))
+ die("git_check_attr died");
+ output_attr(check, file);
  } else {
- if (git_all_attrs(full_path, &cnt, &check))
- die("git_all_attrs died");
- output_attr(cnt, check, file);
- free(check);
+ check = git_attr_check_alloc();
+ git_all_attrs(full_path, check);
+ output_attr(check, file);
+ git_attr_check_free(check);
  }
  free(full_path);
 }
 
 static void check_attr_stdin_paths(const char *prefix,
-   int cnt, struct git_attr_check_elem *check)
+   struct git_attr_check *check)
 {
  struct strbuf buf = STRBUF_INIT;
  struct strbuf unquoted = STRBUF_INIT;
@@ -85,7 +87,7 @@ static void check_attr_stdin_paths(const char *prefix,
  die("line is badly quoted");
  strbuf_swap(&buf, &unquoted);
  }
- check_attr(prefix, cnt, check, buf.buf);
+ check_attr(prefix, check, buf.buf);
  maybe_flush_or_die(stdout, "attribute to stdout");
  }
  strbuf_release(&buf);
@@ -100,7 +102,7 @@ static NORETURN void error_with_usage(const char *msg)
 
 int cmd_check_attr(int argc, const char **argv, const char *prefix)
 {
- struct git_attr_check_elem *check;
+ struct git_attr_check *check;
  int cnt, i, doubledash, filei;
 
  if (!is_bare_repository())
@@ -163,24 +165,16 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
  if (all_attrs) {
  check = NULL;
  } else {
- check = xcalloc(cnt, sizeof(*check));
- for (i = 0; i < cnt; i++) {
- const char *name;
- struct git_attr *a;
- name = argv[i];
- a = git_attr(name);
- if (!a)
- return error("%s: not a valid attribute name",
- name);
- check[i].attr = a;
- }
+ check = git_attr_check_alloc();
+ for (i = 0; i < cnt; i++)
+ git_attr_check_append(check, argv[i]);
  }
 
  if (stdin_paths)
- check_attr_stdin_paths(prefix, cnt, check);
+ check_attr_stdin_paths(prefix, check);
  else {
  for (i = filei; i < argc; i++)
- check_attr(prefix, cnt, check, argv[i]);
+ check_attr(prefix, check, argv[i]);
  maybe_flush_or_die(stdout, "attribute to stdout");
  }
  return 0;
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 11/12] attr: convert git_check_attrs() callers to use the new API

Junio C Hamano
In reply to this post by Junio C Hamano
The remaining callers are all simple "I have N attributes I am
interested in.  I'll ask about them with various paths one by one".

After this step, no caller to git_check_attrs() remains.  After
removing it, we can extend "struct git_attr_check" struct with data
that can be used in optimizing the query for the specific N
attributes it contains.

Signed-off-by: Junio C Hamano <[hidden email]>
---
 builtin/pack-objects.c | 19 +++++--------------
 convert.c              | 18 +++++++-----------
 ll-merge.c             | 33 ++++++++++++++-------------------
 userdiff.c             | 19 ++++++++-----------
 ws.c                   | 19 ++++++-------------
 5 files changed, 40 insertions(+), 68 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 167c301..c6c2a69 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -886,24 +886,15 @@ static void write_pack_file(void)
  written, nr_result);
 }
 
-static void setup_delta_attr_check(struct git_attr_check_elem *check)
-{
- static struct git_attr *attr_delta;
-
- if (!attr_delta)
- attr_delta = git_attr("delta");
-
- check[0].attr = attr_delta;
-}
-
 static int no_try_delta(const char *path)
 {
- struct git_attr_check_elem check[1];
+ static struct git_attr_check *check;
 
- setup_delta_attr_check(check);
- if (git_check_attrs(path, ARRAY_SIZE(check), check))
+ if (!check)
+ check = git_attr_check_initl("delta", NULL);
+ if (git_check_attr(path, check))
  return 0;
- if (ATTR_FALSE(check->value))
+ if (ATTR_FALSE(check->check[0].value))
  return 1;
  return 0;
 }
diff --git a/convert.c b/convert.c
index 058da86..510d1b9 100644
--- a/convert.c
+++ b/convert.c
@@ -760,24 +760,20 @@ struct conv_attrs {
  int ident;
 };
 
-static const char *conv_attr_name[] = {
- "crlf", "ident", "filter", "eol", "text",
-};
-#define NUM_CONV_ATTRS ARRAY_SIZE(conv_attr_name)
-
 static void convert_attrs(struct conv_attrs *ca, const char *path)
 {
- int i;
- static struct git_attr_check_elem ccheck[NUM_CONV_ATTRS];
+ static struct git_attr_check *check;
 
- if (!ccheck[0].attr) {
- for (i = 0; i < NUM_CONV_ATTRS; i++)
- ccheck[i].attr = git_attr(conv_attr_name[i]);
+ if (!check) {
+ check = git_attr_check_initl("crlf", "ident",
+     "filter", "eol", "text",
+     NULL);
  user_convert_tail = &user_convert;
  git_config(read_convert_config, NULL);
  }
 
- if (!git_check_attrs(path, NUM_CONV_ATTRS, ccheck)) {
+ if (!git_check_attr(path, check)) {
+ struct git_attr_check_elem *ccheck = check->check;
  ca->crlf_action = git_path_check_crlf(ccheck + 4);
  if (ca->crlf_action == CRLF_UNDEFINED)
  ca->crlf_action = git_path_check_crlf(ccheck + 0);
diff --git a/ll-merge.c b/ll-merge.c
index 772b14e..d4380c4 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -334,15 +334,6 @@ static const struct ll_merge_driver *find_ll_merge_driver(const char *merge_attr
  return &ll_merge_drv[LL_TEXT_MERGE];
 }
 
-static int git_path_check_merge(const char *path, struct git_attr_check_elem check[2])
-{
- if (!check[0].attr) {
- check[0].attr = git_attr("merge");
- check[1].attr = git_attr("conflict-marker-size");
- }
- return git_check_attrs(path, 2, check);
-}
-
 static void normalize_file(mmfile_t *mm, const char *path)
 {
  struct strbuf strbuf = STRBUF_INIT;
@@ -360,7 +351,7 @@ int ll_merge(mmbuffer_t *result_buf,
      mmfile_t *theirs, const char *their_label,
      const struct ll_merge_options *opts)
 {
- static struct git_attr_check_elem check[2];
+ static struct git_attr_check *check;
  static const struct ll_merge_options default_opts;
  const char *ll_driver_name = NULL;
  int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
@@ -374,10 +365,14 @@ int ll_merge(mmbuffer_t *result_buf,
  normalize_file(ours, path);
  normalize_file(theirs, path);
  }
- if (!git_path_check_merge(path, check)) {
- ll_driver_name = check[0].value;
- if (check[1].value) {
- marker_size = atoi(check[1].value);
+
+ if (!check)
+ check = git_attr_check_initl("merge", "conflict-marker-size", NULL);
+
+ if (!git_check_attr(path, check)) {
+ ll_driver_name = check->check[0].value;
+ if (check->check[1].value) {
+ marker_size = atoi(check->check[1].value);
  if (marker_size <= 0)
  marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
  }
@@ -392,13 +387,13 @@ int ll_merge(mmbuffer_t *result_buf,
 
 int ll_merge_marker_size(const char *path)
 {
- static struct git_attr_check_elem check;
+ static struct git_attr_check *check;
  int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 
- if (!check.attr)
- check.attr = git_attr("conflict-marker-size");
- if (!git_check_attrs(path, 1, &check) && check.value) {
- marker_size = atoi(check.value);
+ if (!check)
+ check = git_attr_check_initl("conflict-marker-size", NULL);
+ if (!git_check_attr(path, check) && check->check[0].value) {
+ marker_size = atoi(check->check[0].value);
  if (marker_size <= 0)
  marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
  }
diff --git a/userdiff.c b/userdiff.c
index 3560024..5473063 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -250,25 +250,22 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {
 
 struct userdiff_driver *userdiff_find_by_path(const char *path)
 {
- static struct git_attr *attr;
- struct git_attr_check_elem check;
-
- if (!attr)
- attr = git_attr("diff");
- check.attr = attr;
+ static struct git_attr_check *check;
 
+ if (!check)
+ check = git_attr_check_initl("diff", NULL);
  if (!path)
  return NULL;
- if (git_check_attrs(path, 1, &check))
+ if (git_check_attr(path, check))
  return NULL;
 
- if (ATTR_TRUE(check.value))
+ if (ATTR_TRUE(check->check[0].value))
  return &driver_true;
- if (ATTR_FALSE(check.value))
+ if (ATTR_FALSE(check->check[0].value))
  return &driver_false;
- if (ATTR_UNSET(check.value))
+ if (ATTR_UNSET(check->check[0].value))
  return NULL;
- return userdiff_find_by_name(check.value);
+ return userdiff_find_by_name(check->check[0].value);
 }
 
 struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)
diff --git a/ws.c b/ws.c
index 7350905..bb3270c 100644
--- a/ws.c
+++ b/ws.c
@@ -71,24 +71,17 @@ unsigned parse_whitespace_rule(const char *string)
  return rule;
 }
 
-static void setup_whitespace_attr_check(struct git_attr_check_elem *check)
-{
- static struct git_attr *attr_whitespace;
-
- if (!attr_whitespace)
- attr_whitespace = git_attr("whitespace");
- check[0].attr = attr_whitespace;
-}
-
 unsigned whitespace_rule(const char *pathname)
 {
- struct git_attr_check_elem attr_whitespace_rule;
+ static struct git_attr_check *attr_whitespace_rule;
+
+ if (!attr_whitespace_rule)
+ attr_whitespace_rule = git_attr_check_initl("whitespace", NULL);
 
- setup_whitespace_attr_check(&attr_whitespace_rule);
- if (!git_check_attrs(pathname, 1, &attr_whitespace_rule)) {
+ if (!git_check_attr(pathname, attr_whitespace_rule)) {
  const char *value;
 
- value = attr_whitespace_rule.value;
+ value = attr_whitespace_rule->check[0].value;
  if (ATTR_TRUE(value)) {
  /* true (whitespace) */
  unsigned all_rule = ws_tab_width(whitespace_rule_cfg);
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 12/12] attr: retire git_check_attrs() API

Junio C Hamano
In reply to this post by Junio C Hamano
Since nobody uses the old API, make it file-scope static, and update
the documentation to describe the new API.

Signed-off-by: Junio C Hamano <[hidden email]>
---
 Documentation/technical/api-gitattributes.txt | 62 +++++++++++++++------------
 attr.c                                        |  3 +-
 attr.h                                        |  2 -
 3 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 2602668..6f13f94 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -16,10 +16,15 @@ Data Structure
  of no interest to the calling programs.  The name of the
  attribute can be retrieved by calling `git_attr_name()`.
 
+`struct git_attr_check_elem`::
+
+ This structure represents one attribute and its value.
+
 `struct git_attr_check`::
 
- This structure represents a set of attributes to check in a call
- to `git_check_attr()` function, and receives the results.
+ This structure represents a collection of `git_attr_check_elem`.
+ It is passed to `git_check_attr()` function, specifying the
+ attributes to check, and receives their values.
 
 
 Attribute Values
@@ -48,49 +53,47 @@ value of the attribute for the path.
 Querying Specific Attributes
 ----------------------------
 
-* Prepare an array of `struct git_attr_check` to define the list of
-  attributes you would want to check.  To populate this array, you would
-  need to define necessary attributes by calling `git_attr()` function.
+* Prepare `struct git_attr_check` using git_attr_check_initl()
+  function, enumerating the names of attributes whose values you are
+  interested in, terminated with a NULL pointer.
 
 * Call `git_check_attr()` to check the attributes for the path.
 
-* Inspect `git_attr_check` structure to see how each of the attribute in
-  the array is defined for the path.
+* Inspect `git_attr_check` structure to see how each of the
+  attribute in the array is defined for the path.
 
 
 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
-  pointers to `struct git_attr` obtained by calling `git_attr()`:
+. Prepare a `struct git_attr_check` with two elements (because
+  we are checking two attributes):
 
 ------------
-static struct git_attr_check check[2];
+static struct git_attr_check *check;
 static void setup_check(void)
 {
- if (check[0].attr)
+ if (check)
  return; /* already done */
- check[0].attr = git_attr("crlf");
- check[1].attr = git_attr("ident");
+ check = git_attr_check_initl("crlf", "ident", NULL);
 }
 ------------
 
-. Call `git_check_attr()` with the prepared array of `struct git_attr_check`:
+. Call `git_check_attr()` with the prepared `struct git_attr_check`:
 
 ------------
  const char *path;
 
  setup_check();
- git_check_attr(path, ARRAY_SIZE(check), check);
+ git_check_attr(path, check);
 ------------
 
-. Act on `.value` member of the result, left in `check[]`:
+. Act on `.value` member of the result, left in `check->check[]`:
 
 ------------
- const char *value = check[0].value;
+ const char *value = check->check[0].value;
 
  if (ATTR_TRUE(value)) {
  The attribute is Set, by listing only the name of the
@@ -115,14 +118,17 @@ Querying All Attributes
 
 To get the values of all attributes associated with a file:
 
-* Call `git_all_attrs()`, which returns an array of `git_attr_check`
-  structures.
+* Prepare an empty `git_attr_check` structure by calling
+  `git_attr_check_alloc()`.
+
+* Call `git_all_attrs()`, which populates the `git_attr_check`
+  with the attributes attached to the path.
 
-* Iterate over the `git_attr_check` array to examine the attribute
-  names and values.  The name of the attribute described by a
-  `git_attr_check` object can be retrieved via
-  `git_attr_name(check[i].attr)`.  (Please note that no items will be
-  returned for unset attributes, so `ATTR_UNSET()` will return false
-  for all returned `git_array_check` objects.)
+* Iterate over the `git_attr_check.check[]` array to examine
+  the attribute names and values.  The name of the attribute
+  described by a  `git_attr_check.check[]` object can be retrieved via
+  `git_attr_name(check->check[i].attr)`.  (Please note that no items
+  will be returned for unset attributes, so `ATTR_UNSET()` will return
+  false for all returned `git_array_check` objects.)
 
-* Free the `git_array_check` array.
+* Free the `git_array_check` by calling `git_attr_check_free()`.
diff --git a/attr.c b/attr.c
index 9c187bc..c3295a9 100644
--- a/attr.c
+++ b/attr.c
@@ -769,7 +769,8 @@ static void collect_some_attrs(const char *path, int num,
  rem = fill(path, pathlen, basename_offset, stk, rem);
 }
 
-int git_check_attrs(const char *path, int num, struct git_attr_check_elem *check)
+static int git_check_attrs(const char *path, int num,
+   struct git_attr_check_elem *check)
 {
  int i;
 
diff --git a/attr.h b/attr.h
index b5557ae..7dc49f8 100644
--- a/attr.h
+++ b/attr.h
@@ -52,8 +52,6 @@ extern void git_attr_check_free(struct git_attr_check *);
  */
 char *git_attr_name(struct git_attr *);
 
-int git_check_attrs(const char *path, int, struct git_attr_check_elem *);
-
 /*
  * Retrieve all attributes that apply to the specified path.
  * check holds the attributes and their values.
--
2.8.2-748-gfb85f76

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 01/12] commit.c: use strchrnul() to scan for one line

Stefan Beller-4
In reply to this post by Junio C Hamano
On Mon, May 16, 2016 at 2:05 PM, Junio C Hamano <[hidden email]> wrote:

> Signed-off-by: Junio C Hamano <[hidden email]>
> ---
>  commit.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 3f4f371..1f9ee8a 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
>                 p++;
>         if (*p) {
>                 p += 2;
> -               for (eol = p; *eol && *eol != '\n'; eol++)
> -                       ; /* do nothing */
> +               eol = strchrnul(p, '\n');

Nit:
You could include the +2 into the arg of  strchrnul,
such that you can drop the braces.

>         } else
>                 eol = p;
>
> --
> 2.8.2-748-gfb85f76
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 02/12] attr.c: use strchrnul() to scan for one line

Stefan Beller-4
In reply to this post by Junio C Hamano
On Mon, May 16, 2016 at 2:05 PM, Junio C Hamano <[hidden email]> wrote:

> Signed-off-by: Junio C Hamano <[hidden email]>
> ---
>  attr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index eec5d7d..45aec1b 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -402,8 +402,8 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
>         for (sp = buf; *sp; ) {
>                 char *ep;
>                 int more;
> -               for (ep = sp; *ep && *ep != '\n'; ep++)
> -                       ;
> +
> +               ep = strchrnul(sp, '\n');

(Even lesser nit as in patch 1)
You could directly assign ep and more when declaring them.

>                 more = (*ep == '\n');
>                 *ep = '\0';
>                 handle_attr_line(res, sp, path, ++lineno, macro_ok);
> --
> 2.8.2-748-gfb85f76
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 03/12] attr.c: update a stale comment on "struct match_attr"

Stefan Beller-4
In reply to this post by Junio C Hamano
On Mon, May 16, 2016 at 2:05 PM, Junio C Hamano <[hidden email]> wrote:

> When 82dce998 (attr: more matching optimizations from .gitignore,
> 2012-10-15) changed a pointer to a string "*pattern" into an
> embedded "struct pattern" in struct match_attr, it forgot to update
> the comment that describes the structure.
>
> Signed-off-by: Junio C Hamano <[hidden email]>
> ---
>  attr.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index 45aec1b..4ae7801 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -131,9 +131,8 @@ struct pattern {
>   * 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.)
> + * If is_macro is false, then u.pat is the filename pattern to which the
> + * rule applies.

and we don't need to talk about memory ownership here as that
is clear for 'struct pattern' documented elsewhere?

>   *
>   * In either case, num_attr is the number of attributes affected by
>   * this rule, and state is an array listing them.  The attributes are
> --
> 2.8.2-748-gfb85f76
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 03/12] attr.c: update a stale comment on "struct match_attr"

Junio C Hamano
On Mon, May 16, 2016 at 4:34 PM, Stefan Beller <[hidden email]> wrote:
>> - * 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.)
>> + * If is_macro is false, then u.pat is the filename pattern to which the
>> + * rule applies.
>
> and we don't need to talk about memory ownership here as that
> is clear for 'struct pattern' documented elsewhere?

It is embedded in the structure which you cannot lose independently, no?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 01/12] commit.c: use strchrnul() to scan for one line

Junio C Hamano
In reply to this post by Stefan Beller-4
On Mon, May 16, 2016 at 4:19 PM, Stefan Beller <[hidden email]> wrote:
>>         if (*p) {
>>                 p += 2;
>> -               for (eol = p; *eol && *eol != '\n'; eol++)
>> -                       ; /* do nothing */
>> +               eol = strchrnul(p, '\n');
>
> Nit:
> You could include the +2 into the arg of  strchrnul,
> such that you can drop the braces.

You're right. With or without braces,

  eol = strchrnul(p + 2, '\n');

would be easier to understand (I do not know offhand if the value of p
matters after this part, as I am responding from GMail Web UI without
access to the wider context in the source, though).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 02/12] attr.c: use strchrnul() to scan for one line

Junio C Hamano
In reply to this post by Stefan Beller-4
On Mon, May 16, 2016 at 4:28 PM, Stefan Beller <[hidden email]> wrote:

>>         for (sp = buf; *sp; ) {
>>                 char *ep;
>>                 int more;
>> -               for (ep = sp; *ep && *ep != '\n'; ep++)
>> -                       ;
>> +
>> +               ep = strchrnul(sp, '\n');
>
> (Even lesser nit as in patch 1)
> You could directly assign ep and more when declaring them.

I'd prefer not to.

"A declaration block, blank and statement" pattern allows you
to have the declaration of variables to group related things
together and in an order that makes it easier to explain.
If you throw in initialization there, you cannot do that anymore
(e.g. ep must be initialized before you can compute more).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 01/12] commit.c: use strchrnul() to scan for one line

Stefan Beller-4
In reply to this post by Junio C Hamano
On Mon, May 16, 2016 at 4:41 PM, Junio C Hamano <[hidden email]> wrote:

> On Mon, May 16, 2016 at 4:19 PM, Stefan Beller <[hidden email]> wrote:
>>>         if (*p) {
>>>                 p += 2;
>>> -               for (eol = p; *eol && *eol != '\n'; eol++)
>>> -                       ; /* do nothing */
>>> +               eol = strchrnul(p, '\n');
>>
>> Nit:
>> You could include the +2 into the arg of  strchrnul,
>> such that you can drop the braces.
>
> You're right. With or without braces,
>
>   eol = strchrnul(p + 2, '\n');
>
> would be easier to understand (I do not know offhand if the value of p
> matters after this part, as I am responding from GMail Web UI without
> access to the wider context in the source, though).

Heh, good point, I did not think it through apparently. `p` matters.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
12