[PATCH 1/2] Add strchrnul()

[PATCH 1/2] Add strchrnul()

[René Scharfe]

As suggested by Pierre Habouzit, add strchrnul().  It's a useful GNU
extension and can simplify string parser code.  There are several
places in git that can be converted to strchrnul(); as a trivial
example, this patch introduces its usage to builtin-fetch--tool.c.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---

 Makefile              |   13 +++++++++++++
 builtin-fetch--tool.c |    8 ++------
 compat/strchrnul.c    |    8 ++++++++
 git-compat-util.h     |    5 +++++
 4 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 0d5590f..578c999 100644
--- a/Makefile
+++ b/Makefile
@@ -30,6 +30,8 @@ all::
 #
 # Define NO_MEMMEM if you don't have memmem.
 #
+# Define NO_STRCHRNUL if you don't have strchrnul.
+#
 # Define NO_STRLCPY if you don't have strlcpy.
 #
 # Define NO_STRTOUMAX if you don't have strtoumax in the C library.
@@ -406,6 +408,7 @@ ifeq ($(uname_S),Darwin)
 	OLD_ICONV = UnfortunatelyYes
 	NO_STRLCPY = YesPlease
 	NO_MEMMEM = YesPlease
+	NO_STRCHRNUL = YesPlease
 endif
 ifeq ($(uname_S),SunOS)
 	NEEDS_SOCKET = YesPlease
@@ -413,6 +416,7 @@ ifeq ($(uname_S),SunOS)
 	SHELL_PATH = /bin/bash
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
+	NO_STRCHRNUL = YesPlease
 	NO_HSTRERROR = YesPlease
 	ifeq ($(uname_R),5.8)
 		NEEDS_LIBICONV = YesPlease
@@ -438,6 +442,7 @@ ifeq ($(uname_O),Cygwin)
 	NO_D_INO_IN_DIRENT = YesPlease
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
+	NO_STRCHRNUL = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
@@ -452,12 +457,14 @@ endif
 ifeq ($(uname_S),FreeBSD)
 	NEEDS_LIBICONV = YesPlease
 	NO_MEMMEM = YesPlease
+	NO_STRCHRNUL = YesPlease
 	BASIC_CFLAGS += -I/usr/local/include
 	BASIC_LDFLAGS += -L/usr/local/lib
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
+	NO_STRCHRNUL = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	BASIC_CFLAGS += -I/usr/local/include
 	BASIC_LDFLAGS += -L/usr/local/lib
@@ -473,6 +480,7 @@ endif
 ifeq ($(uname_S),AIX)
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
+	NO_STRCHRNUL = YesPlease
 	NO_STRLCPY = YesPlease
 	NEEDS_LIBICONV=YesPlease
 endif
@@ -485,6 +493,7 @@ ifeq ($(uname_S),IRIX64)
 	NO_SETENV=YesPlease
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
+	NO_STRCHRNUL = YesPlease
 	NO_STRLCPY = YesPlease
 	NO_SOCKADDR_STORAGE=YesPlease
 	SHELL_PATH=/usr/gnu/bin/bash
@@ -695,6 +704,10 @@ ifdef NO_MEMMEM
 	COMPAT_CFLAGS += -DNO_MEMMEM
 	COMPAT_OBJS += compat/memmem.o
 endif
+ifdef NO_STRCHRNUL
+	COMPAT_CFLAGS += -DNO_STRCHRNUL
+	COMPAT_OBJS += compat/strchrnul.o
+endif
 
 ifdef THREADED_DELTA_SEARCH
 	BASIC_CFLAGS += -DTHREADED_DELTA_SEARCH
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 6a78517..ed60847 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -435,9 +435,7 @@ static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_resu
 				cp++;
 			if (!*cp)
 				break;
