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.

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 <[email protected]>

---

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

Makefile +0 -0

index 0d5590f..578c999 100644

Quoted 2 lines · René Scharfe

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

Quoted 8 lines · Andreas Ericsson

Quoted 2 lines · René Scharfe

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

Quoted 3 lines

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,

Quoted 4 lines · Andreas Ericsson

Quoted 2 lines · René Scharfe

-#ifdef NO_STRCHRNUL
+#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)

Quoted 8 lines

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.

git-compat-util.h +0 -0

index 11e6df6..dd96f7a 100644

git-compat-util.h void *gitmemmem(const void *haystack, size_t haystacklen, · line 189

Quoted 9 lines · Junio C Hamano

git-compat-util.h +1 -1

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

Quoted 21 lines · David Symonds

git-compat-util.h void *gitmemmem(const void *haystack, size_t haystacklen, · line 189

Quoted 9 lines · Junio C Hamano

git-compat-util.h +1 -1

@@ -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

Quoted 1 line

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

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

Quoted 3 lines · Johannes Sixt

Quoted 1 line · David Symonds

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.

Quoted 7 lines · David Symonds

Quoted 3 lines · Johannes Sixt

Quoted 1 line · David Symonds

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?

---

git-compat-util.h +7 -1

index 3d147b6..276a437 100644
@@ -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)

  {

Quoted 9 lines · Johannes Sixt

git-compat-util.h +7 -1

-#if !defined(__GLIBC_PREREQ) && !__GLIBC_PREREQ(2, 1)
+#ifdef __GLIBC_PREREQ
+#if __GLIBC_PREREQ(2, 1)
+#define HAVE_STRCHRNUL
+#endif
+#endif
+
+#ifndef HAVE_STRCHRNUL

Quoted 3 lines

  #define strchrnul gitstrchrnul
  static inline char *gitstrchrnul(const char *s, int c)
  {

Yep, that works just fine. Ack.

Quoted 14 lines · 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 <[email protected]>

---

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

Makefile all:: · line 34

Quoted 10 lines

Makefile +2 -0

index 0d5590f..578c999 100644
@@ -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?

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.

builtin-fetch--tool.c +1 -3

index 6a78517..ed60847 100644
@@ -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;

[Cc: Andreas Ericsson <[email protected]>,
René Scharfe <[email protected]>,
Junio C Hamano <[email protected]>,
[email protected]]

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@

Quoted 1 line

This seems overly complicated. How about this instead?

[...]

Quoted 4 lines

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

Quoted 7 lines

(quoted diff) +7 -0

+#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++;
+

Review

Quoted 4 lines

(quoted diff) +4 -0

+       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?

builtin-fetch--tool.c

Quoted 1 line

builtin-fetch--tool.c +0 -0

This IMHO should go to separate patch.

Quoted 17 lines · Jakub Narebski

Review

Quoted 11 lines · René Scharfe

(quoted diff) +11 -0

+#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.

Makefile +8 -0

index 0d5590f..578c999 100644
@@ -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

compat/strchrnul.c new file +0 -0

new file mode 100644
index 0000000..51839fe