{"thread":{"id":"36729","subject":"[PATCH 0/2] tolower cleanups","startedAt":"2014-05-22T09:43:08Z","lastAt":"2014-05-22T23:17:59Z","messageCount":5,"participants":["Jeff King","Kyle J. McKay"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"242475","messageId":"20140522094251.GA14994@sigill.intra.peff.net","threadId":"36729","inReplyTo":null,"subject":"[PATCH 0/2] tolower cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:43:08Z","receivedAt":"2014-05-22T09:43:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These two patches were pulled from the http charset series I posted\nnearby. The second iteration of that series did not need them, but they\nmay have value as cleanups.\n\n  [1/2]: daemon/config: factor out duplicate xstrdup_tolower\n  [2/2]: strbuf: add strbuf_tolower function\n\nThe first one is a real reduction of code. The second is just making\ncode public that _might_ get used later, so it's a bit less exciting.\nBoth are independent of each other, so we can take or leave them\nseparately.\n"},{"id":"242476","messageId":"20140522094358.GA15255@sigill.intra.peff.net","threadId":"36729","inReplyTo":"20140522094251.GA14994@sigill.intra.peff.net","subject":"[PATCH 1/2] daemon/config: factor out duplicate xstrdup_tolower","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:44:09Z","receivedAt":"2014-05-22T09:44:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have two implementations of the same function; let's drop\nthat to one. We take the name from daemon.c, but the\nimplementation (which is just slightly more efficient) from\nthe config code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/config.c | 15 +--------------\n daemon.c         |  8 --------\n strbuf.c         | 13 +++++++++++++\n strbuf.h         |  2 ++\n 4 files changed, 16 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 5677c94..fcd8474 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -395,19 +395,6 @@ static int urlmatch_collect_fn(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n-static char *dup_downcase(const char *string)\n-{\n-\tchar *result;\n-\tsize_t len, i;\n-\n-\tlen = strlen(string);\n-\tresult = xmalloc(len + 1);\n-\tfor (i = 0; i < len; i++)\n-\t\tresult[i] = tolower(string[i]);\n-\tresult[i] = '\\0';\n-\treturn result;\n-}\n-\n static int get_urlmatch(const char *var, const char *url)\n {\n \tchar *section_tail;\n@@ -422,7 +409,7 @@ static int get_urlmatch(const char *var, const char *url)\n \tif (!url_normalize(url, &config.url))\n \t\tdie(\"%s\", config.url.err);\n \n-\tconfig.section = dup_downcase(var);\n+\tconfig.section = xstrdup_tolower(var);\n \tsection_tail = strchr(config.section, '.');\n \tif (section_tail) {\n \t\t*section_tail = '\\0';\ndiff --git a/daemon.c b/daemon.c\nindex eba1255..f9c63e9 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -475,14 +475,6 @@ static void make_service_overridable(const char *name, int ena)\n \tdie(\"No such service %s\", name);\n }\n \n-static char *xstrdup_tolower(const char *str)\n-{\n-\tchar *p, *dup = xstrdup(str);\n-\tfor (p = dup; *p; p++)\n-\t\t*p = tolower(*p);\n-\treturn dup;\n-}\n-\n static void parse_host_and_port(char *hostport, char **host,\n \tchar **port)\n {\ndiff --git a/strbuf.c b/strbuf.c\nindex ee96dcf..854c725 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -570,3 +570,16 @@ int fprintf_ln(FILE *fp, const char *fmt, ...)\n \t\treturn -1;\n \treturn ret + 1;\n }\n+\n+char *xstrdup_tolower(const char *string)\n+{\n+\tchar *result;\n+\tsize_t len, i;\n+\n+\tlen = strlen(string);\n+\tresult = xmalloc(len + 1);\n+\tfor (i = 0; i < len; i++)\n+\t\tresult[i] = tolower(string[i]);\n+\tresult[i] = '\\0';\n+\treturn result;\n+}\ndiff --git a/strbuf.h b/strbuf.h\nindex 39c14cf..4de7531 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -183,4 +183,6 @@ extern int printf_ln(const char *fmt, ...);\n __attribute__((format (printf,2,3)))\n extern int fprintf_ln(FILE *fp, const char *fmt, ...);\n \n+char *xstrdup_tolower(const char *);\n+\n #endif /* STRBUF_H */\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242477","messageId":"20140522094424.GB15255@sigill.intra.peff.net","threadId":"36729","inReplyTo":"20140522094251.GA14994@sigill.intra.peff.net","subject":"[PATCH 2/2] strbuf: add strbuf_tolower function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:44:24Z","receivedAt":"2014-05-22T09:44:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This makes config's lowercase() function public.\n\nNote that we could continue to offer a pure-string\nlowercase, but there would be no callers (in most\npure-string cases, we actually duplicate and lowercase the\nduplicate).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/technical/api-strbuf.txt | 4 ++++\n config.c                               | 8 +-------\n strbuf.c                               | 7 +++++++\n strbuf.h                               | 1 +\n 4 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\nindex 3350d97..8480f89 100644\n--- a/Documentation/technical/api-strbuf.txt\n+++ b/Documentation/technical/api-strbuf.txt\n@@ -125,6 +125,10 @@ Functions\n \n \tStrip whitespace from the end of a string.\n \n+`strbuf_tolower`::\n+\n+\tLowercase each character in the buffer using `tolower`.\n+\n `strbuf_cmp`::\n \n \tCompare two buffers. Returns an integer less than, equal to, or greater\ndiff --git a/config.c b/config.c\nindex a30cb5c..03ce5c6 100644\n--- a/config.c\n+++ b/config.c\n@@ -147,12 +147,6 @@ int git_config_include(const char *var, const char *value, void *data)\n \treturn ret;\n }\n \n-static void lowercase(char *p)\n-{\n-\tfor (; *p; p++)\n-\t\t*p = tolower(*p);\n-}\n-\n void git_config_push_parameter(const char *text)\n {\n \tstruct strbuf env = STRBUF_INIT;\n@@ -180,7 +174,7 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tlowercase(pair[0]->buf);\n+\tstrbuf_tolower(pair[0]);\n \tif (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\ndiff --git a/strbuf.c b/strbuf.c\nindex 854c725..3da4f3e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb)\n \tsb->buf[sb->len] = '\\0';\n }\n \n+void strbuf_tolower(struct strbuf *sb)\n+{\n+\tchar *p;\n+\tfor (p = sb->buf; *p; p++)\n+\t\t*p = tolower(*p);\n+}\n+\n struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \t\t\t\t int terminator, int max)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex 4de7531..25328b9 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n extern void strbuf_trim(struct strbuf *);\n extern void strbuf_rtrim(struct strbuf *);\n extern void strbuf_ltrim(struct strbuf *);\n+extern void strbuf_tolower(struct strbuf *sb);\n extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n \n /*\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242483","messageId":"20140522134253.GA16408@sigill.intra.peff.net","threadId":"36729","inReplyTo":"CAP8UFD0PkA_VokFpRkdaKE2UW1AtL6mCY2bOSxOCqX_C9wB=OQ@mail.gmail.com","subject":"Re: [PATCH 2/2] strbuf: add strbuf_tolower function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T13:42:54Z","receivedAt":"2014-05-22T13:42:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[re-adding list cc]\n\nOn Thu, May 22, 2014 at 03:16:45PM +0200, Christian Couder wrote:\n\n> > +void strbuf_tolower(struct strbuf *sb)\n> > +{\n> > +       char *p;\n> > +       for (p = sb->buf; *p; p++)\n> > +               *p = tolower(*p);\n> > +}\n> \n> Last time I tried a change like the above, I was told that strbufs are\n> buffers that can contain NUL bytes. So maybe it should be:\n> \n>        for (p = sb->buf; p < sb->buf + sb->len; p++)\n>               *p = tolower(*p);\n\nHah. I wrote it like that originally, and in review was asked to switch\nit. I agree that it might be worth keeping strbuf functions 8bit-clean.\nThe original intent of the strbuf code was to make C strings easier to\nuse, but I think we sometimes use them as general buffers, too.\n\n-Peff\n"},{"id":"242542","messageId":"4831E838-1373-42F6-B490-2D2943E319C9@gmail.com","threadId":"36729","inReplyTo":"20140522134253.GA16408@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] strbuf: add strbuf_tolower function","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-05-22T23:17:59Z","receivedAt":"2014-05-22T23:17:59Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On May 22, 2014, at 06:42, Jeff King wrote:\n\n> [re-adding list cc]\n>\n> On Thu, May 22, 2014 at 03:16:45PM +0200, Christian Couder wrote:\n>\n>>> +void strbuf_tolower(struct strbuf *sb)\n>>> +{\n>>> +       char *p;\n>>> +       for (p = sb->buf; *p; p++)\n>>> +               *p = tolower(*p);\n>>> +}\n>>\n>> Last time I tried a change like the above, I was told that strbufs  \n>> are\n>> buffers that can contain NUL bytes. So maybe it should be:\n>>\n>>       for (p = sb->buf; p < sb->buf + sb->len; p++)\n>>              *p = tolower(*p);\n>\n> Hah. I wrote it like that originally, and in review was asked to  \n> switch\n> it. I agree that it might be worth keeping strbuf functions 8bit- \n> clean.\n> The original intent of the strbuf code was to make C strings easier to\n> use, but I think we sometimes use them as general buffers, too.\n\n\nI didn't see this patch before I sent the other email, but this is the  \nrelevant part:\n\nOn May 22, 2014, at 15:52, Kyle J. McKay wrote:\n> The only reason I brought up the code difference in the first place  \n> was that the comment was \"This makes config's lowercase() function  \n> public\" which made me expect to see basically the equivalent of  \n> replacing a \"static\" with an \"extern\", but actually the function  \n> being made public was a different implementation than config's  \n> lowercase() function.  So it looks like the original PATCH 2/9  \n> version of the code is best, but perhaps the comment can be tweaked  \n> a bit to not convey the same impression I got.  Maybe something like  \n> \"Replace config's lowercase() function with a public one\".\n\n\nOr even just delete the \"This makes config's lowercase() function  \npublic\" line after switching the implementation back.\n\n--Kyle\n"}]}