Re: [PATCH 04/10] attr: more matching optimizations from .gitignore
Nguyen Thai Ngoc Duy pclo...@gmail.com writes: My objection is no-op lines are timed bombs. But users can already do dir attr (no slashes), which is no-op. So yeah, no-op is fine. Exactly. If you are not catching and barfing the no-slashed variant at the syntax level (and you shouldn't), you shouldn't do so for slashed ones. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] attr: more matching optimizations from .gitignore
On Sat, Oct 6, 2012 at 1:48 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: +Unlike `.gitignore`, negative patterns are not supported. +Patterns that match directories are also not supported. Is are not supported the right phrasing? I think it makes perfect sense not to forbid !path attr1, because it is unclear what it means (e.g. path -attr1 vs path !attr1). So I would say Negative patterns are forbidden as they do not make any sense. !path/sub/ alone does not mean anything. It must be used together with a positive pattern to define the set of paths the same attribute assignment statement applies to. This makes sense (attr, -attr1 or !attr1 are all OK): *.c attr1 !foo.c attr1 But this does not (actually !foo.c line has no effects because of different attribute assignments): *.c attr1 !foo.c attr2 It could be even more confusing in multiple attribute manipulation: *.c attr1 *.h -attr2 !foo.[ch] attr1 -attr2 So not supported and forbidden are equally OK. I just want to raise a point that it has some use before we go for forbidden. Nguyen Thai Ngoc Duy pclo...@gmail.com writes: On Sat, Oct 6, 2012 at 12:36 PM, Junio C Hamano gits...@pobox.com wrote: Or the user might think path/ attr1 sets attr1 for all files under path/ because it does not make sense to attach attributes to a directory in git. ... We may not have a need to assign a real attribute to a directory right now, because nothing in Git asks for an attribute for a directory. But that does not necessarily mean we would never need a way to give an attribute to a directory but not to its contents. Exactly why we should not make path/ attr no-op. If we want to make it meaningful some day in future, I don't think we want those no-ops lay around and suddenly cause changes in behavior with a new version of git. I do not think you understood. path/ attr is a no-op from the point of view of the *users* of the current versions of Git. It is perfectly fine to accept and apply attr to path/; no codepath in Git should be asking about path/ anyway, so it ends up to be a no-op. You shouldn't be erroring out at the syntactic level, i.e. when these lines are parsed. My objection is no-op lines are timed bombs. But users can already do dir attr (no slashes), which is no-op. So yeah, no-op is fine. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] attr: more matching optimizations from .gitignore
On Sat, Oct 6, 2012 at 12:36 PM, Junio C Hamano gits...@pobox.com wrote: Or the user might think path/ attr1 sets attr1 for all files under path/ because it does not make sense to attach attributes to a directory in git. ... We may not have a need to assign a real attribute to a directory right now, because nothing in Git asks for an attribute for a directory. But that does not necessarily mean we would never need a way to give an attribute to a directory but not to its contents. Exactly why we should not make path/ attr no-op. If we want to make it meaningful some day in future, I don't think we want those no-ops lay around and suddenly cause changes in behavior with a new version of git. If one does not think it through, the path/ excluded example might appear that there is no difference between setting exclude to the path itself and setting it to path and everything underneath it, but that comes largely from the nature of exclude attribute (think of exclude attribute as exclude itself and everything under it). From a user perspective, the thinking through portion is usually less than the try-and-see. There is no reason to assume other attributes we may want to give to a directory share the same recursive kind of semantics. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] attr: more matching optimizations from .gitignore
Nguyen Thai Ngoc Duy pclo...@gmail.com writes: On Sat, Oct 6, 2012 at 12:36 PM, Junio C Hamano gits...@pobox.com wrote: Or the user might think path/ attr1 sets attr1 for all files under path/ because it does not make sense to attach attributes to a directory in git. ... We may not have a need to assign a real attribute to a directory right now, because nothing in Git asks for an attribute for a directory. But that does not necessarily mean we would never need a way to give an attribute to a directory but not to its contents. Exactly why we should not make path/ attr no-op. If we want to make it meaningful some day in future, I don't think we want those no-ops lay around and suddenly cause changes in behavior with a new version of git. I do not think you understood. path/ attr is a no-op from the point of view of the *users* of the current versions of Git. It is perfectly fine to accept and apply attr to path/; no codepath in Git should be asking about path/ anyway, so it ends up to be a no-op. You shouldn't be erroring out at the syntactic level, i.e. when these lines are parsed. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/10] attr: more matching optimizations from .gitignore
.gitattributes and .gitignore share the same pattern syntax but has
separate matching implementation. Over the years, ignore's
implementation accumulates more optimizations while attr's stays the
same.
This patch adds those optimizations to .gitattributes. Basically it
tries to avoid fnmatch/wildmatch in favor of strncmp as much as
possible.
There are two syntaxes that .gitignore supports but .gitattributes
does not: negative patterns and directory matching. They have never
worked and whether they will is up for future discussion. Meanwhile
make a note in the document and reject such patterns.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/gitattributes.txt | 2 ++
attr.c | 68 ++---
dir.c | 8 ++---
dir.h | 1 +
4 files changed, 64 insertions(+), 15 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e16f3e1..702b8c1 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -56,6 +56,8 @@ When more than one pattern matches the path, a later line
overrides an earlier line. This overriding is done per
attribute. The rules how the pattern matches paths are the
same as in `.gitignore` files; see linkgit:gitignore[5].
+Unlike `.gitignore`, negative patterns are not supported.
+Patterns that match directories are also not supported.
When deciding what attributes are assigned to a path, git
consults `$GIT_DIR/info/attributes` file (which has the highest
diff --git a/attr.c b/attr.c
index aeac564..1aa058e 100644
--- a/attr.c
+++ b/attr.c
@@ -115,6 +115,13 @@ struct attr_state {
const char *setto;
};
+struct pattern {
+ const char *pattern;
+ int patternlen;
+ int nowildcardlen;
+ int flags; /* EXC_FLAG_* */
+};
+
/*
* One rule, as from a .gitattributes file.
*
@@ -131,7 +138,7 @@ struct attr_state {
*/
struct match_attr {
union {
- char *pattern;
+ struct pattern pat;
struct git_attr *attr;
} u;
char is_macro;
@@ -241,9 +248,18 @@ static struct match_attr *parse_attr_line(const char
*line, const char *src,
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;
+ char *p = (char *)(res-state[num_attr]);
+ memcpy(p, name, namelen);
+ p[namelen] = 0;
+ res-u.pat.pattern = p;
+ parse_exclude_pattern(res-u.pat.pattern,
+ res-u.pat.patternlen,
+ res-u.pat.flags,
+ res-u.pat.nowildcardlen);
+ if (res-u.pat.flags EXC_FLAG_NEGATIVE)
+ die(_(Negative patterns are not supported in git
attributes));
+ if (res-u.pat.flags EXC_FLAG_MUSTBEDIR)
+ die(_(Directory patterns are not supported in git
attributes));
}
res-is_macro = is_macro;
res-num_attr = num_attr;
@@ -645,25 +661,55 @@ static void prepare_attr_stack(const char *path)
static int path_matches(const char *pathname, int pathlen,
const char *basename,
- const char *pattern,
+ const struct pattern *pat,
const char *base, int baselen)
{
- if (!strchr(pattern, '/')) {
+ const char *pattern = pat-pattern;
+ int prefix = pat-nowildcardlen;
+ const char *name;
+ int namelen;
+
+ if (pat-flags EXC_FLAG_NODIR) {
+ if (prefix == pat-patternlen
+ !strcmp_icase(pattern, basename))
+ return 1;
+
+ if (pat-flags EXC_FLAG_ENDSWITH
+ pat-patternlen - 1 = pathlen
+ !strcmp_icase(pattern + 1, pathname +
+ pathlen - pat-patternlen + 1))
+ return 1;
+
return (fnmatch_icase(pattern, basename, 0) == 0);
}
/*
* match with FNM_PATHNAME; the pattern has base implicitly
* in front of it.
*/
- if (*pattern == '/')
+ if (*pattern == '/') {
pattern++;
+ prefix--;
+ }
+
+ /*
+* note: unlike excluded_from_list, baselen here does not
+* contain the trailing slash
+*/
+
if (pathlen baselen ||
(baselen pathname[baselen] != '/') ||
strncmp(pathname, base, baselen))
return 0;
- if (baselen != 0)
- baselen++;
- return fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;
+
+
Re: [PATCH 04/10] attr: more matching optimizations from .gitignore
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: +Unlike `.gitignore`, negative patterns are not supported. +Patterns that match directories are also not supported. Is are not supported the right phrasing? I think it makes perfect sense not to forbid !path attr1, because it is unclear what it means (e.g. path -attr1 vs path !attr1). So I would say Negative patterns are forbidden as they do not make any sense. But for the latter, I think it makes a lot more sense to just accept path/ attr1 and doing nothing. The user requests to set an attribute to path that has to be a directory, and there is nothing wrong in such a request in itself. But nothing in git asks for attributes for directories (because we do not track directories), and such a request happens to be a no-op. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] attr: more matching optimizations from .gitignore
On Sat, Oct 6, 2012 at 1:48 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: +Unlike `.gitignore`, negative patterns are not supported. +Patterns that match directories are also not supported. Is are not supported the right phrasing? I think it makes perfect sense not to forbid !path attr1, because it is unclear what it means (e.g. path -attr1 vs path !attr1). So I would say Negative patterns are forbidden as they do not make any sense. OK But for the latter, I think it makes a lot more sense to just accept path/ attr1 and doing nothing. The user requests to set an attribute to path that has to be a directory, and there is nothing wrong in such a request in itself. But nothing in git asks for attributes for directories (because we do not track directories), and such a request happens to be a no-op. Or the user might think path/ attr1 sets attr1 for all files under path/ because it does not make sense to attach attributes to a directory in git. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
