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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 31, 2015 at 01:40:46AM -0500, Eric Sunshine wrote:

> > 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?

I assumed that this was at least as contentious as 2/14, so it wouldn't
be a problem. I'm inclined to leave it, unless it looks like we'll scrap
2/14.

> > +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.

Right. There are a few other gotchas with tolower(), and we work around
it by implementing the ctype functions ourselves (even on non-broken
platforms). It's one of the oldest bits of git-compat-util.h.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]