Subject: Re: [PATCH 00/12] Hard coded string length cleanup

From: Junio C Hamano <[email-masked]>
Date: Fri, 20 Dec 2013 08:53:27 -0800

René Scharfe <[email-masked]> writes:

> Am 20.12.2013 00:50, schrieb Duy Nguyen:
>> On Fri, Dec 20, 2013 at 6:32 AM, René Scharfe <[email-masked]> wrote:
>>> Seeing that skip_prefix_defval is mostly used in the form
>>> skip_prefix_defval(foo, prefix, foo) I wonder if it makes sense to
>>> first change skip_prefix to return the full string instead of NULL
>>> if the prefix is not matched.  Would the resulting function cover
>>> most use cases?  And would it still be easily usable?
>> 
>> That was skip_prefix_gently() that I forgot to replace in a commit
>> message, before I turned it into _defval variant. The reason for
>> _defval is it could be use to chain expression together without adding
>> temporary variables, e.g.
>> 
>> -       if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
>> +       if (isspace(*skip_prefix_defval(line->buf, ">From", "NOSPACE"))) {
>> 
>> Without _defval, one would need to do if ((p = skip_prefix(..)) &&
>> isspace(*p)). I'm not entirely sure this is a good thing though as it
>> could make it a bit harder to read.
>
> That usage is quite rare compared to occurrences of
> skip_prefix_defval(foo, prefix, foo), no?  Adding a temporary variable
> for them wouldn't be that bad if we can simplify the API to a single
> function -- if that one is usable, that is.
>
> On the other hand, we could add a special function for that example
> and we'd already have three users in the tree (patch below).  I think
> that's too narrow a use case for a library function, though.  Doing
> the following instead in the three cases doesn't seem to be too bad:
>
> 	rest = skip_prefix(line->buf, ">From");
> 	if (rest != line->buf && isspace(*rest)) {

Yeah, I personally feel that the "NOSPACE" hack is a bit too ugly to
live in a code meant to be maintained for a longer term.  The above
with a "rest" variable, whose assignment is outside if () condition,
is so far the easiest to read, at least to me.

I am not convinced if skip-prefix-and-space is even a good
abstraction of anything; it feels a bit too specialized.

Thanks.

> ---
>  builtin/apply.c    | 2 +-
>  builtin/mailinfo.c | 4 ++--
>  git-compat-util.h  | 1 +
>  strbuf.c           | 9 +++++++++
>  4 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index b0d0986..b96befd 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -433,7 +433,7 @@ static unsigned long linelen(const char *buffer, unsigned long size)
>  
>  static int is_dev_null(const char *str)
>  {
> -	return !memcmp("/dev/null", str, 9) && isspace(str[9]);
> +	return skip_prefix_and_space(str, "/dev/null") != str;
>  }
>  
>  #define TERM_SPACE	1
> diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
> index 2c3cd8e..2575989 100644
> --- a/builtin/mailinfo.c
> +++ b/builtin/mailinfo.c
> @@ -328,11 +328,11 @@ static int check_header(const struct strbuf *line,
>  	}
>  
>  	/* for inbody stuff */
> -	if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
> +	if (skip_prefix_and_space(line->buf, ">From") != line->buf) {
>  		ret = 1; /* Should this return 0? */
>  		goto check_header_out;
>  	}
> -	if (starts_with(line->buf, "[PATCH]") && isspace(line->buf[7])) {
> +	if (skip_prefix_and_space(line->buf, "[PATCH]") != line->buf) {
>  		for (i = 0; header[i]; i++) {
>  			if (!memcmp("Subject", header[i], 7)) {
>  				handle_header(&hdr_data[i], line);
> diff --git a/git-compat-util.h b/git-compat-util.h
> index dcb92c4..a083918 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -355,6 +355,7 @@ extern int prefixcmp(const char *str, const char *prefix);
>  extern int ends_with(const char *str, const char *suffix);
>  extern int suffixcmp(const char *str, const char *suffix);
>  extern const char *skip_prefix(const char *str, const char *prefix);
> +extern const char *skip_prefix_and_space(const char *str, const char *prefix);
>  
>  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
>  
> diff --git a/strbuf.c b/strbuf.c
> index 222df13..768331f 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -47,6 +47,15 @@ const char *skip_prefix(const char *str, const char *prefix)
>  			return str;
>  }
>  
> +const char *skip_prefix_and_space(const char *str, const char *prefix)
> +{
> +	const char *p = skip_prefix(str, prefix);
> +	if (((p != str) || !*prefix) && isspace(*p))
> +		return p + 1;
> +	else
> +		return str;
> +}
> +
>  /*
>   * Used as the default ->buf value, so that people can always assume
>   * buf is non NULL and ->buf is NUL terminated even for a freshly