{"thread":{"id":"35269","subject":"[PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead","startedAt":"2013-11-03T20:13:02Z","lastAt":"2013-11-04T22:37:13Z","messageCount":7,"participants":["Christian Couder","Jonathan Nieder","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"230069","messageId":"20131103201303.14446.7508.chriscool@tuxfamily.org","threadId":"35269","inReplyTo":null,"subject":"[PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-03T20:13:02Z","receivedAt":"2013-11-03T20:13:02Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Commit 8cc5b290 (git merge -X<option>, 25 Nov 2009) introduced\nsuffixcmp() with nearly the same implementation as postfixcmp()\nthat already existed since commit 211c8968 (Make git-remote a\nbuiltin, 29 Feb 2008).\n\nAs postfixcmp() has always been static in builtin/remote.c\nand is used nowhere else, it makes more sense to remove it\nand use suffixcmp() instead in builtin/remote.c, rather than\nto remove suffixcmp().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/remote.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 4e14891..9b3a98e 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -80,14 +80,6 @@ static int verbose;\n static int show_all(void);\n static int prune_remote(const char *remote, int dry_run);\n \n-static inline int postfixcmp(const char *string, const char *postfix)\n-{\n-\tint len1 = strlen(string), len2 = strlen(postfix);\n-\tif (len1 < len2)\n-\t\treturn 1;\n-\treturn strcmp(string + len1 - len2, postfix);\n-}\n-\n static int fetch_remote(const char *name)\n {\n \tconst char *argv[] = { \"fetch\", name, NULL, NULL };\n@@ -277,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 (!postfixcmp(key, \".remote\")) {\n+\t\tif (!suffixcmp(key, \".remote\")) {\n \t\t\tname = xstrndup(key, strlen(key) - 7);\n \t\t\ttype = REMOTE;\n-\t\t} else if (!postfixcmp(key, \".merge\")) {\n+\t\t} else if (!suffixcmp(key, \".merge\")) {\n \t\t\tname = xstrndup(key, strlen(key) - 6);\n \t\t\ttype = MERGE;\n-\t\t} else if (!postfixcmp(key, \".rebase\")) {\n+\t\t} else if (!suffixcmp(key, \".rebase\")) {\n \t\t\tname = xstrndup(key, strlen(key) - 7);\n \t\t\ttype = REBASE;\n \t\t} else\n-- \n1.8.4.1.576.g36ba827.dirty\n"},{"id":"230079","messageId":"20131104175602.GB31881@google.com","threadId":"35269","inReplyTo":"20131103201303.14446.7508.chriscool@tuxfamily.org","subject":"Re: [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-11-04T17:56:02Z","receivedAt":"2013-11-04T17:56:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Christian Couder wrote:\n\n> Commit 8cc5b290 (git merge -X<option>, 25 Nov 2009) introduced\n> suffixcmp() with nearly the same implementation as postfixcmp()\n> that already existed since commit 211c8968 (Make git-remote a\n> builtin, 29 Feb 2008).\n[...]\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  builtin/remote.c | 14 +++-----------\n>  1 file changed, 3 insertions(+), 11 deletions(-)\n\nMakes sense.  Thanks for noticing.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"230086","messageId":"xmqqbo20ynxs.fsf@gitster.dls.corp.google.com","threadId":"35269","inReplyTo":"20131103201303.14446.7508.chriscool@tuxfamily.org","subject":"Re: [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-04T19:19:43Z","receivedAt":"2013-11-04T19:19:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> Commit 8cc5b290 (git merge -X<option>, 25 Nov 2009) introduced\n> suffixcmp() with nearly the same implementation as postfixcmp()\n> that already existed since commit 211c8968 (Make git-remote a\n> builtin, 29 Feb 2008).\n\nThis \"nearly the same\" piqued my curiosity ;-)\n\nThe postfixcmp() you are removing will say \"f\" > \".txt\" while\nsuffixcmp() will say \"f\" < \".txt\".\n\nAs postfixcmp() is only used to compare for equality, the\ndistinction does not matter and does not affect the correctness of\nthis patch, but I am not sure which answer is more correct.\n\nI do not think anybody sane uses prefixcmp() or suffixcmp() for\nanything but checking with zero; in other words, I suspect that all\nuses of Xcmp() can be replaced with !!Xcmp(), so as a separate\nclean-up patch, we may at least want to make it clear that the\ncallers should not expect anything but \"does str have sfx as its\nsuffix, yes or no?\" by doing something like this:\n\n int suffixcmp(const char *str, const char *suffix)\n {\n \tint len = strlen(str), suflen = strlen(suffix);\n \tif (len < suflen)\n \t\treturn -1;\n \telse\n-\t\treturn strcmp(str + len - suflen, suffix);\n+\t\treturn !!strcmp(str + len - suflen, suffix);\n }\n\nI am not absolutely sure about doing the same to prefixcmp(),\nthough. It could be used for ordering, even though no existing code\nseems to do so.\n\n> As postfixcmp() has always been static in builtin/remote.c\n> and is used nowhere else, it makes more sense to remove it\n> and use suffixcmp() instead in builtin/remote.c, rather than\n> to remove suffixcmp().\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  builtin/remote.c | 14 +++-----------\n>  1 file changed, 3 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 4e14891..9b3a98e 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -80,14 +80,6 @@ static int verbose;\n>  static int show_all(void);\n>  static int prune_remote(const char *remote, int dry_run);\n>  \n> -static inline int postfixcmp(const char *string, const char *postfix)\n> -{\n> -\tint len1 = strlen(string), len2 = strlen(postfix);\n> -\tif (len1 < len2)\n> -\t\treturn 1;\n> -\treturn strcmp(string + len1 - len2, postfix);\n> -}\n> -\n>  static int fetch_remote(const char *name)\n>  {\n>  \tconst char *argv[] = { \"fetch\", name, NULL, NULL };\n> @@ -277,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 (!postfixcmp(key, \".remote\")) {\n> +\t\tif (!suffixcmp(key, \".remote\")) {\n>  \t\t\tname = xstrndup(key, strlen(key) - 7);\n>  \t\t\ttype = REMOTE;\n> -\t\t} else if (!postfixcmp(key, \".merge\")) {\n> +\t\t} else if (!suffixcmp(key, \".merge\")) {\n>  \t\t\tname = xstrndup(key, strlen(key) - 6);\n>  \t\t\ttype = MERGE;\n> -\t\t} else if (!postfixcmp(key, \".rebase\")) {\n> +\t\t} else if (!suffixcmp(key, \".rebase\")) {\n>  \t\t\tname = xstrndup(key, strlen(key) - 7);\n>  \t\t\ttype = REBASE;\n>  \t\t} else\n"},{"id":"230087","messageId":"20131104.211627.54064800775850019.chriscool@tuxfamily.org","threadId":"35269","inReplyTo":"xmqqbo20ynxs.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-04T20:16:27Z","receivedAt":"2013-11-04T20:16:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\nSubject: Re: [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead\nDate: Mon, 04 Nov 2013 11:19:43 -0800\n\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> \n>> Commit 8cc5b290 (git merge -X<option>, 25 Nov 2009) introduced\n>> suffixcmp() with nearly the same implementation as postfixcmp()\n>> that already existed since commit 211c8968 (Make git-remote a\n>> builtin, 29 Feb 2008).\n> \n> This \"nearly the same\" piqued my curiosity ;-)\n\nYeah, I realize I should have explained the differences.\n \n> The postfixcmp() you are removing will say \"f\" > \".txt\" while\n> suffixcmp() will say \"f\" < \".txt\".\n> \n> As postfixcmp() is only used to compare for equality, the\n> distinction does not matter and does not affect the correctness of\n> this patch, but I am not sure which answer is more correct.\n\nYeah, that's also my opinion. I am not even sure if there is one more\ncorrect answer than the other.\n\n> I do not think anybody sane uses prefixcmp() or suffixcmp() for\n> anything but checking with zero; in other words, I suspect that all\n> uses of Xcmp() can be replaced with !!Xcmp(), so as a separate\n> clean-up patch, we may at least want to make it clear that the\n> callers should not expect anything but \"does str have sfx as its\n> suffix, yes or no?\" by doing something like this:\n> \n>  int suffixcmp(const char *str, const char *suffix)\n>  {\n>  \tint len = strlen(str), suflen = strlen(suffix);\n>  \tif (len < suflen)\n>  \t\treturn -1;\n>  \telse\n> -\t\treturn strcmp(str + len - suflen, suffix);\n> +\t\treturn !!strcmp(str + len - suflen, suffix);\n>  }\n> \n> I am not absolutely sure about doing the same to prefixcmp(),\n> though. It could be used for ordering, even though no existing code\n> seems to do so.\n\nI agree.\n\nI will resend a 2 patch long patch series where the first patch will\nhave an improved commit message and the second patch will do what you suggest above.\n\nThanks,\nChristian.\n"},{"id":"230088","messageId":"xmqq7gcnzznt.fsf@gitster.dls.corp.google.com","threadId":"35269","inReplyTo":"xmqqbo20ynxs.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-04T20:21:10Z","receivedAt":"2013-11-04T20:21:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I do not think anybody sane uses prefixcmp() or suffixcmp() for\n> anything but checking with zero; in other words, I suspect that all\n> uses of Xcmp() can be replaced with !!Xcmp(), so as a separate\n> clean-up patch, we may at least want to make it clear that the\n> callers should not expect anything but \"does str have sfx as its\n> suffix, yes or no?\" by doing something like this:\n>\n>  int suffixcmp(const char *str, const char *suffix)\n>  {\n>  \tint len = strlen(str), suflen = strlen(suffix);\n>  \tif (len < suflen)\n>  \t\treturn -1;\n>  \telse\n> -\t\treturn strcmp(str + len - suflen, suffix);\n> +\t\treturn !!strcmp(str + len - suflen, suffix);\n>  }\n>\n> I am not absolutely sure about doing the same to prefixcmp(),\n> though. It could be used for ordering, even though no existing code\n> seems to do so.\n\nI just realized why this suggestion is incomplete; if we were to go\nthis route, we should rename the function to has_suffix() or\nsomething. anything-cmp() ought to be usable as an ordering\ncomparison function, but suffixcmp() clearly isn't.\n"},{"id":"230097","messageId":"alpine.DEB.1.00.1311042316520.1191@s15462909.onlinehome-server.info","threadId":"35269","inReplyTo":"xmqq7gcnzznt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2013-11-04T22:17:27Z","receivedAt":"2013-11-04T22:17:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 4 Nov 2013, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > I do not think anybody sane uses prefixcmp() or suffixcmp() for\n> > anything but checking with zero; in other words, I suspect that all\n> > uses of Xcmp() can be replaced with !!Xcmp(), so as a separate\n> > clean-up patch, we may at least want to make it clear that the\n> > callers should not expect anything but \"does str have sfx as its\n> > suffix, yes or no?\" by doing something like this:\n> >\n> >  int suffixcmp(const char *str, const char *suffix)\n> >  {\n> >  \tint len = strlen(str), suflen = strlen(suffix);\n> >  \tif (len < suflen)\n> >  \t\treturn -1;\n> >  \telse\n> > -\t\treturn strcmp(str + len - suflen, suffix);\n> > +\t\treturn !!strcmp(str + len - suflen, suffix);\n> >  }\n> >\n> > I am not absolutely sure about doing the same to prefixcmp(),\n> > though. It could be used for ordering, even though no existing code\n> > seems to do so.\n> \n> I just realized why this suggestion is incomplete; if we were to go\n> this route, we should rename the function to has_suffix() or\n> something. anything-cmp() ought to be usable as an ordering\n> comparison function, but suffixcmp() clearly isn't.\n\nI have to admit that I do not understand why a change in suffixcmp()'s\nbehavior is needed.\n\nCiao,\nJohannes\n"},{"id":"230098","messageId":"xmqqtxfraj52.fsf@gitster.dls.corp.google.com","threadId":"35269","inReplyTo":"alpine.DEB.1.00.1311042316520.1191@s15462909.onlinehome-server.info","subject":"Re: [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-04T22:37:13Z","receivedAt":"2013-11-04T22:37:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Junio,\n>\n> On Mon, 4 Nov 2013, Junio C Hamano wrote:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>> > I do not think anybody sane uses prefixcmp() or suffixcmp() for\n>> > anything but checking with zero; in other words, I suspect that all\n>> > uses of Xcmp() can be replaced with !!Xcmp(), so as a separate\n>> > clean-up patch, we may at least want to make it clear that the\n>> > callers should not expect anything but \"does str have sfx as its\n>> > suffix, yes or no?\" by doing something like this:\n>> >\n>> >  int suffixcmp(const char *str, const char *suffix)\n>> >  {\n>> >  \tint len = strlen(str), suflen = strlen(suffix);\n>> >  \tif (len < suflen)\n>> >  \t\treturn -1;\n>> >  \telse\n>> > -\t\treturn strcmp(str + len - suflen, suffix);\n>> > +\t\treturn !!strcmp(str + len - suflen, suffix);\n>> >  }\n>> >\n>> > I am not absolutely sure about doing the same to prefixcmp(),\n>> > though. It could be used for ordering, even though no existing code\n>> > seems to do so.\n>> \n>> I just realized why this suggestion is incomplete; if we were to go\n>> this route, we should rename the function to has_suffix() or\n>> something. anything-cmp() ought to be usable as an ordering\n>> comparison function, but suffixcmp() clearly isn't.\n>\n> I have to admit that I do not understand why a change in suffixcmp()'s\n> behavior is needed.\n\nI made the suggestion only because I do not understand why the\nfunction should order \"f\" and \".txt\" in the\n\n\t\"f\" < \".txt\"\n\norder. Even worse, the other function postfixcmp() orders them the\nother way around.\n\nIf -1 returned from the function were an indication of error \"The\nstring does not even have that suffix\", then I would have been a bit\nmore sympathetic, and its current behaviour in that case could be\nargued as a special case of the broader return value \"non-zero (from\nthe ordinary strcmp() return codeflow) means the string does not\nhave that suffix and zero means the string ends with the suffix\".\n\nBut then, a function that pretends to be for ordering comparison,\nwith a name that ends with cmp(), and then declaring that \"no, this\nis not for ordering; the sign of the result does not matter--what\nonly matters is if it returns zero or non-zero\", feels quite\nschizophrenic, at least to me.\n\nAnd my earlier suggestion to change the return value *is* not a\nright change.  It still keeps the pretense of comparison for\nordering (i.e. ...cmp() name), while returning a value that cannot\npossibly be used for ordering.\n\nSo I think the right patch should make the function like this:\n\n\tint has_suffix(const char *str, const char *suffix)\n        {\n        \tint len = strlen(str);\n                int suffix_len = strlen(suffix);\n                if (len < suffix_len)\n                \treturn 0;\n\t\treturn !strcmp(str + len - suffix_len, suffix);\n\t}\n"}]}