Re: [PATCH 03/10] Move builtin-remote's skip_prefix() to git-compat-util.h

[Junio C Hamano <gitster@pobox.com>]

Miklos Vajna <vmiklos@frugalware.org> writes:

> diff --git a/builtin-remote.c b/builtin-remote.c
> index c49f00f..9c25018 100644
> --- a/builtin-remote.c
> +++ b/builtin-remote.c
> @@ -29,12 +29,6 @@ static inline int postfixcmp(const char *string, const char *postfix)
>  	return strcmp(string + len1 - len2, postfix);
>  }
>  
> -static inline const char *skip_prefix(const char *name, const char *prefix)
> -{
> -	return !name ? "" :
> -		prefixcmp(name, prefix) ?  name : name + strlen(prefix);
> -}
> -
>  static int opt_parse_track(const struct option *opt, const char *arg, int not)
>  {
>  	struct path_list *list = opt->value;
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 01c4045..af515d4 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -127,6 +127,12 @@ extern void set_warn_routine(void (*routine)(const char *warn, va_list params));
>  
>  extern int prefixcmp(const char *str, const char *prefix);
>  
> +static inline const char *skip_prefix(const char *name, const char *prefix)
> +{
> +	return !name ? "" :
> +		prefixcmp(name, prefix) ?  name : name + strlen(prefix);
> +}
> +

Somehow you seemed to have picked the one whose semantics is defined much
less clearly.  For one thing, it takes more effort (and unnatural way to
check) for the caller to detect the case where prefix did not match than
the one that returns NULL.  Worse, I think this one is less efficient,
doing strlen(prefix) twice.

> diff --git a/parse-options.c b/parse-options.c
> index acf3fe3..aa164d0 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -22,12 +22,6 @@ static inline const char *get_arg(struct optparse_t *p)
>  	return *++p->argv;
>  }
>  
> -static inline const char *skip_prefix(const char *str, const char *prefix)
> -{
> -	size_t len = strlen(prefix);
> -	return strncmp(str, prefix, len) ? NULL : str + len;
> -}
> -
>  static int opterror(const struct option *opt, const char *reason, int flags)
>  {
>  	if (flags & OPT_SHORT)
> @@ -161,7 +155,7 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
>  
>  		rest = skip_prefix(arg, options->long_name);
>  		if (options->type == OPTION_ARGUMENT) {
> -			if (!rest)
> +			if (!strcmp(rest, arg))

Ugh.

At least please have the courtesy of not making it more expensive than the
original unnecessarily.  Isn't (rest == arg) enough here?

Still, I think the one from builtin-remote.c you used is a much worse
implementation of the same thing between the two.  It was Ok while it was
a local scope hack only for builtin-remote.c's use, but a special calling
convention like "if name is NULL return empty string" should not be
promoted to public utility library status without defending why it is
always a good idea to do so.