Jeff KingThese two patches were pulled from the http charset series I posted nearby. The second iteration of that series did not need them, but they may have value as cleanups.
[1/2]: daemon/config: factor out duplicate xstrdup_tolower [2/2]: strbuf: add strbuf_tolower function
The first one is a real reduction of code. The second is just making code public that _might_ get used later, so it's a bit less exciting. Both are independent of each other, so we can take or leave them separately.
Jeff King[PATCH 1/2] daemon/config: factor out duplicate xstrdup_tolower
We have two implementations of the same function; let's drop that to one. We take the name from daemon.c, but the implementation (which is just slightly more efficient) from the config code.
Signed-off-by: Jeff King <peff@peff.net> --- builtin/config.c | 15 +-------------- daemon.c | 8 -------- strbuf.c | 13 +++++++++++++ strbuf.h | 2 ++ 4 files changed, 16 insertions(+), 22 deletions(-)
builtin/config.c, daemon.c, strbuf.c, strbuf.h
diff --git a/builtin/config.c b/builtin/config.c index 5677c94..fcd8474 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -395,19 +395,6 @@ static int urlmatch_collect_fn(const char *var, const char *value, void *cb) return 0; } -static char *dup_downcase(const char *string) -{ - char *result; - size_t len, i; - - len = strlen(string); - result = xmalloc(len + 1); - for (i = 0; i < len; i++) - result[i] = tolower(string[i]); - result[i] = '\0'; - return result; -} - static int get_urlmatch(const char *var, const char *url) { char *section_tail; @@ -422,7 +409,7 @@ static int get_urlmatch(const char *var, const char *url) if (!url_normalize(url, &config.url)) die("%s", config.url.err); - config.section = dup_downcase(var); + config.section = xstrdup_tolower(var); section_tail = strchr(config.section, '.'); if (section_tail) { *section_tail = '\0'; diff --git a/daemon.c b/daemon.c index eba1255..f9c63e9 100644 --- a/daemon.c +++ b/daemon.c @@ -475,14 +475,6 @@ static void make_service_overridable(const char *name, int ena) die("No such service %s", name); } -static char *xstrdup_tolower(const char *str) -{ - char *p, *dup = xstrdup(str); - for (p = dup; *p; p++) - *p = tolower(*p); - return dup; -} - static void parse_host_and_port(char *hostport, char **host, char **port) { diff --git a/strbuf.c b/strbuf.c index ee96dcf..854c725 100644 --- a/strbuf.c +++ b/strbuf.c @@ -570,3 +570,16 @@ int fprintf_ln(FILE *fp, const char *fmt, ...) return -1; return ret + 1; } + +char *xstrdup_tolower(const char *string) +{ + char *result; + size_t len, i; + + len = strlen(string); + result = xmalloc(len + 1); + for (i = 0; i < len; i++) + result[i] = tolower(string[i]); + result[i] = '\0'; + return result; +} diff --git a/strbuf.h b/strbuf.h index 39c14cf..4de7531 100644 --- a/strbuf.h +++ b/strbuf.h @@ -183,4 +183,6 @@ extern int printf_ln(const char *fmt, ...); __attribute__((format (printf,2,3))) extern int fprintf_ln(FILE *fp, const char *fmt, ...); +char *xstrdup_tolower(const char *); + #endif /* STRBUF_H */
-- 2.0.0.rc1.436.g03cb729
Jeff King[PATCH 2/2] strbuf: add strbuf_tolower function
This makes config's lowercase() function public.
Note that we could continue to offer a pure-string lowercase, but there would be no callers (in most pure-string cases, we actually duplicate and lowercase the duplicate).
Signed-off-by: Jeff King <peff@peff.net> --- Documentation/technical/api-strbuf.txt | 4 ++++ config.c | 8 +------- strbuf.c | 7 +++++++ strbuf.h | 1 + 4 files changed, 13 insertions(+), 7 deletions(-)
Documentation/technical/api-strbuf.txt, config.c, strbuf.c, strbuf.h
diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt index 3350d97..8480f89 100644 --- a/Documentation/technical/api-strbuf.txt +++ b/Documentation/technical/api-strbuf.txt @@ -125,6 +125,10 @@ Functions Strip whitespace from the end of a string. +`strbuf_tolower`:: + + Lowercase each character in the buffer using `tolower`. + `strbuf_cmp`:: Compare two buffers. Returns an integer less than, equal to, or greater diff --git a/config.c b/config.c index a30cb5c..03ce5c6 100644 --- a/config.c +++ b/config.c @@ -147,12 +147,6 @@ int git_config_include(const char *var, const char *value, void *data) return ret; } -static void lowercase(char *p) -{ - for (; *p; p++) - *p = tolower(*p); -} - void git_config_push_parameter(const char *text) { struct strbuf env = STRBUF_INIT; @@ -180,7 +174,7 @@ int git_config_parse_parameter(const char *text, strbuf_list_free(pair); return error("bogus config parameter: %s", text); } - lowercase(pair[0]->buf); + strbuf_tolower(pair[0]); if (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) { strbuf_list_free(pair); return -1; diff --git a/strbuf.c b/strbuf.c index 854c725..3da4f3e 100644 --- a/strbuf.c +++ b/strbuf.c @@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb) sb->buf[sb->len] = '\0'; } +void strbuf_tolower(struct strbuf *sb) +{ + char *p; + for (p = sb->buf; *p; p++) + *p = tolower(*p); +} + struct strbuf **strbuf_split_buf(const char *str, size_t slen, int terminator, int max) { diff --git a/strbuf.h b/strbuf.h index 4de7531..25328b9 100644 --- a/strbuf.h +++ b/strbuf.h @@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) extern void strbuf_trim(struct strbuf *); extern void strbuf_rtrim(struct strbuf *); extern void strbuf_ltrim(struct strbuf *); +extern void strbuf_tolower(struct strbuf *sb); extern int strbuf_cmp(const struct strbuf *, const struct strbuf *); /*
-- 2.0.0.rc1.436.g03cb729
Jeff KingRe: [PATCH 2/2] strbuf: add strbuf_tolower function
[re-adding list cc]
On Thu, May 22, 2014 at 03:16:45PM +0200, Christian Couder wrote:
> > +void strbuf_tolower(struct strbuf *sb)
> > +{
> > + char *p;
> > + for (p = sb->buf; *p; p++)
> > + *p = tolower(*p);
> > +}
>
> Last time I tried a change like the above, I was told that strbufs are
> buffers that can contain NUL bytes. So maybe it should be:
>
> for (p = sb->buf; p < sb->buf + sb->len; p++)
> *p = tolower(*p);Hah. I wrote it like that originally, and in review was asked to switch it. I agree that it might be worth keeping strbuf functions 8bit-clean. The original intent of the strbuf code was to make C strings easier to use, but I think we sometimes use them as general buffers, too.
-Peff
Kyle J. McKayRe: [PATCH 2/2] strbuf: add strbuf_tolower function
On May 22, 2014, at 06:42, Jeff King wrote:
> [re-adding list cc]
>
> On Thu, May 22, 2014 at 03:16:45PM +0200, Christian Couder wrote:
>
>>> +void strbuf_tolower(struct strbuf *sb)
>>> +{
>>> + char *p;
>>> + for (p = sb->buf; *p; p++)
>>> + *p = tolower(*p);
>>> +}
>>
>> Last time I tried a change like the above, I was told that strbufs
>> are
>> buffers that can contain NUL bytes. So maybe it should be:
>>
>> for (p = sb->buf; p < sb->buf + sb->len; p++)
>> *p = tolower(*p);
>
> Hah. I wrote it like that originally, and in review was asked to
> switch
> it. I agree that it might be worth keeping strbuf functions 8bit-
> clean.
> The original intent of the strbuf code was to make C strings easier to
> use, but I think we sometimes use them as general buffers, too.I didn't see this patch before I sent the other email, but this is the relevant part:
On May 22, 2014, at 15:52, Kyle J. McKay wrote:
> The only reason I brought up the code difference in the first place > was that the comment was "This makes config's lowercase() function > public" which made me expect to see basically the equivalent of > replacing a "static" with an "extern", but actually the function > being made public was a different implementation than config's > lowercase() function. So it looks like the original PATCH 2/9 > version of the code is best, but perhaps the comment can be tweaked > a bit to not convey the same impression I got. Maybe something like > "Replace config's lowercase() function with a public one".
Or even just delete the "This makes config's lowercase() function public" line after switching the implementation back.
--Kyle