-			np = strchr(cp, '\n');
-			if (!np)
-				np = cp + strlen(cp);
+			np = strchrnul(cp, '\n');
 			if (pass) {
 				lrr_list[i].line = cp;
 				lrr_list[i].name = cp + 41;
@@ -461,9 +459,7 @@ static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_resu
 			rref++;
 		if (!*rref)
 			break;
-		next = strchr(rref, '\n');
-		if (!next)
-			next = rref + strlen(rref);
+		next = strchrnul(rref, '\n');
 		rreflen = next - rref;
 
 		for (i = 0; i < lrr_count; i++) {
diff --git a/compat/strchrnul.c b/compat/strchrnul.c
new file mode 100644
index 0000000..51839fe
--- /dev/null
+++ b/compat/strchrnul.c
@@ -0,0 +1,8 @@
+#include "../git-compat-util.h"
+
+char *gitstrchrnul(const char *s, int c)
+{
+	while (*s && *s != c)
+		s++;
+	return (char *)s;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 7b29d1b..e72654b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -183,6 +183,11 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
                 const void *needle, size_t needlelen);
 #endif
 
+#ifdef NO_STRCHRNUL
+#define strchrnul gitstrchrnul
+char *gitstrchrnul(const char *s, int c);
+#endif
+
 extern void release_pack_memory(size_t, int);
 
 static inline char* xstrdup(const char *str)

Re: [PATCH 1/2] Add strchrnul()

[Johannes Schindelin]

Hi,

On Fri, 9 Nov 2007, Ren? Scharfe wrote:

> diff --git a/Makefile b/Makefile
> index 0d5590f..578c999 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -30,6 +30,8 @@ all::
>  #
>  # Define NO_MEMMEM if you don't have memmem.
>  #
> +# Define NO_STRCHRNUL if you don't have strchrnul.
> +#
>  # Define NO_STRLCPY if you don't have strlcpy.
>  #
>  # Define NO_STRTOUMAX if you don't have strtoumax in the C library.
> @@ -406,6 +408,7 @@ ifeq ($(uname_S),Darwin)
>  	OLD_ICONV = UnfortunatelyYes
>  	NO_STRLCPY = YesPlease
>  	NO_MEMMEM = YesPlease
> +	NO_STRCHRNUL = YesPlease
>  endif
>  ifeq ($(uname_S),SunOS)
>  	NEEDS_SOCKET = YesPlease
> @@ -413,6 +416,7 @@ ifeq ($(uname_S),SunOS)
>  	SHELL_PATH = /bin/bash
>  	NO_STRCASESTR = YesPlease
>  	NO_MEMMEM = YesPlease
> +	NO_STRCHRNUL = YesPlease
>  	NO_HSTRERROR = YesPlease
>  	ifeq ($(uname_R),5.8)
>  		NEEDS_LIBICONV = YesPlease
> @@ -438,6 +442,7 @@ ifeq ($(uname_O),Cygwin)
>  	NO_D_INO_IN_DIRENT = YesPlease
>  	NO_STRCASESTR = YesPlease
>  	NO_MEMMEM = YesPlease
> +	NO_STRCHRNUL = YesPlease
>  	NO_SYMLINK_HEAD = YesPlease
>  	NEEDS_LIBICONV = YesPlease
>  	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
> @@ -452,12 +457,14 @@ endif
>  ifeq ($(uname_S),FreeBSD)
>  	NEEDS_LIBICONV = YesPlease
>  	NO_MEMMEM = YesPlease
> +	NO_STRCHRNUL = YesPlease
>  	BASIC_CFLAGS += -I/usr/local/include
>  	BASIC_LDFLAGS += -L/usr/local/lib
>  endif
>  ifeq ($(uname_S),OpenBSD)
>  	NO_STRCASESTR = YesPlease
>  	NO_MEMMEM = YesPlease
> +	NO_STRCHRNUL = YesPlease
>  	NEEDS_LIBICONV = YesPlease
>  	BASIC_CFLAGS += -I/usr/local/include
>  	BASIC_LDFLAGS += -L/usr/local/lib
> @@ -473,6 +480,7 @@ endif
>  ifeq ($(uname_S),AIX)
>  	NO_STRCASESTR=YesPlease
>  	NO_MEMMEM = YesPlease
> +	NO_STRCHRNUL = YesPlease
>  	NO_STRLCPY = YesPlease
>  	NEEDS_LIBICONV=YesPlease
>  endif
> @@ -485,6 +493,7 @@ ifeq ($(uname_S),IRIX64)
>  	NO_SETENV=YesPlease
>  	NO_STRCASESTR=YesPlease
>  	NO_MEMMEM = YesPlease
> +	NO_STRCHRNUL = YesPlease
>  	NO_STRLCPY = YesPlease
>  	NO_SOCKADDR_STORAGE=YesPlease
>  	SHELL_PATH=/usr/gnu/bin/bash

Might be easier to define HAVE_STRCHRNUL ;-)

> diff --git a/compat/strchrnul.c b/compat/strchrnul.c
> new file mode 100644
> index 0000000..51839fe
> --- /dev/null
> +++ b/compat/strchrnul.c
> @@ -0,0 +1,8 @@
> +#include "../git-compat-util.h"
> +
> +char *gitstrchrnul(const char *s, int c)
> +{
> +	while (*s && *s != c)
> +		s++;
> +	return (char *)s;
> +}

This is so short, I think it is even better to inline it.

Ciao,
Dscho

Re: [PATCH 1/2] Add strchrnul()

[Jeff King]

On Fri, Nov 09, 2007 at 01:49:36AM +0100, René Scharfe wrote:

> As suggested by Pierre Habouzit, add strchrnul().  It's a useful GNU
> extension and can simplify string parser code.  There are several
> places in git that can be converted to strchrnul(); as a trivial
> example, this patch introduces its usage to builtin-fetch--tool.c.

Minor nit: a comment (in the code or even in the commit message)
describing the what the function does would be nice. On a GNU system,
one can look at the man page, but on others there is no indication of
what it does except for reading the code (and maybe the answer is "this
is a common library function, you idiot", but I am familiar with many
GNU extensions and didn't know this one).

-Peff

Re: [PATCH 1/2] Add strchrnul()

[Andreas Ericsson]

René Scharfe wrote:
> As suggested by Pierre Habouzit, add strchrnul().  It's a useful GNU
> extension and can simplify string parser code.  There are several
> places in git that can be converted to strchrnul(); as a trivial
> example, this patch introduces its usage to builtin-fetch--tool.c.
> 
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
> 
>  Makefile              |   13 +++++++++++++
>  builtin-fetch--tool.c |    8 ++------
>  compat/strchrnul.c    |    8 ++++++++
>  git-compat-util.h     |    5 +++++
>  4 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 0d5590f..578c999 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -30,6 +30,8 @@ all::
>  #
>  # Define NO_MEMMEM if you don't have memmem.
>  #
> +# Define NO_STRCHRNUL if you don't have strchrnul.
> +#


This seems overly complicated. How about this instead?

From: Andreas Ericsson <ae@op5.se>
Subject: [PATCH] Add strchrnul()

As suggested by Pierre Habouzit, add strchrnul().  It's a useful GNU
extension and can simplify string parser code.  There are several
places in git that can be converted to strchrnul(); as a trivial
example, this patch introduces its usage to builtin-fetch--tool.c.

strchrnul() was introduced in glibc in April 1999 and included in
glibc-2.1. Checking for that version means the majority of all git
users would get to use the optimized version in glibc. Of the
remaining few some might get to use a slightly slower version
than necessary but probably not slower than what we have today.

Original patch by Rene Scharfe <rene.scharfe@lsrfire.ath.cx>

Signed-off-by: Andreas Ericsson <ae@op5.se>
---

I'm fairly much against forcing people to know what library
functions they have in order to get software to compile
properly. This is, imo, a neater solution, and also inlines
the function as suggested by Dscho.

diff --git a/git-compat-util.h b/git-compat-util.h
index 7b29d1b..9fedf33 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -105,6 +105,18 @@ extern void set_die_routine(void (*routine)(const char *err
 extern void set_error_routine(void (*routine)(const char *err, va_list params))
 extern void set_warn_routine(void (*routine)(const char *warn, va_list params))
 
+
+#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
+# define strchrnul(s, c) gitstrchrnul(s, c)
+static inline char *gitstrchrnul(const char *s, int c)
+{
+       while (*s && *s != c)
+               s++;
+
+       return (char *)s;
+}
+#endif
+
 #ifdef NO_MMAP
 
 #ifndef PROT_READ
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 6a78517..ed60847 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -435,9 +435,7 @@ static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_resu
 				cp++;
 			if (!*cp)
 				break;
-			np = strchr(cp, '\n');
-			if (!np)
-				np = cp + strlen(cp);
+			np = strchrnul(cp, '\n');
 			if (pass) {
 				lrr_list[i].line = cp;
 				lrr_list[i].name = cp + 41;
@@ -461,9 +459,7 @@ static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_resu
 			rref++;
 		if (!*rref)
 			break;
-		next = strchr(rref, '\n');
-		if (!next)
-			next = rref + strlen(rref);
+		next = strchrnul(rref, '\n');
 		rreflen = next - rref;
 
 		for (i = 0; i < lrr_count; i++) {
-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 1/2] Add strchrnul()

[Jakub Narebski]

[Cc: Andreas Ericsson <ae@op5.se>, 
     René Scharfe <rene.scharfe@lsrfire.ath.cx>, 
     Junio C Hamano <gitster@pobox.com>,
     git@vger.kernel.org]

Andreas Ericsson wrote:

> René Scharfe wrote:
>> As suggested by Pierre Habouzit, add strchrnul().  It's a useful GNU
>> extension and can simplify string parser code.  There are several
>> places in git that can be converted to strchrnul(); as a trivial
>> example, this patch introduces its usage to builtin-fetch--tool.c.
>> 
>> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
>> ---
>> 
>>  Makefile              |   13 +++++++++++++
>>  builtin-fetch--tool.c |    8 ++------
>>  compat/strchrnul.c    |    8 ++++++++
>>  git-compat-util.h     |    5 +++++
>>  4 files changed, 28 insertions(+), 6 deletions(-)
>> 
>> diff --git a/Makefile b/Makefile
>> index 0d5590f..578c999 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -30,6 +30,8 @@ all::
>>  #
>>  # Define NO_MEMMEM if you don't have memmem.
>>  #
>> +# Define NO_STRCHRNUL if you don't have strchrnul.
>> +#

Original patch lacked adding appropriate test to configure,
i.e. something like below to configure.ac

 #
 # Define NO_STRCHRNUL if you don't have strchrnul.
 AC_CHECK_FUNC(strchrnul,
 [NO_STRCHRNUL=],
 [NO_STRCHRNUL=YesPlease])
 AC_SUBST(NO_STRCHRNUL)
 
and the following line to config.mak.in

 NO_STRCHRNUL=@NO_STRCHRNUL@

> This seems overly complicated. How about this instead?
[...]
> I'm fairly much against forcing people to know what library
> functions they have in order to get software to compile
> properly. This is, imo, a neater solution, and also inlines
> the function as suggested by Dscho.

Wouldn't it be better to add ./configure check instead? See above.

Although I guess that people using ./configure to set compilation
options (to generate config.mak.autogen) are minority...

> +#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
> +# define strchrnul(s, c) gitstrchrnul(s, c)
> +static inline char *gitstrchrnul(const char *s, int c)
> +{
> +       while (*s && *s != c)
> +               s++;
> +
> +       return (char *)s;
> +}
> +#endif
> +

This is good solution, although I'm not sure about the check itself.
What if somebody has libc which is not glibc, but it does have
strchrnul?

> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
> index 6a78517..ed60847 100644
> --- a/builtin-fetch--tool.c
> +++ b/builtin-fetch--tool.c
> @@ -435,9 +435,7 @@ static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_resu
>                               cp++;
>                       if (!*cp)
>                               break;
> -                     np = strchr(cp, '\n');
> -                     if (!np)
> -                             np = cp + strlen(cp);
> +                     np = strchrnul(cp, '\n');
>                       if (pass) {
>                               lrr_list[i].line = cp;
>                               lrr_list[i].name = cp + 41;
> @@ -461,9 +459,7 @@ static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_resu
>                       rref++;
>               if (!*rref)
>                       break;
> -             next = strchr(rref, '\n');
> -             if (!next)
> -                     next = rref + strlen(rref);
> +             next = strchrnul(rref, '\n');
>               rreflen = next - rref;
>  
>               for (i = 0; i < lrr_count; i++) {

This IMHO should go to separate patch.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 1/2] Add strchrnul()

[Andreas Ericsson]

Jakub Narebski wrote:
> [Cc: Andreas Ericsson <ae@op5.se>, 
>      René Scharfe <rene.scharfe@lsrfire.ath.cx>, 
>      Junio C Hamano <gitster@pobox.com>,
>      git@vger.kernel.org]
> 

I'm not sure what's up with this, but I didn't see this email on git@vger,
so re-adding it to the Cc list now.

> 
> Original patch lacked adding appropriate test to configure,
> i.e. something like below to configure.ac
> 
>  #
>  # Define NO_STRCHRNUL if you don't have strchrnul.
>  AC_CHECK_FUNC(strchrnul,
>  [NO_STRCHRNUL=],
>  [NO_STRCHRNUL=YesPlease])
>  AC_SUBST(NO_STRCHRNUL)
>  
> and the following line to config.mak.in
> 
>  NO_STRCHRNUL=@NO_STRCHRNUL@
> 
>> This seems overly complicated. How about this instead?
> [...]
>> I'm fairly much against forcing people to know what library
>> functions they have in order to get software to compile
>> properly. This is, imo, a neater solution, and also inlines
>> the function as suggested by Dscho.
> 
> Wouldn't it be better to add ./configure check instead? See above.
> 
> Although I guess that people using ./configure to set compilation
> options (to generate config.mak.autogen) are minority...
> 

Perhaps. I know I don't anyway, and now it's become standard not to
do so for a significant part of the git-tracking world.

>> +#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
>> +# define strchrnul(s, c) gitstrchrnul(s, c)
>> +static inline char *gitstrchrnul(const char *s, int c)
>> +{
>> +       while (*s && *s != c)
>> +               s++;
>> +
>> +       return (char *)s;
>> +}
>> +#endif
>> +
> 
> This is good solution, although I'm not sure about the check itself.
> What if somebody has libc which is not glibc, but it does have
> strchrnul?
> 

They most likely fall into the minority that get to suffer from
using code that's as fast as what's in there today, although
a bit more readable. The glibc optimization is really only
worthwhile for architectures where strchrnul()-like operations
have microcode support, or when it's used on large strings.

YMMV. I suppose rewriting it as

#if defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 1)
# define HAVE_STRCHRNUL
#endif

#ifdef HAVE_STRCHRNUL
...

would work too, and will provide an easier way out for other fellas
wanting to say "Hey, my favourite solaris libc has this too!". OTOH,
that rewrite can be done when the first such case appears.


>> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
> 
> This IMHO should go to separate patch.
> 

*shrug* Rene had it in his. Monkey see monkey do ;-)

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 1/2] Add strchrnul()

[Jakub Narebski]

Andreas Ericsson wrote:
> Jakub Narebski wrote:
>> René Scharfe wrote:

>>> +#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
>>> +# define strchrnul(s, c) gitstrchrnul(s, c)
>>> +static inline char *gitstrchrnul(const char *s, int c)
>>> +{
>>> +       while (*s && *s != c)
>>> +               s++;
>>> +
>>> +       return (char *)s;
>>> +}
>>> +#endif
>>> +
>> 
>> This is good solution, although I'm not sure about the check itself.
>> What if somebody has libc which is not glibc, but it does have
>> strchrnul? 
> 
> They most likely fall into the minority that get to suffer from
> using code that's as fast as what's in there today, although
> a bit more readable. The glibc optimization is really only
> worthwhile for architectures where strchrnul()-like operations
> have microcode support, or when it's used on large strings.

If we end up using this solution, then adding test for strchrnul in the 
configure (configure.ac and config.mak.in) is not needed...

> YMMV. I suppose rewriting it as
> 
> #if defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 1)

#if !defined(HAVE_STRCHRNUL) && \
     defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 1)

> # define HAVE_STRCHRNUL
> #endif
> 
> #ifdef HAVE_STRCHRNUL
> ...
> 
> would work too, and will provide an easier way out for other fellas
> wanting to say "Hey, my favourite solaris libc has this too!". OTOH,
> that rewrite can be done when the first such case appears.

...but if we end up using this version (be it HAVE_STRCHRNUL, or 
NO_STRCHRNUL), then test for strchrnul in ./configure is I think
necessary.

-- 
Jakub Narebski
Poland

[PATCH] Simplify strchrnul() compat code

[René Scharfe]

From: Andreas Ericsson <ae@op5.se>

strchrnul() was introduced in glibc in April 1999 and included in
glibc-2.1. Checking for that version means the majority of all git
users would get to use the optimized version in glibc. Of the
remaining few some might get to use a slightly slower version
than necessary but probably not slower than what we have today.

Rediffed-against-next-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
I agree that we can do without providing a way to use a native
version outside glibc until we actually encounter such a thing.

 Makefile           |   13 -------------
 compat/strchrnul.c |    8 --------
 git-compat-util.h  |    9 +++++++--
 3 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/Makefile b/Makefile
index 4f1d7ca..4a5d2b9 100644
--- a/Makefile
+++ b/Makefile
@@ -30,8 +30,6 @@ all::
 #
 # Define NO_MEMMEM if you don't have memmem.
 #
-# Define NO_STRCHRNUL if you don't have strchrnul.
-#
 # Define NO_STRLCPY if you don't have strlcpy.
 #
 # Define NO_STRTOUMAX if you don't have strtoumax in the C library.
@@ -409,7 +407,6 @@ ifeq ($(uname_S),Darwin)
 	OLD_ICONV = UnfortunatelyYes
 	NO_STRLCPY = YesPlease
 	NO_MEMMEM = YesPlease
-	NO_STRCHRNUL = YesPlease
 endif
 ifeq ($(uname_S),SunOS)
 	NEEDS_SOCKET = YesPlease
@@ -417,7 +414,6 @@ ifeq ($(uname_S),SunOS)
 	SHELL_PATH = /bin/bash
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
-	NO_STRCHRNUL = YesPlease
 	NO_HSTRERROR = YesPlease
 	ifeq ($(uname_R),5.8)
 		NEEDS_LIBICONV = YesPlease
@@ -443,7 +439,6 @@ ifeq ($(uname_O),Cygwin)
 	NO_D_INO_IN_DIRENT = YesPlease
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
-	NO_STRCHRNUL = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
@@ -458,14 +453,12 @@ endif
 ifeq ($(uname_S),FreeBSD)
 	NEEDS_LIBICONV = YesPlease
 	NO_MEMMEM = YesPlease
-	NO_STRCHRNUL = YesPlease
 	BASIC_CFLAGS += -I/usr/local/include
 	BASIC_LDFLAGS += -L/usr/local/lib
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
-	NO_STRCHRNUL = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	BASIC_CFLAGS += -I/usr/local/include
 	BASIC_LDFLAGS += -L/usr/local/lib
@@ -481,7 +474,6 @@ endif
 ifeq ($(uname_S),AIX)
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
-	NO_STRCHRNUL = YesPlease
 	NO_STRLCPY = YesPlease
 	NEEDS_LIBICONV=YesPlease
 endif
@@ -494,7 +486,6 @@ ifeq ($(uname_S),IRIX64)
 	NO_SETENV=YesPlease
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
-	NO_STRCHRNUL = YesPlease
 	NO_STRLCPY = YesPlease
 	NO_SOCKADDR_STORAGE=YesPlease
 	SHELL_PATH=/usr/gnu/bin/bash
@@ -705,10 +696,6 @@ ifdef NO_MEMMEM
 	COMPAT_CFLAGS += -DNO_MEMMEM
 	COMPAT_OBJS += compat/memmem.o
 endif
-ifdef NO_STRCHRNUL
-	COMPAT_CFLAGS += -DNO_STRCHRNUL
-	COMPAT_OBJS += compat/strchrnul.o
-endif
 
 ifdef THREADED_DELTA_SEARCH
 	BASIC_CFLAGS += -DTHREADED_DELTA_SEARCH
diff --git a/compat/strchrnul.c b/compat/strchrnul.c
deleted file mode 100644
index 51839fe..0000000
--- a/compat/strchrnul.c
+++ /dev/null
@@ -1,8 +0,0 @@
-#include "../git-compat-util.h"
-
-char *gitstrchrnul(const char *s, int c)
-{
-	while (*s && *s != c)
-		s++;
-	return (char *)s;
-}
diff --git a/git-compat-util.h b/git-compat-util.h
index e72654b..11e6df6 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -183,9 +183,14 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
                 const void *needle, size_t needlelen);
 #endif
 
-#ifdef NO_STRCHRNUL
+#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
 #define strchrnul gitstrchrnul
-char *gitstrchrnul(const char *s, int c);
+static inline char *gitstrchrnul(const char *s, int c)
+{
+        while (*s && *s != c)
+                s++;
+        return (char *)s;
+}
 #endif
 
 extern void release_pack_memory(size_t, int);

Re: [PATCH] Simplify strchrnul() compat code

[Andreas Ericsson]

René Scharfe wrote:
>  
> -#ifdef NO_STRCHRNUL
> +#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)

This will break things for users of glibc-2.1.1 (the first release still
available from ftp://sources.redhat.com/pub/glibc/old-releases that
includes the strchrnul() function), since __GLIBC_PREREQ() was invented
after strchrnul() was introduced.

Replacing __GLIBC__ with __GLIBC_PREREQ (as in the original patch) will
solve it nicely. Users of glibc-2.1.1 will be the odd minority where
strchrnul() is available in their libc but not used.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Simplify strchrnul() compat code

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> René Scharfe wrote:
>>  -#ifdef NO_STRCHRNUL
>> +#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
>
> This will break things for users of glibc-2.1.1 (the first release still
> available from ftp://sources.redhat.com/pub/glibc/old-releases that
> includes the strchrnul() function), since __GLIBC_PREREQ() was invented
> after strchrnul() was introduced.
>
> Replacing __GLIBC__ with __GLIBC_PREREQ (as in the original patch) will
> solve it nicely. Users of glibc-2.1.1 will be the odd minority where
> strchrnul() is available in their libc but not used.

Do you mean this on top of René's patch?  Although I do not
think I saw "the original patch" that did it this way, I think
it makes sense.

diff --git a/git-compat-util.h b/git-compat-util.h
index 11e6df6..dd96f7a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -183,7 +183,7 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
                 const void *needle, size_t needlelen);
 #endif
 
-#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
+#if !defined(__GLIBC_PREREQ) && !__GLIBC_PREREQ(2, 1)
 #define strchrnul gitstrchrnul
 static inline char *gitstrchrnul(const char *s, int c)
 {

Re: [PATCH] Simplify strchrnul() compat code

[Andreas Ericsson]

Junio C Hamano wrote:
> Andreas Ericsson <ae@op5.se> writes:
> 
>> René Scharfe wrote:
>>>  -#ifdef NO_STRCHRNUL
>>> +#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
>> This will break things for users of glibc-2.1.1 (the first release still
>> available from ftp://sources.redhat.com/pub/glibc/old-releases that
>> includes the strchrnul() function), since __GLIBC_PREREQ() was invented
>> after strchrnul() was introduced.
>>
>> Replacing __GLIBC__ with __GLIBC_PREREQ (as in the original patch) will
>> solve it nicely. Users of glibc-2.1.1 will be the odd minority where
>> strchrnul() is available in their libc but not used.
> 
> Do you mean this on top of René's patch?

Yes.

>  Although I do not
> think I saw "the original patch" that did it this way,

My memory seems to be failing. I never committed my proposal to my repo,
and the one I sent out was the __GLIBC__ || !__GLIBC_PREREQ thing. My
apologies.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Simplify strchrnul() compat code

[David Symonds]

On Nov 11, 2007 9:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Andreas Ericsson <ae@op5.se> writes:
>
> > René Scharfe wrote:
> >>  -#ifdef NO_STRCHRNUL
> >> +#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
> >
> > This will break things for users of glibc-2.1.1 (the first release still
> > available from ftp://sources.redhat.com/pub/glibc/old-releases that
> > includes the strchrnul() function), since __GLIBC_PREREQ() was invented
> > after strchrnul() was introduced.
> >
> > Replacing __GLIBC__ with __GLIBC_PREREQ (as in the original patch) will
> > solve it nicely. Users of glibc-2.1.1 will be the odd minority where
> > strchrnul() is available in their libc but not used.
>
> Do you mean this on top of René's patch?  Although I do not
> think I saw "the original patch" that did it this way, I think
> it makes sense.
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 11e6df6..dd96f7a 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -183,7 +183,7 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
>                  const void *needle, size_t needlelen);
>  #endif
>
> -#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
> +#if !defined(__GLIBC_PREREQ) && !__GLIBC_PREREQ(2, 1)
>  #define strchrnul gitstrchrnul
>  static inline char *gitstrchrnul(const char *s, int c)
>  {

I just tested it on my machine (OS X Tiger) now that it's in 'next',
and this breaks the build:

    CC git.o
In file included from builtin.h:4,
                 from git.c:1:
git-compat-util.h:187:48: error: missing binary operator before token "("
make: *** [git.o] Error 1


I don't think I have __GLIBC_PREREQ defined anywhere I can find.

Dave.

Re: [PATCH] Simplify strchrnul() compat code

[Johannes Sixt]

David Symonds schrieb:
> On Nov 11, 2007 9:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> @@ -183,7 +183,7 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
>>                  const void *needle, size_t needlelen);
>>  #endif
>>
>> -#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
>> +#if !defined(__GLIBC_PREREQ) && !__GLIBC_PREREQ(2, 1)
>>  #define strchrnul gitstrchrnul
>>  static inline char *gitstrchrnul(const char *s, int c)
>>  {
> 
> I just tested it on my machine (OS X Tiger) now that it's in 'next',
> and this breaks the build:
> 
>     CC git.o
> In file included from builtin.h:4,
>                  from git.c:1:
> git-compat-util.h:187:48: error: missing binary operator before token "("
> make: *** [git.o] Error 1
> 
> 
> I don't think I have __GLIBC_PREREQ defined anywhere I can find.

Turn the && in that line into || and it should work.

-- Hannes

Re: [PATCH] Simplify strchrnul() compat code

[David Symonds]

On Nov 12, 2007 8:12 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> David Symonds schrieb:
> > On Nov 11, 2007 9:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> @@ -183,7 +183,7 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
> >>                  const void *needle, size_t needlelen);
> >>  #endif
> >>
> >> -#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
> >> +#if !defined(__GLIBC_PREREQ) && !__GLIBC_PREREQ(2, 1)
> >>  #define strchrnul gitstrchrnul
> >>  static inline char *gitstrchrnul(const char *s, int c)
> >>  {
> >
> > I just tested it on my machine (OS X Tiger) now that it's in 'next',
> > and this breaks the build:
> >
> >     CC git.o
> > In file included from builtin.h:4,
> >                  from git.c:1:
> > git-compat-util.h:187:48: error: missing binary operator before token "("
> > make: *** [git.o] Error 1
> >
> >
> > I don't think I have __GLIBC_PREREQ defined anywhere I can find.
>
> Turn the && in that line into || and it should work.

Nope, no dice. Plus, that'd change the logic. I also tried turning the
(!X && !Y) into (X || Y) to no avail.


Dave.

Re: [PATCH] Simplify strchrnul() compat code

[Johannes Sixt]

David Symonds schrieb:
> On Nov 12, 2007 8:12 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> David Symonds schrieb:
>>> I don't think I have __GLIBC_PREREQ defined anywhere I can find.
>> Turn the && in that line into || and it should work.
> 
> Nope, no dice. Plus, that'd change the logic. I also tried turning the
> (!X && !Y) into (X || Y) to no avail.

Ok, then how about this?

---
diff --git a/git-compat-util.h b/git-compat-util.h
index 3d147b6..276a437 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -184,7 +184,13 @@ void *gitmemmem(const void *haystack,
                  const void *needle, size_t needlelen);
  #endif

-#if !defined(__GLIBC_PREREQ) && !__GLIBC_PREREQ(2, 1)
+#ifdef __GLIBC_PREREQ
+#if __GLIBC_PREREQ(2, 1)
+#define HAVE_STRCHRNUL
+#endif
+#endif
+
+#ifndef HAVE_STRCHRNUL
  #define strchrnul gitstrchrnul
  static inline char *gitstrchrnul(const char *s, int c)
  {

Re: [PATCH] Simplify strchrnul() compat code

[David Symonds]

On Nov 12, 2007 8:50 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> David Symonds schrieb:
> > On Nov 12, 2007 8:12 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> >> David Symonds schrieb:
> >>> I don't think I have __GLIBC_PREREQ defined anywhere I can find.
> >> Turn the && in that line into || and it should work.
> >
> > Nope, no dice. Plus, that'd change the logic. I also tried turning the
> > (!X && !Y) into (X || Y) to no avail.
>
> Ok, then how about this?
>
> ---
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 3d147b6..276a437 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -184,7 +184,13 @@ void *gitmemmem(const void *haystack,
>                   const void *needle, size_t needlelen);
>   #endif
>
> -#if !defined(__GLIBC_PREREQ) && !__GLIBC_PREREQ(2, 1)
> +#ifdef __GLIBC_PREREQ
> +#if __GLIBC_PREREQ(2, 1)
> +#define HAVE_STRCHRNUL
> +#endif
> +#endif
> +
> +#ifndef HAVE_STRCHRNUL
>
>   #define strchrnul gitstrchrnul
>   static inline char *gitstrchrnul(const char *s, int c)
>   {

Yep, that works just fine. Ack.


Dave.

Re: [PATCH] Simplify strchrnul() compat code

[Jakub Narebski]

David Symonds wrote:
> On Nov 12, 2007 8:50 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:

>> -#if !defined(__GLIBC_PREREQ) && !__GLIBC_PREREQ(2, 1)
>> +#ifdef __GLIBC_PREREQ
>> +#if __GLIBC_PREREQ(2, 1)
>> +#define HAVE_STRCHRNUL
>> +#endif
>> +#endif
>> +
>> +#ifndef HAVE_STRCHRNUL
>>
>>   #define strchrnul gitstrchrnul
>>   static inline char *gitstrchrnul(const char *s, int c)
>>   {
> 
> Yep, that works just fine. Ack.

This version has also the following advantage that you can set
HAVE_STRCHRNULL if above test does not detect it in your library.

Althought you'd better write then:

+#if __GLIBC_PREREQ(2, 1) && !defined(HAVE_STRCHRNUL)

Ack.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

[PATCH] Fix preprocessor logic that determines the availablity of strchrnul().

[Johannes Sixt]

Apart from the error in the condition (&& should actually be ||), the
construct

    #if !defined(A) || !A

leads to a syntax error in the C preprocessor if A is indeed not defined.

Tested-by: David Symonds <dsymonds@gmail.com>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
  git-compat-util.h |    8 +++++++-
  1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 3d147b6..276a437 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -184,7 +184,13 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
                  const void *needle, size_t needlelen);
  #endif

-#if !defined(__GLIBC_PREREQ) && !__GLIBC_PREREQ(2, 1)
+#ifdef __GLIBC_PREREQ
+#if __GLIBC_PREREQ(2, 1)
+#define HAVE_STRCHRNUL
+#endif
+#endif
+
+#ifndef HAVE_STRCHRNUL
  #define strchrnul gitstrchrnul
  static inline char *gitstrchrnul(const char *s, int c)
  {
-- 
1.5.3.5.578.g1f8ec