Re: [PATCH 03/14] strutil: add skip_prefix_icase
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Dec 31, 2015, 06:40 UTC
- Message-ID
- <CAPig+cTF_HF-xoiV2osEi6RnGBKQTmkvmUcUn1GbYHVP15_hDA@mail.gmail.com>
- In-Reply-To
- <20151229072204.GC8842@sigill.intra.peff.net>
On Tue, Dec 29, 2015 at 2:22 AM, Jeff King <peff@peff.net> wrote:
Show 30 quoted lines
> 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?
Show 17 quoted lines
> 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; > +} > + > +/*