{"thread":{"id":"35281","subject":"[PATCH v2] Rename suffixcmp() to has_suffix() and inverse its result","startedAt":"2013-11-05T04:57:21Z","lastAt":"2013-11-05T19:12:48Z","messageCount":3,"participants":["Christian Couder","Max Horn","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"230109","messageId":"20131105045722.20276.20853.chriscool@tuxfamily.org","threadId":"35281","inReplyTo":null,"subject":"[PATCH v2] Rename suffixcmp() to has_suffix() and inverse its result","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-05T04:57:21Z","receivedAt":"2013-11-05T04:57:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"As suffixcmp() should not be used as an ordering comparison function,\nand anything-cmp() ought to be usable as an ordering comparison function,\nsuffixcmp() should be renamed to something that doesn't end with \"cmp\".\n\nhas_suffix() is a straightforward name for such a function, except\nthat with such a name callers will expect that it will return 1\nwhen the suffix is present and 0 otherwise.\n\nSo we need to also inverse the value returned by this function to\nmatch what the callers will expect, because suffixcmp() like all\nanything-cmp() returns 0 when the suffix is present and 1 or -1\notherwise.\n\nAs we inverse the value returned by the function, we also have\nto inverse the ways its callers are using its returned value.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\nHi Junio and Peff,\n\nSo here is a new version of the patch to rename\nsuffixcmp() into has_suffix(). We now inverse the\nresult of the function as we rename it.\n\nThis patch should be added to or squashed into the\npatch series that removes postfixcmp().\n\nThanks,\nChristian.\n\n builtin/clone.c           | 2 +-\n builtin/fetch.c           | 2 +-\n builtin/merge-recursive.c | 2 +-\n builtin/remote.c          | 6 +++---\n builtin/repack.c          | 2 +-\n connected.c               | 2 +-\n git-compat-util.h         | 2 +-\n strbuf.c                  | 6 +++---\n 8 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 874e0fd..84fb1bd 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -510,7 +510,7 @@ static void write_followtags(const struct ref *refs, const char *msg)\n \tfor (ref = refs; ref; ref = ref->next) {\n \t\tif (prefixcmp(ref->name, \"refs/tags/\"))\n \t\t\tcontinue;\n-\t\tif (!suffixcmp(ref->name, \"^{}\"))\n+\t\tif (has_suffix(ref->name, \"^{}\"))\n \t\t\tcontinue;\n \t\tif (!has_sha1_file(ref->old_sha1))\n \t\t\tcontinue;\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex bd7a101..8eb6cd0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -653,7 +653,7 @@ static void find_non_local_tags(struct transport *transport,\n \t\t * to fetch then we can mark the ref entry in the list\n \t\t * as one to ignore by setting util to NULL.\n \t\t */\n-\t\tif (!suffixcmp(ref->name, \"^{}\")) {\n+\t\tif (has_suffix(ref->name, \"^{}\")) {\n \t\t\tif (item && !has_sha1_file(ref->old_sha1) &&\n \t\t\t    !will_fetch(head, ref->old_sha1) &&\n \t\t\t    !has_sha1_file(item->util) &&\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex 3a64f5d..e7f1a39 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -29,7 +29,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n \tstruct commit *result;\n \n \tinit_merge_options(&o);\n-\tif (argv[0] && !suffixcmp(argv[0], \"-subtree\"))\n+\tif (argv[0] && has_suffix(argv[0], \"-subtree\"))\n \t\to.subtree_shift = \"\";\n \n \tif (argc < 4)\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 9b3a98e..b9a1024 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -269,13 +269,13 @@ static int config_read_branches(const char *key, const char *value, void *cb)\n \t\tenum { REMOTE, MERGE, REBASE } type;\n \n \t\tkey += 7;\n-\t\tif (!suffixcmp(key, \".remote\")) {\n+\t\tif (has_suffix(key, \".remote\")) {\n \t\t\tname = xstrndup(key, strlen(key) - 7);\n \t\t\ttype = REMOTE;\n-\t\t} else if (!suffixcmp(key, \".merge\")) {\n+\t\t} else if (has_suffix(key, \".merge\")) {\n \t\t\tname = xstrndup(key, strlen(key) - 6);\n \t\t\ttype = MERGE;\n-\t\t} else if (!suffixcmp(key, \".rebase\")) {\n+\t\t} else if (has_suffix(key, \".rebase\")) {\n \t\t\tname = xstrndup(key, strlen(key) - 7);\n \t\t\ttype = REBASE;\n \t\t} else\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a0ff5c7..9ef518d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -78,7 +78,7 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)\n \t\treturn;\n \n \twhile ((e = readdir(dir)) != NULL) {\n-\t\tif (suffixcmp(e->d_name, \".pack\"))\n+\t\tif (!has_suffix(e->d_name, \".pack\"))\n \t\t\tcontinue;\n \n \t\tlen = strlen(e->d_name) - strlen(\".pack\");\ndiff --git a/connected.c b/connected.c\nindex fae8d64..be9304e 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -38,7 +38,7 @@ int check_everything_connected_with_transport(sha1_iterate_fn fn,\n \tif (transport && transport->smart_options &&\n \t    transport->smart_options->self_contained_and_connected &&\n \t    transport->pack_lockfile &&\n-\t    !suffixcmp(transport->pack_lockfile, \".keep\")) {\n+\t    has_suffix(transport->pack_lockfile, \".keep\")) {\n \t\tstruct strbuf idx_file = STRBUF_INIT;\n \t\tstrbuf_addstr(&idx_file, transport->pack_lockfile);\n \t\tstrbuf_setlen(&idx_file, idx_file.len - 5); /* \".keep\" */\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 7776f12..0f6a31e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -351,7 +351,7 @@ extern void set_error_routine(void (*routine)(const char *err, va_list params));\n extern void set_die_is_recursing_routine(int (*routine)(void));\n \n extern int prefixcmp(const char *str, const char *prefix);\n-extern int suffixcmp(const char *str, const char *suffix);\n+extern int has_suffix(const char *str, const char *suffix);\n \n static inline const char *skip_prefix(const char *str, const char *prefix)\n {\ndiff --git a/strbuf.c b/strbuf.c\nindex 9ba50de..0d784b5 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -10,13 +10,13 @@ int prefixcmp(const char *str, const char *prefix)\n \t\t\treturn (unsigned char)*prefix - (unsigned char)*str;\n }\n \n-int suffixcmp(const char *str, const char *suffix)\n+int has_suffix(const char *str, const char *suffix)\n {\n \tint len = strlen(str), suflen = strlen(suffix);\n \tif (len < suflen)\n-\t\treturn -1;\n+\t\treturn 0;\n \telse\n-\t\treturn !!strcmp(str + len - suflen, suffix);\n+\t\treturn !strcmp(str + len - suflen, suffix);\n }\n \n /*\n-- \n1.8.4.1.561.g186b3da.dirty\n"},{"id":"230120","messageId":"C9A6A1E9-E08C-4C9B-B18D-CED751ACB975@quendi.de","threadId":"35281","inReplyTo":"20131105045722.20276.20853.chriscool@tuxfamily.org","subject":"Re: [PATCH v2] Rename suffixcmp() to has_suffix() and inverse its result","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2013-11-05T18:39:36Z","receivedAt":"2013-11-05T18:39:36Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"+1 for the change. I find the resulting code easier to understand, too.\n\n\nOn 05.11.2013, at 05:57, Christian Couder <chriscool@tuxfamily.org> wrote:\n\n> As suffixcmp() should not be used as an ordering comparison function,\n> and anything-cmp() ought to be usable as an ordering comparison function,\n> suffixcmp() should be renamed to something that doesn't end with \"cmp\".\n> \n> has_suffix() is a straightforward name for such a function, except\n> that with such a name callers will expect that it will return 1\n> when the suffix is present and 0 otherwise.\n> \n> So we need to also inverse the value returned by this function ti\n\n> match what the callers will expect, because suffixcmp() like all\n> anything-cmp() returns 0 when the suffix is present and 1 or -1\n> otherwise.\n> \n> As we inverse the value returned by the function, we also have\n> to inverse the ways its callers are using its returned value.\n\ns/inverse/invert/  (multiple times)\n\nTaking 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).\n\nHow about something like this:\n\n--- 8< ----\n\nRename suffixcmp() to has_suffix() and invert its result\n\nNow has_suffix() returns 1 when the suffix is present and 0 otherwise.\n\nThe old name followed the pattern anything-cmp(), which suggests\na general comparison function suitable for e.g. sorting objects.\nBut this was not the case for suffixcmp().\n\n--- 8< ----\n\n\nBy 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:\n\n  suffixcmp(\"3\", \"31\") = -1  (because \"31\" is longer than \"3\"), so \"3\" < \"31\"\n  suffixcmp(\"31\", \"2\") = -1  (because \"1\" < \"2\"), so \"31\" < \"2\"\nbut\n  suffixcmp(\"3\", \"2\") = 1    so \"3\" > \"2\"\n\n\n\n> \n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n> Hi Junio and Peff,\n> \n> So here is a new version of the patch to rename\n> suffixcmp() into has_suffix(). We now inverse the\n> result of the function as we rename it.\n> \n> This patch should be added to or squashed into the\n> patch series that removes postfixcmp().\n> \n\n[...]\n\n\n"},{"id":"230123","messageId":"xmqqd2meaci7.fsf@gitster.dls.corp.google.com","threadId":"35281","inReplyTo":"C9A6A1E9-E08C-4C9B-B18D-CED751ACB975@quendi.de","subject":"Re: [PATCH v2] Rename suffixcmp() to has_suffix() and inverse its result","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-05T19:12:48Z","receivedAt":"2013-11-05T19:12:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Horn <max@quendi.de> writes:\n\n> +1 for the change. I find the resulting code easier to understand, too.\n> ...\n>\n> Taking one step back, shouldn't the commit message rather explain\n> the new status, instead of referring so much to the past? If I\n> imagine somebody reading this in a year, they might not even know\n> suffixcmp (e.g. if they joined the project after this patch was\n> merged).\n>\n> How about something like this:\n>\n> --- 8< ----\n>\n> Rename suffixcmp() to has_suffix() and invert its result\n>\n> Now has_suffix() returns 1 when the suffix is present and 0 otherwise.\n>\n> The old name followed the pattern anything-cmp(), which suggests\n> a general comparison function suitable for e.g. sorting objects.\n> But this was not the case for suffixcmp().\n\nYes, much more concise and to the point.\n"}]}