Re: [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 4, 2013, 19:19 UTC
Message-ID: <xmqqbo20ynxs.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20131103201303.14446.7508.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

> Commit 8cc5b290 (git merge -X<option>, 25 Nov 2009) introduced
> suffixcmp() with nearly the same implementation as postfixcmp()
> that already existed since commit 211c8968 (Make git-remote a
> builtin, 29 Feb 2008).

This "nearly the same" piqued my curiosity ;-)

The postfixcmp() you are removing will say "f" > ".txt" while suffixcmp() will say "f" < ".txt".

As postfixcmp() is only used to compare for equality, the distinction does not matter and does not affect the correctness of this patch, but I am not sure which answer is more correct.

I do not think anybody sane uses prefixcmp() or suffixcmp() for anything but checking with zero; in other words, I suspect that all uses of Xcmp() can be replaced with !!Xcmp(), so as a separate clean-up patch, we may at least want to make it clear that the callers should not expect anything but "does str have sfx as its suffix, yes or no?" by doing something like this:

 int suffixcmp(const char *str, const char *suffix)
 {
 	int len = strlen(str), suflen = strlen(suffix);
 	if (len < suflen)
 		return -1;
 	else
-		return strcmp(str + len - suflen, suffix);
+		return !!strcmp(str + len - suflen, suffix);
 }

I am not absolutely sure about doing the same to prefixcmp(), though. It could be used for ordering, even though no existing code seems to do so.

Show 46 quoted lines

> As postfixcmp() has always been static in builtin/remote.c
> and is used nowhere else, it makes more sense to remove it
> and use suffixcmp() instead in builtin/remote.c, rather than
> to remove suffixcmp().
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/remote.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 4e14891..9b3a98e 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -80,14 +80,6 @@ static int verbose;
>  static int show_all(void);
>  static int prune_remote(const char *remote, int dry_run);
>  
> -static inline int postfixcmp(const char *string, const char *postfix)
> -{
> -	int len1 = strlen(string), len2 = strlen(postfix);
> -	if (len1 < len2)
> -		return 1;
> -	return strcmp(string + len1 - len2, postfix);
> -}
> -
>  static int fetch_remote(const char *name)
>  {
>  	const char *argv[] = { "fetch", name, NULL, NULL };
> @@ -277,13 +269,13 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>  		enum { REMOTE, MERGE, REBASE } type;
>  
>  		key += 7;
> -		if (!postfixcmp(key, ".remote")) {
> +		if (!suffixcmp(key, ".remote")) {
>  			name = xstrndup(key, strlen(key) - 7);
>  			type = REMOTE;
> -		} else if (!postfixcmp(key, ".merge")) {
> +		} else if (!suffixcmp(key, ".merge")) {
>  			name = xstrndup(key, strlen(key) - 6);
>  			type = MERGE;
> -		} else if (!postfixcmp(key, ".rebase")) {
> +		} else if (!suffixcmp(key, ".rebase")) {
>  			name = xstrndup(key, strlen(key) - 7);
>  			type = REBASE;
>  		} else