[PATCH 0/2] tolower cleanups

[PATCH 0/2] tolower cleanups

[Jeff King]

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

[PATCH 1/2] daemon/config: factor out duplicate xstrdup_tolower

[Jeff King]

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(-)

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

[PATCH 2/2] strbuf: add strbuf_tolower function

[Jeff King]

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(-)

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

Re: [PATCH 2/2] strbuf: add strbuf_tolower function

[Jeff King]

[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

Re: [PATCH 2/2] strbuf: add strbuf_tolower function

[Kyle J. McKay]

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