[PATCH v2] Rename suffixcmp() to has_suffix() and inverse its result

[PATCH v2] Rename suffixcmp() to has_suffix() and inverse its result

[Christian Couder]

As suffixcmp() should not be used as an ordering comparison function,
and anything-cmp() ought to be usable as an ordering comparison function,
suffixcmp() should be renamed to something that doesn't end with "cmp".

has_suffix() is a straightforward name for such a function, except
that with such a name callers will expect that it will return 1
when the suffix is present and 0 otherwise.

So we need to also inverse the value returned by this function to
match what the callers will expect, because suffixcmp() like all
anything-cmp() returns 0 when the suffix is present and 1 or -1
otherwise.

As we inverse the value returned by the function, we also have
to inverse the ways its callers are using its returned value.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Hi Junio and Peff,

So here is a new version of the patch to rename
suffixcmp() into has_suffix(). We now inverse the
result of the function as we rename it.

This patch should be added to or squashed into the
patch series that removes postfixcmp().

Thanks,
Christian.

 builtin/clone.c           | 2 +-
 builtin/fetch.c           | 2 +-
 builtin/merge-recursive.c | 2 +-
 builtin/remote.c          | 6 +++---
 builtin/repack.c          | 2 +-
 connected.c               | 2 +-
 git-compat-util.h         | 2 +-
 strbuf.c                  | 6 +++---
 8 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 874e0fd..84fb1bd 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -510,7 +510,7 @@ static void write_followtags(const struct ref *refs, const char *msg)
 	for (ref = refs; ref; ref = ref->next) {
 		if (prefixcmp(ref->name, "refs/tags/"))
 			continue;
-		if (!suffixcmp(ref->name, "^{}"))
+		if (has_suffix(ref->name, "^{}"))
 			continue;
 		if (!has_sha1_file(ref->old_sha1))
 			continue;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bd7a101..8eb6cd0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -653,7 +653,7 @@ static void find_non_local_tags(struct transport *transport,
 		 * to fetch then we can mark the ref entry in the list
 		 * as one to ignore by setting util to NULL.
 		 */
-		if (!suffixcmp(ref->name, "^{}")) {
+		if (has_suffix(ref->name, "^{}")) {
 			if (item && !has_sha1_file(ref->old_sha1) &&
 			    !will_fetch(head, ref->old_sha1) &&
 			    !has_sha1_file(item->util) &&
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 3a64f5d..e7f1a39 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -29,7 +29,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 	struct commit *result;
 
 	init_merge_options(&o);
-	if (argv[0] && !suffixcmp(argv[0], "-subtree"))
+	if (argv[0] && has_suffix(argv[0], "-subtree"))
 		o.subtree_shift = "";
 
 	if (argc < 4)
diff --git a/builtin/remote.c b/builtin/remote.c
index 9b3a98e..b9a1024 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -269,13 +269,13 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 		enum { REMOTE, MERGE, REBASE } type;
 
 		key += 7;
-		if (!suffixcmp(key, ".remote")) {
+		if (has_suffix(key, ".remote")) {
 			name = xstrndup(key, strlen(key) - 7);
 			type = REMOTE;
-		} else if (!suffixcmp(key, ".merge")) {
+		} else if (has_suffix(key, ".merge")) {
 			name = xstrndup(key, strlen(key) - 6);
 			type = MERGE;
-		} else if (!suffixcmp(key, ".rebase")) {
+		} else if (has_suffix(key, ".rebase")) {
 			name = xstrndup(key, strlen(key) - 7);
 			type = REBASE;
 		} else
diff --git a/builtin/repack.c b/builtin/repack.c
index a0ff5c7..9ef518d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -78,7 +78,7 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)
 		return;
 
 	while ((e = readdir(dir)) != NULL) {
-		if (suffixcmp(e->d_name, ".pack"))
+		if (!has_suffix(e->d_name, ".pack"))
 			continue;
 
 		len = strlen(e->d_name) - strlen(".pack");
diff --git a/connected.c b/connected.c
index fae8d64..be9304e 100644
--- a/connected.c
+++ b/connected.c
@@ -38,7 +38,7 @@ int check_everything_connected_with_transport(sha1_iterate_fn fn,
 	if (transport && transport->smart_options &&
 	    transport->smart_options->self_contained_and_connected &&
 	    transport->pack_lockfile &&
-	    !suffixcmp(transport->pack_lockfile, ".keep")) {
+	    has_suffix(transport->pack_lockfile, ".keep")) {
 		struct strbuf idx_file = STRBUF_INIT;
 		strbuf_addstr(&idx_file, transport->pack_lockfile);
 		strbuf_setlen(&idx_file, idx_file.len - 5); /* ".keep" */
diff --git a/git-compat-util.h b/git-compat-util.h
index 7776f12..0f6a31e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -351,7 +351,7 @@ extern void set_error_routine(void (*routine)(const char *err, va_list params));
 extern void set_die_is_recursing_routine(int (*routine)(void));
 
 extern int prefixcmp(const char *str, const char *prefix);
-extern int suffixcmp(const char *str, const char *suffix);
+extern int has_suffix(const char *str, const char *suffix);
 
 static inline const char *skip_prefix(const char *str, const char *prefix)
 {
diff --git a/strbuf.c b/strbuf.c
index 9ba50de..0d784b5 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -10,13 +10,13 @@ int prefixcmp(const char *str, const char *prefix)
 			return (unsigned char)*prefix - (unsigned char)*str;
 }
 
-int suffixcmp(const char *str, const char *suffix)
+int has_suffix(const char *str, const char *suffix)
 {
 	int len = strlen(str), suflen = strlen(suffix);
 	if (len < suflen)
-		return -1;
+		return 0;
 	else
-		return !!strcmp(str + len - suflen, suffix);
+		return !strcmp(str + len - suflen, suffix);
 }
 
 /*
-- 
1.8.4.1.561.g186b3da.dirty

Re: [PATCH v2] Rename suffixcmp() to has_suffix() and inverse its result

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 2429 bytes --]

+1 for the change. I find the resulting code easier to understand, too.


On 05.11.2013, at 05:57, Christian Couder <chriscool@tuxfamily.org> wrote:

> As suffixcmp() should not be used as an ordering comparison function,
> and anything-cmp() ought to be usable as an ordering comparison function,
> suffixcmp() should be renamed to something that doesn't end with "cmp".
> 
> has_suffix() is a straightforward name for such a function, except
> that with such a name callers will expect that it will return 1
> when the suffix is present and 0 otherwise.
> 
> So we need to also inverse the value returned by this function ti

> match what the callers will expect, because suffixcmp() like all
> anything-cmp() returns 0 when the suffix is present and 1 or -1
> otherwise.
> 
> As we inverse the value returned by the function, we also have
> to inverse the ways its callers are using its returned value.

s/inverse/invert/  (multiple times)

Taking one step back, shouldn't the commit message rather explain the new status, instead of referring so much to the past? If I imagine somebody reading this in a year, they might not even know suffixcmp (e.g. if they joined the project after this patch was merged).

How about something like this:

--- 8< ----

Rename suffixcmp() to has_suffix() and invert its result

Now has_suffix() returns 1 when the suffix is present and 0 otherwise.

The old name followed the pattern anything-cmp(), which suggests
a general comparison function suitable for e.g. sorting objects.
But this was not the case for suffixcmp().

--- 8< ----


By the way, a much stronger reason why suffixcmp is not suitable than that it is not clear what it would mean, is that it is not transitive. I.e. for an ordering you would want that if a<b and b<c then a<c. This is /was not the case for suffixcmp:

  suffixcmp("3", "31") = -1  (because "31" is longer than "3"), so "3" < "31"
  suffixcmp("31", "2") = -1  (because "1" < "2"), so "31" < "2"
but
  suffixcmp("3", "2") = 1    so "3" > "2"



> 
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Hi Junio and Peff,
> 
> So here is a new version of the patch to rename
> suffixcmp() into has_suffix(). We now inverse the
> result of the function as we rename it.
> 
> This patch should be added to or squashed into the
> patch series that removes postfixcmp().
> 

[...]



[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: [PATCH v2] Rename suffixcmp() to has_suffix() and inverse its result

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

> +1 for the change. I find the resulting code easier to understand, too.
> ...
>
> Taking one step back, shouldn't the commit message rather explain
> the new status, instead of referring so much to the past? If I
> imagine somebody reading this in a year, they might not even know
> suffixcmp (e.g. if they joined the project after this patch was
> merged).
>
> How about something like this:
>
> --- 8< ----
>
> Rename suffixcmp() to has_suffix() and invert its result
>
> Now has_suffix() returns 1 when the suffix is present and 0 otherwise.
>
> The old name followed the pattern anything-cmp(), which suggests
> a general comparison function suitable for e.g. sorting objects.
> But this was not the case for suffixcmp().

Yes, much more concise and to the point.