Re: [PATCH 03/14] strutil: add skip_prefix_icase

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Dec 29, 2015 at 2:22 AM, Jeff King <peff@peff.net> wrote:
> Some sites that otherwise would use skip_prefix cannot do
> so, because it has no way to do case-insensitive
> comparisons. Such sites usually get around this by using
> strncasecmp, at the cost of having to use magic numbers.
> We can help them by providing a case-insensitive version of
> skip_prefix.
>
> Unfortunately, we don't share any code with the original
> skip_prefix. Since this is performance-sensitive code, we
> would not want to introduce an extra "do we are about case?"
> conditional into the middle of the loop. We could instead
> use macros or another technique to generate the
> almost-identical implementations, but the function simply
> isn't long enough to merit that confusing boilerplate.
>
> To show off the new function, we convert a simple case in
> log's add_header function.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/log.c b/builtin/log.c
> @@ -677,10 +677,10 @@ static void add_header(const char *value)
> -       if (!strncasecmp(value, "to: ", 4))
> -               item = string_list_append(&extra_to, value + 4);
> -       else if (!strncasecmp(value, "cc: ", 4))
> -               item = string_list_append(&extra_cc, value + 4);
> +       if (skip_prefix_icase(value, "to: ", &value))
> +               item = string_list_append(&extra_to, value);
> +       else if (skip_prefix_icase(value, "cc: ", &value))
> +               item = string_list_append(&extra_cc, value);

Is it worth holding this patch, with its introduction of
skip_prefix_icase(), hostage to the unrelated change in the previous
patch (2/14) which touches the same bit of code? Would it make sense
to split this change out?

>         else
>                 item = string_list_append(&extra_hdr, value);
>
> diff --git a/strutil.h b/strutil.h
> @@ -32,6 +32,21 @@ static inline int skip_prefix(const char *str, const char *prefix,
>  /*
> + * Identical to skip_prefix, but compare characters case-insensitively.
> + */
> +static inline int skip_prefix_icase(const char *str, const char *prefix,
> +                                   const char **out)
> +{
> +       do {
> +               if (!*prefix) {
> +                       *out = str;
> +                       return 1;
> +               }
> +       } while (tolower(*str++) == tolower(*prefix++));

I wondered initially if we should be concerned about invoking
tolower() with an expression with side-effects since some older and/or
non-compliant libraries implement it as a macro which doesn't ensure
that the argument is evaluated only once. However, it seems that Git
already uses tolower(*p++) in a few other places, so I guess we're not
worrying about those broken implementations.

> +       return 0;
> +}
> +
> +/*