Re: [PATCH] attr: expose validity check for attribute names

[Stefan Beller <sbeller@google.com>]

On Wed, May 18, 2016 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Export attr_name_valid() function, and a helper function that
> returns the message to be given when a given <name, len> 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 <gitster@pobox.com>
> ---
>  * And then finally the error message one.  I didn't change
>    the message itself to spell out the rules here, though.
>
>  attr.c | 39 +++++++++++++++++++++++++--------------
>  attr.h |  3 +++
>  2 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index 7f20e21..8f54871 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)

Mental note:
The patch series with the pathspec attrs on top of this series makes this
non-static and exposes the invalid_attr_name for wider use, so I'll resolve
the merge conflict accordingly.

>  {
>         /*
>          * 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(&err, name, len);
> +       fprintf(stderr, "%s: %s:%d\n", err.buf, src, lineno);
> +       strbuf_release(&err);
>  }
>
>  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);
>                         return NULL;
>                 }
>         }
> diff --git a/attr.h b/attr.h
> index 78d6d5a..fc72030 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);

ok, I agree with this one.

> +extern void invalid_attr_name_message(struct strbuf *, const char *, int);

This makes sense, too, so the caller can just collect the error message and
attach its own flair to it, i.e. another message to explain the context about
wrong attributes.

Thanks,
Stefan

> +
>  /* Internal use */
>  extern const char git_attr__true[];
>  extern const char git_attr__false[];
> --
> 2.8.2-759-geb611ab
>