git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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;
> +}
> +
> +/*
Previous: Jeff KingNext: Jeff King
Message 8 of 29 in “counting trailers with shortlogs”
  1. 0/14 counting trailers with shortlogsJeff King, Dec 29, 2015
  2. 01/14 move string functions out of git-compat-utilJeff King, Dec 29, 2015
  3. 02/14 log: refactor add_header to drop some magic numbersJeff King, Dec 29, 2015
  4. Eric SunshineDec 31, 2015
  5. Jeff KingJan 1, 2016
  6. Jeff KingJan 1, 2016
  7. 03/14 strutil: add skip_prefix_icaseJeff King, Dec 29, 2015
  8. Eric SunshineDec 31, 2015
  9. Jeff KingJan 1, 2016
  10. 04/14 shortlog: use skip_prefix_icase to parse "Author" linesJeff King, Dec 29, 2015
  11. Eric SunshineDec 31, 2015
  12. Jeff KingJan 1, 2016
  13. 05/14 shortlog: use strbufs to read from stdinJeff King, Dec 29, 2015
  14. 06/14 shortlog: replace hand-parsing of author with pretty-printerJeff King, Dec 29, 2015
  15. Eric SunshineJan 4, 2016
  16. Jeff KingJan 4, 2016
  17. 07/14 shortlog: optimize "--summary" modeJeff King, Dec 29, 2015
  18. 08/14 shortlog: optimize out useless "<none>" normalizationJeff King, Dec 29, 2015
  19. 09/14 shortlog: optimize out useless string listJeff King, Dec 29, 2015
  20. 10/14 shortlog: change "author" variables to "ident"Jeff King, Dec 29, 2015
  21. 11/14 shortlog: allow grouping by committer identJeff King, Dec 29, 2015
  22. Eric SunshineJan 4, 2016
  23. Jeff KingJan 4, 2016
  24. 12/14 trailer: factor out config readingJeff King, Dec 29, 2015
  25. 13/14 trailer: add interface for parsing commit trailersJeff King, Dec 29, 2015
  26. 14/14 shortlog: match commit trailers with --identJeff King, Dec 29, 2015
  27. Jeff KingDec 29, 2015
  28. Eric SunshineJan 4, 2016
  29. Jeff KingJan 4, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.