Re: [PATCH 17/36] attr: expose validity check for attribute names
On Mon, Oct 24, 2016 at 2:07 PM, Stefan Bellerwrote: > On Sun, Oct 23, 2016 at 8:07 AM, Ramsay Jones > wrote: >> >> >> On 23/10/16 00:32, Stefan Beller wrote: >>> From: Junio C Hamano >>> >>> Export attr_name_valid() function, and a helper function that >>> returns the message to be given when a given pair >>> is not a good name for an attribute. >>> >>> We could later update the message to exactly spell out what the >>> rules for a good attribute name are, etc. >>> >>> Signed-off-by: Junio C Hamano >>> Signed-off-by: Stefan Beller >>> --- >> >> [snip] >> >>> +extern int attr_name_valid(const char *name, size_t namelen); >>> +extern void invalid_attr_name_message(struct strbuf *, const char *, int); >>> + >> >> The symbol 'attr_name_valid()' is not used outside of attr.c, even >> by the end of this series. Do you expect this function to be used >> in any future series? (The export is deliberate and it certainly >> seems like it should be part of the public interface, but ...) >> refactoring this series again, I will make use of attr_name_valid outside of attr.c, so I'll keep this patch.
Re: [PATCH 17/36] attr: expose validity check for attribute names
On Sun, Oct 23, 2016 at 8:07 AM, Ramsay Joneswrote: > > > On 23/10/16 00:32, Stefan Beller wrote: >> From: Junio C Hamano >> >> Export attr_name_valid() function, and a helper function that >> returns the message to be given when a given pair >> is not a good name for an attribute. >> >> We could later update the message to exactly spell out what the >> rules for a good attribute name are, etc. >> >> Signed-off-by: Junio C Hamano >> Signed-off-by: Stefan Beller >> --- > > [snip] > >> +extern int attr_name_valid(const char *name, size_t namelen); >> +extern void invalid_attr_name_message(struct strbuf *, const char *, int); >> + > > The symbol 'attr_name_valid()' is not used outside of attr.c, even > by the end of this series. Do you expect this function to be used > in any future series? (The export is deliberate and it certainly > seems like it should be part of the public interface, but ...) > > In contrast, the 'invalid_attr_name_message()' function is called > from code in pathspec.c, which relies on 'git_attr_counted()' to > call 'attr_name_valid()' internally to check for validity. :-D Yeah, I am taking over Junios patches and do not quite implement what Junio thought I would. ;) So I guess it is a communication mismatch. git_attr_counted is a wrapper around attr_name_valid in the way that it either returns NULL when the attr name is invalid or it does extra work and returns a pointer to an attr. So I think for API completeness we'd want to keep attr_name_valid around, as otherwise the API looks strange. But that doesn't seem like a compelling reason, so I'll drop it from the header file and make it static. Thanks, Stefan
Re: [PATCH 17/36] attr: expose validity check for attribute names
On 23/10/16 00:32, Stefan Beller wrote: > From: Junio C Hamano> > Export attr_name_valid() function, and a helper function that > returns the message to be given when a given pair > is not a good name for an attribute. > > We could later update the message to exactly spell out what the > rules for a good attribute name are, etc. > > Signed-off-by: Junio C Hamano > Signed-off-by: Stefan Beller > --- [snip] > +extern int attr_name_valid(const char *name, size_t namelen); > +extern void invalid_attr_name_message(struct strbuf *, const char *, int); > + The symbol 'attr_name_valid()' is not used outside of attr.c, even by the end of this series. Do you expect this function to be used in any future series? (The export is deliberate and it certainly seems like it should be part of the public interface, but ...) In contrast, the 'invalid_attr_name_message()' function is called from code in pathspec.c, which relies on 'git_attr_counted()' to call 'attr_name_valid()' internally to check for validity. :-D ATB, Ramsay Jones
[PATCH 17/36] attr: expose validity check for attribute names
From: Junio C HamanoExport attr_name_valid() function, and a helper function that returns the message to be given when a given pair is not a good name for an attribute. We could later update the message to exactly spell out what the rules for a good attribute name are, etc. Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller --- attr.c | 39 +-- attr.h | 3 +++ 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/attr.c b/attr.c index 90dbacd..659dc41 100644 --- a/attr.c +++ b/attr.c @@ -59,23 +59,38 @@ static unsigned hash_name(const char *name, int namelen) return val; } -static int invalid_attr_name(const char *name, int namelen) +int attr_name_valid(const char *name, size_t namelen) { /* * Attribute name cannot begin with '-' and must consist of * characters from [-A-Za-z0-9_.]. */ if (namelen <= 0 || *name == '-') - return -1; + return 0; while (namelen--) { char ch = *name++; if (! (ch == '-' || ch == '.' || ch == '_' || ('0' <= ch && ch <= '9') || ('a' <= ch && ch <= 'z') || ('A' <= ch && ch <= 'Z')) ) - return -1; + return 0; } - return 0; + return 1; +} + +void invalid_attr_name_message(struct strbuf *err, const char *name, int len) +{ + strbuf_addf(err, _("%.*s is not a valid attribute name"), + len, name); +} + +static void report_invalid_attr(const char *name, size_t len, + const char *src, int lineno) +{ + struct strbuf err = STRBUF_INIT; + invalid_attr_name_message(, name, len); + fprintf(stderr, "%s: %s:%d\n", err.buf, src, lineno); + strbuf_release(); } struct git_attr *git_attr_counted(const char *name, size_t len) @@ -90,7 +105,7 @@ struct git_attr *git_attr_counted(const char *name, size_t len) return a; } - if (invalid_attr_name(name, len)) + if (!attr_name_valid(name, len)) return NULL; FLEX_ALLOC_MEM(a, name, name, len); @@ -176,17 +191,15 @@ static const char *parse_attr(const char *src, int lineno, const char *cp, cp++; len--; } - if (invalid_attr_name(cp, len)) { - fprintf(stderr, - "%.*s is not a valid attribute name: %s:%d\n", - len, cp, src, lineno); + if (!attr_name_valid(cp, len)) { + report_invalid_attr(cp, len, src, lineno); 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() +* the second pass, no need for attr_name_valid() * check here. */ if (*cp == '-' || *cp == '!') { @@ -229,10 +242,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src, name += strlen(ATTRIBUTE_MACRO_PREFIX); name += strspn(name, blank); namelen = strcspn(name, blank); - if (invalid_attr_name(name, namelen)) { - fprintf(stderr, - "%.*s is not a valid attribute name: %s:%d\n", - namelen, name, src, lineno); + if (!attr_name_valid(name, namelen)) { + report_invalid_attr(name, namelen, src, lineno); goto fail_return; } } diff --git a/attr.h b/attr.h index bcedf92..40abc16 100644 --- a/attr.h +++ b/attr.h @@ -13,6 +13,9 @@ extern struct git_attr *git_attr(const char *); /* The same, but with counted string */ extern struct git_attr *git_attr_counted(const char *, size_t); +extern int attr_name_valid(const char *name, size_t namelen); +extern void invalid_attr_name_message(struct strbuf *, const char *, int); + /* Internal use */ extern const char git_attr__true[]; extern const char git_attr__false[]; -- 2.10.1.508.g6572022