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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2008, 22:38 UTC
Message-ID: <7vtzg7zew0.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <5aca216074b88d68f97b8223ebf6272dfe6bddeb.1212698317.git.vmiklos@frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

Show 31 quoted lines

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

Show 23 quoted lines

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