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

[Jakub Narebski <jnareb@gmail.com>]

[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