{"thread":{"id":"44016","subject":"2.10.0: multiple versionsort.prereleasesuffix buggy?","startedAt":"2016-09-05T22:52:41Z","lastAt":"2016-12-20T16:49:18Z","messageCount":40,"participants":["Leho Kraav (Conversion Ready)","Jeff King","SZEDER Gábor","Junio C Hamano","Duy Nguyen","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"301153","messageId":"beba7c55-3f18-66a2-acd9-77f2a5cc55a7@conversionready.com","threadId":"44016","inReplyTo":null,"subject":"2.10.0: multiple versionsort.prereleasesuffix buggy?","fromName":"Leho Kraav (Conversion Ready)","fromEmail":"leho@conversionready.com","sentAt":"2016-09-05T22:42:28Z","receivedAt":"2016-09-05T22:52:41Z","isPatch":false,"sender":{"key":"leho@conversionready.com","avatar":null},"body":"Hi all\n\n\nHere's the testing tree https://github.com/woothemes/woocommerce\n\n.git/config has:\n\n[versionsort] \n \n\n prereleasesuffix = -beta\n prereleasesuffix = -RC\n\n$ git tag -l --sort=version:refname\n...\n2.5.0-RC1\n2.5.0-RC2\n2.5.0-RC3\n2.5.0-beta-1\n2.5.0-beta-2\n2.5.0-beta-3\n2.5.0\n2.5.1\n2.5.2\n2.5.3\n2.5.4\n2.5.5\n2.6.0-RC1\n2.6.0-RC2\n2.6.0-beta-1\n2.6.0-beta-2\n2.6.0-beta-3\n2.6.0-beta-4\n2.6.0\n2.6.1\n2.6.2\n2.6.3\n2.6.4\n\nPer documentation, I'm supposed to see something like\n...\n2.5.0-beta-1\n2.5.0-beta-2\n2.5.0-beta-3\n2.5.0-RC1\n2.5.0-RC2\n2.5.0-RC3\n2.5.0\n...\n\n\nNo matter what I do in `.git/config`, RC goes up front. What's going on?\n\n(Yes, this project's tag capitalization is messed up.)\n"},{"id":"301154","messageId":"20160905232145.fr46rgfb66eeymwp@sigill.intra.peff.net","threadId":"44016","inReplyTo":"beba7c55-3f18-66a2-acd9-77f2a5cc55a7@conversionready.com","subject":"Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-05T23:21:45Z","receivedAt":"2016-09-05T23:21:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 06, 2016 at 01:42:28AM +0300, Leho Kraav (Conversion Ready) wrote:\n\n> Here's the testing tree https://github.com/woothemes/woocommerce\n> \n> .git/config has:\n> \n> [versionsort]\n> \n> \n> prereleasesuffix = -beta\n> prereleasesuffix = -RC\n> \n> $ git tag -l --sort=version:refname\n> [...]\n> 2.6.0-RC1\n> 2.6.0-RC2\n> 2.6.0-beta-1\n> 2.6.0-beta-2\n> 2.6.0-beta-3\n> 2.6.0-beta-4\n\nSo that seems wrong. Even weirder, if I set _only_ \"-beta\", I get:\n\n $ git tag -l --sort=version:refname | grep -v ^2.6.0\n 2.6.0-beta-2\n 2.6.0-beta-3\n 2.6.0-beta-4\n 2.6.0\n 2.6.0-RC1\n 2.6.0-RC2\n 2.6.0-beta-1\n\nUmm...what? beta-1 is sorted away from its companions? That's weird.\n\nI wondered if the presence of \"-\" after the suffix (\"beta-1\" rather than\n\"beta1\") would matter. It looks like that shouldn't matter, though; it's\npurely doing a prefix match on \"do these names differ at a prerelease\nsuffix\".\n\nBut something certainly seems wrong.\n\n-Peff\n"},{"id":"301156","messageId":"20160906010759.14883-1-szeder@ira.uka.de","threadId":"44016","inReplyTo":"20160905232145.fr46rgfb66eeymwp@sigill.intra.peff.net","subject":"Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-09-06T01:07:59Z","receivedAt":"2016-09-06T01:08:31Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n> On Tue, Sep 06, 2016 at 01:42:28AM +0300, Leho Kraav (Conversion Ready) wrote:\n> \n> > Here's the testing tree https://github.com/woothemes/woocommerce\n> > \n> > .git/config has:\n> > \n> > [versionsort]\n> > \n> > \n> > prereleasesuffix = -beta\n> > prereleasesuffix = -RC\n> > \n> > $ git tag -l --sort=version:refname\n> > [...]\n> > 2.6.0-RC1\n> > 2.6.0-RC2\n> > 2.6.0-beta-1\n> > 2.6.0-beta-2\n> > 2.6.0-beta-3\n> > 2.6.0-beta-4\n> \n> So that seems wrong. Even weirder, if I set _only_ \"-beta\", I get:\n> \n> $ git tag -l --sort=version:refname | grep -v ^2.6.0\n> 2.6.0-beta-2\n> 2.6.0-beta-3\n> 2.6.0-beta-4\n> 2.6.0\n> 2.6.0-RC1\n> 2.6.0-RC2\n> 2.6.0-beta-1\n> \n> Umm...what? beta-1 is sorted away from its companions? That's weird.\n> \n> I wondered if the presence of \"-\" after the suffix (\"beta-1\" rather than\n> \"beta1\") would matter. It looks like that shouldn't matter, though; it's\n> purely doing a prefix match on \"do these names differ at a prerelease\n> suffix\".\n> \n> But something certainly seems wrong.\n\nSome of the weirdness is caused by the '-' at the _beginning_ of the\nsuffixes, because versioncmp() gets confused by suffixes starting with\nthe same character(s).\n\nversioncmp() consumes two tagnames up to the first different character\nand then calls swap_prereleases() to try to match prerelease suffixes\nstarting at those characters. This works fine when comparing a\nrelease with a prerelease, e.g. \"2.6.0\" and \"2.6.0-RC1\", because\nswap_prereleases() gets \"\" and \"-RC1\" and the latter does match one of\nthe configured suffixes. However, when comparing two prereleases,\ne.g. \"2.6.0-beta1\" and \"2.6.0-RC1\", then the '-' is consumed from both\ntagnames because the first differing characters are 'b' and 'R', thus\nswap_prereleases() gets \"beta1\" and \"RC1\", which, of course, don't\nmatch any of the configured suffixes without the leading '-'.\n\nIt's way past my bedtime, so for the time being I can only come up\nwith a hacky configuration workaround that seems to deliver the\nexpected results:\n\n[versionsort]\n\tprereleasesuffix = beta\n\tprereleasesuffix = -beta\n\tprereleasesuffix = RC\n\tprereleasesuffix = -RC\n\nBest,\nGábor\n\n"},{"id":"301162","messageId":"20160906040739.37otpk3l2wt7qfbb@sigill.intra.peff.net","threadId":"44016","inReplyTo":"20160906010759.14883-1-szeder@ira.uka.de","subject":"Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-06T04:07:39Z","receivedAt":"2016-09-06T04:07:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 06, 2016 at 03:07:59AM +0200, SZEDER Gábor wrote:\n\n> > So that seems wrong. Even weirder, if I set _only_ \"-beta\", I get:\n> > \n> > $ git tag -l --sort=version:refname | grep -v ^2.6.0\n> > 2.6.0-beta-2\n> > 2.6.0-beta-3\n> > 2.6.0-beta-4\n> > 2.6.0\n> > 2.6.0-RC1\n> > 2.6.0-RC2\n> > 2.6.0-beta-1\n> > \n> > Umm...what? beta-1 is sorted away from its companions? That's weird.\n> > \n> > I wondered if the presence of \"-\" after the suffix (\"beta-1\" rather than\n> > \"beta1\") would matter. It looks like that shouldn't matter, though; it's\n> > purely doing a prefix match on \"do these names differ at a prerelease\n> > suffix\".\n> > \n> > But something certainly seems wrong.\n> \n> Some of the weirdness is caused by the '-' at the _beginning_ of the\n> suffixes, because versioncmp() gets confused by suffixes starting with\n> the same character(s).\n\nOh, right, that makes sense. So it's effectively not finding _any_\nsuffix between X-RC1 and X-beta-1, because we only start looking after\n\"X-\", and none of them match.\n\nI am still confused why \"2.6.0-beta-1\" doesn't get sorted with its\npeers. I'd guess that the comparison function doesn't actually provide a\nstrict ordering, so the results depend on the actual sort algorithm, and\nwhich pairs it ends up comparing.\n\n-Peff\n"},{"id":"301169","messageId":"d95c133b-7d8d-b9f1-7f3b-bcdd4b24aca1@conversionready.com","threadId":"44016","inReplyTo":"20160906010759.14883-1-szeder@ira.uka.de","subject":"Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?","fromName":"Leho Kraav (Conversion Ready)","fromEmail":"leho@conversionready.com","sentAt":"2016-09-06T07:12:15Z","receivedAt":"2016-09-06T07:12:36Z","isPatch":false,"sender":{"key":"leho@conversionready.com","avatar":null},"body":"On 06.09.2016 04:07, SZEDER Gábor wrote:\n>\n> [versionsort]\n> \tprereleasesuffix = beta\n> \tprereleasesuffix = -beta\n> \tprereleasesuffix = RC\n> \tprereleasesuffix = -RC\n>\n> Best,\n> Gábor\n\nYes, yes you are the best. Workaround works, tyvm. I was heading in that \ndirection, too, but never thought to remove leading dash on the \nalternates - instead I tried \"-b\", \"-R\" and similar just to see what \nhappens.\n\n"},{"id":"301229","messageId":"20160906214550.Horde.ducOghtmsQb9pQ6lixxddVz@webmail.informatik.kit.edu","threadId":"44016","inReplyTo":"20160906040739.37otpk3l2wt7qfbb@sigill.intra.peff.net","subject":"Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-09-06T19:45:50Z","receivedAt":"2016-09-06T19:46:00Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nHi,\n\n\nQuoting Jeff King :\n\n> On Tue, Sep 06, 2016 at 03:07:59AM +0200, SZEDER Gábor wrote:\n>\n>>> So that seems wrong. Even weirder, if I set _only_ \"-beta\", I get:\n>>>\n>>> $ git tag -l --sort=version:refname | grep -v ^2.6.0\n>>> 2.6.0-beta-2\n>>> 2.6.0-beta-3\n>>> 2.6.0-beta-4\n>>> 2.6.0\n>>> 2.6.0-RC1\n>>> 2.6.0-RC2\n>>> 2.6.0-beta-1\n>>>\n>>> Umm...what? beta-1 is sorted away from its companions? That's weird.\n>>>\n>>> I wondered if the presence of \"-\" after the suffix (\"beta-1\" rather than\n>>> \"beta1\") would matter. It looks like that shouldn't matter, though; it's\n>>> purely doing a prefix match on \"do these names differ at a prerelease\n>>> suffix\".\n>>>\n>>> But something certainly seems wrong.\n>>\n>> Some of the weirdness is caused by the '-' at the _beginning_ of the\n>> suffixes, because versioncmp() gets confused by suffixes starting with\n>> the same character(s).\n>\n> Oh, right, that makes sense. So it's effectively not finding _any_\n> suffix between X-RC1 and X-beta-1, because we only start looking after\n> \"X-\", and none of them match.\n>\n> I am still confused why \"2.6.0-beta-1\" doesn't get sorted with its\n> peers. I'd guess that the comparison function doesn't actually provide a\n> strict ordering, so the results depend on the actual sort algorithm, and\n> which pairs it ends up comparing.\n\nTurns out that this weirdness is caused by that leading '-' in the suffix,\ntoo.\n\nHere is a manageably small recipe to reproduce:\n\n $ git -c versionsort.prereleasesuffix=-beta tag -l \n--sort=version:refname v2.1.0* v2.1.{1,2}\n v2.1.0-beta-2\n v2.1.0-beta-3\n v2.1.0\n v2.1.0-RC1\n v2.1.0-RC2\n v2.1.0-beta-1\n v2.1.1\n v2.1.2\n\nTracing which pairs of tagnames are compared, I found that somewhere along\nthe line \"v2.1.0-beta-1\" happens to be compared to \"v2.1.0-RC2\", and the\nissue described in my previous email strikes again: the '-' is part of the\ncommon part of the two tagnames, swap_prereleases() gets only \"beta-1\" and\n\"RC2\", thus it can't match the configured \"-beta\" suffix, and since the\nbyte value of 'b' is higher than that of 'R', \"-beta-1\" is sorted after\n\"-RC2\". OTOH, \"v2.1.0-beta-2\" and \"v2.1.0-beta-3\" are only compared to\neach other or to final release tags, but never to any \"-RCx\" tags, hence\nthey are sorted properly.\n\nOnce I finish teaching versioncmp() and swap_prereleases() to cope with\nleading characters of a prereleaseSuffix being part of the common part of\ntwo tagnames, this out-of-order \"beta-1\" issue will be gone as well.\n\nBest,\nGábor\n"},{"id":"301279","messageId":"20160907151251.30978-1-szeder@ira.uka.de","threadId":"44016","inReplyTo":"20160906214550.Horde.ducOghtmsQb9pQ6lixxddVz@webmail.informatik.kit.edu","subject":"[PATCH 0/5] Fix version sort prerelease reordering bug","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-09-07T15:12:46Z","receivedAt":"2016-09-07T15:13:20Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"(Sorry for double post, forgot to Cc: the mailing list...)\n\nThis series fixes a bug, where version sort with prerelease reordering\nputs tagnames in the wrong order, when the common part of two compared\ntagnames ends with the leading character(s) of one or more configured\nprerelease suffixes. More details in the final patch.\n\nThe first two patches are test cleanups, the first is an independent\n\"while at it\", but the second one touches tests that are modified by\nlater patches of this series. The rest is rather straightforward: add\nfailing tests, do some refactoring, and finally fix the bug.\n\nSZEDER Gábor (5):\n t7004-tag: delete unnecessary tags with test_when_finished\n t7004-tag: use test_config helper\n t7004-tag: add version sort tests to show prerelease reordering issues\n versioncmp: pass full tagnames to swap_prereleases()\n versioncmp: cope with common leading parts in\n versionsort.prereleaseSuffix\n\n t/t7004-tag.sh | 83 ++++++++++++++++++++++++++++++++++++++++------------------\n versioncmp.c | 46 +++++++++++++++++++++-----------\n 2 files changed, 87 insertions(+), 42 deletions(-)\n\n-- \n2.10.0.74.g6632b1b\n\n"},{"id":"301280","messageId":"20160907151251.30978-2-szeder@ira.uka.de","threadId":"44016","inReplyTo":"20160907151251.30978-1-szeder@ira.uka.de","subject":"[PATCH 1/5] t7004-tag: delete unnecessary tags with test_when_finished","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-09-07T15:12:47Z","receivedAt":"2016-09-07T15:13:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The '--force is moot with a non-existing tag name' test creates two\nnew tags, which are then deleted right after the test is finished,\noutside the test_expect_success block, allowing 'git tag -d's output to\npollute the test output.\n\nUse test_when_finished to delete those tags.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t7004-tag.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 8b0f71a2ac15..396cffeeb5ad 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -122,11 +122,11 @@ test_expect_success '--force can create a tag with the name of one existing' '\n \ttag_exists mytag'\n \n test_expect_success '--force is moot with a non-existing tag name' '\n+\ttest_when_finished git tag -d newtag forcetag &&\n \tgit tag newtag >expect &&\n \tgit tag --force forcetag >actual &&\n \ttest_cmp expect actual\n '\n-git tag -d newtag forcetag\n \n # deleting tags:\n \n-- \n2.10.0.74.g6632b1b\n\n"},{"id":"301281","messageId":"20160907151251.30978-3-szeder@ira.uka.de","threadId":"44016","inReplyTo":"20160907151251.30978-1-szeder@ira.uka.de","subject":"[PATCH 2/5] t7004-tag: use test_config helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-09-07T15:12:48Z","receivedAt":"2016-09-07T15:13:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"... instead of setting and then manually unsetting configuration\nvariables, on one occasion even outside the test_expect_success block.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t7004-tag.sh | 23 ++++++++++-------------\n 1 file changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 396cffeeb5ad..920a1b4b2e58 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -297,11 +297,9 @@ EOF\n '\n \n test_expect_success 'listing tags in column with column.*' '\n-\tgit config column.tag row &&\n-\tgit config column.ui dense &&\n+\ttest_config column.tag row &&\n+\ttest_config column.ui dense &&\n \tCOLUMNS=40 git tag -l >actual &&\n-\tgit config --unset column.ui &&\n-\tgit config --unset column.tag &&\n \tcat >expected <<\\EOF &&\n a1 aa1 cba t210 t211\n v0.2.1 v1.0 v1.0.1 v1.1.3\n@@ -314,9 +312,8 @@ test_expect_success 'listing tag with -n --column should fail' '\n '\n \n test_expect_success 'listing tags -n in column with column.ui ignored' '\n-\tgit config column.ui \"row dense\" &&\n+\ttest_config column.ui \"row dense\" &&\n \tCOLUMNS=40 git tag -l -n >actual &&\n-\tgit config --unset column.ui &&\n \tcat >expected <<\\EOF &&\n a1 Foo\n aa1 Foo\n@@ -1200,11 +1197,10 @@ test_expect_success GPG,RFC1991 \\\n '\n \n # try to sign with bad user.signingkey\n-git config user.signingkey BobTheMouse\n test_expect_success GPG \\\n \t'git tag -s fails if gpg is misconfigured (bad key)' \\\n-\t'test_must_fail git tag -s -m tail tag-gpg-failure'\n-git config --unset user.signingkey\n+\t'test_config user.signingkey BobTheMouse &&\n+\ttest_must_fail git tag -s -m tail tag-gpg-failure'\n \n # try to produce invalid signature\n test_expect_success GPG \\\n@@ -1484,7 +1480,7 @@ test_expect_success 'reverse lexical sort' '\n '\n \n test_expect_success 'configured lexical sort' '\n-\tgit config tag.sort \"v:refname\" &&\n+\ttest_config tag.sort \"v:refname\" &&\n \tgit tag -l \"foo*\" >actual &&\n \tcat >expect <<-\\EOF &&\n \tfoo1.3\n@@ -1495,6 +1491,7 @@ test_expect_success 'configured lexical sort' '\n '\n \n test_expect_success 'option override configured sort' '\n+\ttest_config tag.sort \"v:refname\" &&\n \tgit tag -l --sort=-refname \"foo*\" >actual &&\n \tcat >expect <<-\\EOF &&\n \tfoo1.6\n@@ -1509,13 +1506,12 @@ test_expect_success 'invalid sort parameter on command line' '\n '\n \n test_expect_success 'invalid sort parameter in configuratoin' '\n-\tgit config tag.sort \"v:notvalid\" &&\n+\ttest_config tag.sort \"v:notvalid\" &&\n \ttest_must_fail git tag -l \"foo*\"\n '\n \n test_expect_success 'version sort with prerelease reordering' '\n-\tgit config --unset tag.sort &&\n-\tgit config versionsort.prereleaseSuffix -rc &&\n+\ttest_config versionsort.prereleaseSuffix -rc &&\n \tgit tag foo1.6-rc1 &&\n \tgit tag foo1.6-rc2 &&\n \tgit tag -l --sort=version:refname \"foo*\" >actual &&\n@@ -1530,6 +1526,7 @@ test_expect_success 'version sort with prerelease reordering' '\n '\n \n test_expect_success 'reverse version sort with prerelease reordering' '\n+\ttest_config versionsort.prereleaseSuffix -rc &&\n \tgit tag -l --sort=-version:refname \"foo*\" >actual &&\n \tcat >expect <<-\\EOF &&\n \tfoo1.10\n-- \n2.10.0.74.g6632b1b\n\n"},{"id":"301282","messageId":"20160907151251.30978-5-szeder@ira.uka.de","threadId":"44016","inReplyTo":"20160907151251.30978-1-szeder@ira.uka.de","subject":"[PATCH 4/5] versioncmp: pass full tagnames to swap_prereleases()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-09-07T15:12:50Z","receivedAt":"2016-09-07T15:13:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The swap_prereleases() helper function is responsible for finding\nconfigured prerelease suffixes in a pair of tagnames to be compared,\nbut this function currently only gets to see only the parts of those\ntwo tagnames starting at the first different character. To fix some\nissues related to multiple prerelease suffixes starting with the same\nleading character(s), this helper function must see the preceeding\nmatching characters as well.\n\nIn preparation for the fix in the following patch, refactor\nswap_prereleases() and its caller to pass two full tagnames and an\nadditional offset indicating the position of the first different\ncharacter.\n\nSigned-off-by: SZEDER Gábor \n---\n versioncmp.c | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/versioncmp.c b/versioncmp.c\nindex 80bfd109fa12..fed02d2a2878 100644\n--- a/versioncmp.c\n+++ b/versioncmp.c\n@@ -25,32 +25,31 @@ static const struct string_list *prereleases;\n static int initialized;\n \n /*\n- * p1 and p2 point to the first different character in two strings. If\n- * either p1 or p2 starts with a prerelease suffix, it will be forced\n- * to be on top.\n+ * off is the offset of the first different character in the two strings\n+ * s1 and s2. If either s1 or s2 contains a prerelease suffix starting\n+ * at that offset, it will be forced to be on top.\n *\n- * If both p1 and p2 start with (different) suffix, the order is\n- * determined by config file.\n+ * If both s1 and s2 contain a (different) suffix at that position, the\n+ * order is determined by config file.\n *\n- * Note that we don't have to deal with the situation when both p1 and\n- * p2 start with the same suffix because the common part is already\n+ * Note that we don't have to deal with the situation when both s1 and\n+ * s2 contain the same suffix because the common part is already\n * consumed by the caller.\n *\n * Return non-zero if *diff contains the return value for versioncmp()\n */\n-static int swap_prereleases(const void *p1_,\n-\t\t\t const void *p2_,\n+static int swap_prereleases(const char *s1,\n+\t\t\t const char *s2,\n+\t\t\t int off,\n \t\t\t int *diff)\n {\n-\tconst char *p1 = p1_;\n-\tconst char *p2 = p2_;\n \tint i, i1 = -1, i2 = -1;\n \n \tfor (i = 0; i < prereleases->nr; i++) {\n \t\tconst char *suffix = prereleases->items[i].string;\n-\t\tif (i1 == -1 && starts_with(p1, suffix))\n+\t\tif (i1 == -1 && starts_with(s1 + off, suffix))\n \t\t\ti1 = i;\n-\t\tif (i2 == -1 && starts_with(p2, suffix))\n+\t\tif (i2 == -1 && starts_with(s2 + off, suffix))\n \t\t\ti2 = i;\n \t}\n \tif (i1 == -1 && i2 == -1)\n@@ -121,7 +120,8 @@ int versioncmp(const char *s1, const char *s2)\n \t\tinitialized = 1;\n \t\tprereleases = git_config_get_value_multi(\"versionsort.prereleasesuffix\");\n \t}\n-\tif (prereleases && swap_prereleases(p1 - 1, p2 - 1, &diff))\n+\tif (prereleases && swap_prereleases(s1, s2, (const char *) p1 - s1 - 1,\n+\t\t\t\t\t &diff))\n \t\treturn diff;\n \n \tstate = result_type[state * 3 + (((c2 == '0') + (isdigit (c2) != 0)))];\n-- \n2.10.0.74.g6632b1b\n\n"},{"id":"301283","messageId":"20160907151251.30978-4-szeder@ira.uka.de","threadId":"44016","inReplyTo":"20160907151251.30978-1-szeder@ira.uka.de","subject":"[PATCH 3/5] t7004-tag: add version sort tests to show prerelease reordering issues","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-09-07T15:12:49Z","receivedAt":"2016-09-07T15:13:41Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Version sort with prerelease reordering sometimes puts tagnames in the\nwrong order, when the common part of two compared tagnames ends with\nthe leading character(s) of one or more configured prerelease\nsuffixes. Add tests that demonstrate these issues.\n\nThe unrelated '--format should list tags as per format given' test\nlater uses tags matching the same prefix as the version sort tests,\nthus was affected by the new tags added for the new tests in this\npatch. Change that test to perform its checks on a different set of\ntags.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t7004-tag.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 48 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 920a1b4b2e58..d69ac4940388 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1511,14 +1511,14 @@ test_expect_success 'invalid sort parameter in configuratoin' '\n '\n \n test_expect_success 'version sort with prerelease reordering' '\n-\ttest_config versionsort.prereleaseSuffix -rc &&\n-\tgit tag foo1.6-rc1 &&\n-\tgit tag foo1.6-rc2 &&\n+\ttest_config versionsort.prereleaseSuffix -beta &&\n+\tgit tag foo1.6-beta1 &&\n+\tgit tag foo1.6-beta2 &&\n \tgit tag -l --sort=version:refname \"foo*\" >actual &&\n \tcat >expect <<-\\EOF &&\n \tfoo1.3\n-\tfoo1.6-rc1\n-\tfoo1.6-rc2\n+\tfoo1.6-beta1\n+\tfoo1.6-beta2\n \tfoo1.6\n \tfoo1.10\n \tEOF\n@@ -1526,18 +1526,54 @@ test_expect_success 'version sort with prerelease reordering' '\n '\n \n test_expect_success 'reverse version sort with prerelease reordering' '\n-\ttest_config versionsort.prereleaseSuffix -rc &&\n+\ttest_config versionsort.prereleaseSuffix -beta &&\n \tgit tag -l --sort=-version:refname \"foo*\" >actual &&\n \tcat >expect <<-\\EOF &&\n \tfoo1.10\n \tfoo1.6\n-\tfoo1.6-rc2\n-\tfoo1.6-rc1\n+\tfoo1.6-beta2\n+\tfoo1.6-beta1\n \tfoo1.3\n \tEOF\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'version sort with prerelease reordering and common leading character' '\n+\ttest_config versionsort.prereleaseSuffix -beta &&\n+\tgit tag foo1.6-after1 &&\n+\tgit tag -l --sort=version:refname \"foo*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.3\n+\tfoo1.6-beta1\n+\tfoo1.6-beta2\n+\tfoo1.6\n+\tfoo1.6-after1\n+\tfoo1.10\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+# Capitalization of suffixes is important here, because \"-RC\" would normally\n+# be sorted before \"-beta\" and the config settings should override that.\n+test_expect_failure 'version sort with prerelease reordering, multiple suffixes and common leading character' '\n+\ttest_config versionsort.prereleaseSuffix -beta &&\n+\tgit config --add versionsort.prereleaseSuffix -RC &&\n+\tgit tag foo1.6-RC1 &&\n+\tgit tag foo1.6-RC2 &&\n+\tgit tag -l --sort=version:refname \"foo*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.3\n+\tfoo1.6-beta1\n+\tfoo1.6-beta2\n+\tfoo1.6-RC1\n+\tfoo1.6-RC2\n+\tfoo1.6\n+\tfoo1.6-after1\n+\tfoo1.10\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n run_with_limited_stack () {\n \t(ulimit -s 128 && \"$@\")\n }\n@@ -1566,13 +1602,11 @@ EOF\"\n \n test_expect_success '--format should list tags as per format given' '\n \tcat >expect <<-\\EOF &&\n-\trefname : refs/tags/foo1.10\n-\trefname : refs/tags/foo1.3\n-\trefname : refs/tags/foo1.6\n-\trefname : refs/tags/foo1.6-rc1\n-\trefname : refs/tags/foo1.6-rc2\n+\trefname : refs/tags/v1.0\n+\trefname : refs/tags/v1.0.1\n+\trefname : refs/tags/v1.1.3\n \tEOF\n-\tgit tag -l --format=\"refname : %(refname)\" \"foo*\" >actual &&\n+\tgit tag -l --format=\"refname : %(refname)\" \"v1*\" >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.10.0.74.g6632b1b\n\n"},{"id":"301284","messageId":"20160907151251.30978-6-szeder@ira.uka.de","threadId":"44016","inReplyTo":"20160907151251.30978-1-szeder@ira.uka.de","subject":"[PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-09-07T15:12:51Z","receivedAt":"2016-09-07T15:13:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Version sort with prerelease reordering sometimes puts tagnames in the\nwrong order, when the common part of two compared tagnames ends with\nthe leading character(s) of one or more configured prerelease\nsuffixes.\n\n $ git config --get-all versionsort.prereleaseSuffix\n -beta\n $ git tag -l --sort=version:refname v2.1.*\n v2.1.0-beta-2\n v2.1.0-beta-3\n v2.1.0\n v2.1.0-RC1\n v2.1.0-RC2\n v2.1.0-beta-1\n v2.1.1\n v2.1.2\n\nThe reason is that when comparing a pair of tagnames, first\nversioncmp() looks for the first different character in a pair of\ntagnames, and then the swap_prereleases() helper function checks for\nprerelease suffixes _starting at_ that character. Thus, when in the\nabove example the sorting algorithm happens to compare the tagnames\n\"v2.1.0-beta-1\" and \"v2.1.0-RC2\", swap_prereleases() will try to match\nthe suffix \"-beta\" against \"beta-1\" to no avail, and the two tagnames\nerroneously end up being ordered lexicographically.\n\nTo fix this issue change swap_prereleases() to look for configured\nprerelease suffixes containing that first different character.\n\nReported-by: Leho Kraav \nSigned-off-by: SZEDER Gábor \n---\n t/t7004-tag.sh | 4 ++--\n versioncmp.c | 24 +++++++++++++++++++-----\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex d69ac4940388..f0cfe1fa3d8b 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1538,7 +1538,7 @@ test_expect_success 'reverse version sort with prerelease reordering' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'version sort with prerelease reordering and common leading character' '\n+test_expect_success 'version sort with prerelease reordering and common leading character' '\n \ttest_config versionsort.prereleaseSuffix -beta &&\n \tgit tag foo1.6-after1 &&\n \tgit tag -l --sort=version:refname \"foo*\" >actual &&\n@@ -1555,7 +1555,7 @@ test_expect_failure 'version sort with prerelease reordering and common leading\n \n # Capitalization of suffixes is important here, because \"-RC\" would normally\n # be sorted before \"-beta\" and the config settings should override that.\n-test_expect_failure 'version sort with prerelease reordering, multiple suffixes and common leading character' '\n+test_expect_success 'version sort with prerelease reordering, multiple suffixes and common leading character' '\n \ttest_config versionsort.prereleaseSuffix -beta &&\n \tgit config --add versionsort.prereleaseSuffix -RC &&\n \tgit tag foo1.6-RC1 &&\ndiff --git a/versioncmp.c b/versioncmp.c\nindex fed02d2a2878..87b49a622423 100644\n--- a/versioncmp.c\n+++ b/versioncmp.c\n@@ -27,7 +27,8 @@ static int initialized;\n /*\n * off is the offset of the first different character in the two strings\n * s1 and s2. If either s1 or s2 contains a prerelease suffix starting\n- * at that offset, it will be forced to be on top.\n+ * at that offset or the character at that offset is part of a\n+ * prerelease suffix, then that string will be forced to be on top.\n *\n * If both s1 and s2 contain a (different) suffix at that position, the\n * order is determined by config file.\n@@ -47,10 +48,23 @@ static int swap_prereleases(const char *s1,\n \n \tfor (i = 0; i < prereleases->nr; i++) {\n \t\tconst char *suffix = prereleases->items[i].string;\n-\t\tif (i1 == -1 && starts_with(s1 + off, suffix))\n-\t\t\ti1 = i;\n-\t\tif (i2 == -1 && starts_with(s2 + off, suffix))\n-\t\t\ti2 = i;\n+\t\tint j, start, suffix_len = strlen(suffix);\n+\t\tif (suffix_len < off)\n+\t\t\tstart = off - suffix_len + 1;\n+\t\telse\n+\t\t\tstart = 0;\n+\t\tfor (j = start; j <= off; j++) {\n+\t\t\tif (i1 == -1 && starts_with(s1 + j, suffix)) {\n+\t\t\t\ti1 = i;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tfor (j = start; j <= off; j++) {\n+\t\t\tif (i2 == -1 && starts_with(s2 + j, suffix)) {\n+\t\t\t\ti2 = i;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \t}\n \tif (i1 == -1 && i2 == -1)\n \t\treturn 0;\n-- \n2.10.0.74.g6632b1b\n\n"},{"id":"301286","messageId":"20160907174841.Horde.Ru1LBEeLKomznlWVG-ZnS-Q@webmail.informatik.kit.edu","threadId":"44016","inReplyTo":"20160907151251.30978-6-szeder@ira.uka.de","subject":"Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-09-07T15:48:41Z","receivedAt":"2016-09-07T15:49:14Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting SZEDER Gábor :\n\n> Version sort with prerelease reordering sometimes puts tagnames in the\n> wrong order, when the common part of two compared tagnames ends with\n> the leading character(s) of one or more configured prerelease\n> suffixes.\n>\n> $ git config --get-all versionsort.prereleaseSuffix\n> -beta\n> $ git tag -l --sort=version:refname v2.1.*\n> v2.1.0-beta-2\n> v2.1.0-beta-3\n> v2.1.0\n> v2.1.0-RC1\n> v2.1.0-RC2\n> v2.1.0-beta-1\n> v2.1.1\n> v2.1.2\n>\n> The reason is that when comparing a pair of tagnames, first\n> versioncmp() looks for the first different character in a pair of\n> tagnames, and then the swap_prereleases() helper function checks for\n> prerelease suffixes _starting at_ that character. Thus, when in the\n> above example the sorting algorithm happens to compare the tagnames\n> \"v2.1.0-beta-1\" and \"v2.1.0-RC2\", swap_prereleases() will try to match\n> the suffix \"-beta\" against \"beta-1\" to no avail, and the two tagnames\n> erroneously end up being ordered lexicographically.\n>\n> To fix this issue change swap_prereleases() to look for configured\n> prerelease suffixes containing that first different character.\n\nNow, while I believe this is the right thing to do to fix this bug,\nthere is a corner case, where multiple configured prerelease suffixes\nmight match the same tagname:\n\n $ git config --get-all versionsort.prereleaseSuffix\n -bar\n -baz\n -foo-bar\n $ ~/src/git/git tag -l --sort=version:refname\n v1.0-foo-bar\n v1.0-foo-baz\n\nI.e. when comparing these two tags, both \"-bar\" and \"-foo-bar\" would\nmatch \"v1.0-foo-bar\", and as \"-bar\" comes first in the config file,\nit wins, and \"v1.0-foo-bar\" is ordered first. An argument could be\nmade to prefer longer matches, in which case \"v1.0-foo-bar\" would be\nordered according to \"-foo-bar\", i.e. as second. However, I don't\nknow what that argument could be, to me neither behavior is better\nthan the other, but the implementation of the \"longest match counts\"\nwould certainly be more complicated.\n\nThe argument I would make is that this is a pathological corner case\nthat doesn't worth worrying about.\n\n\nBest,\nGábor\n"},{"id":"301413","messageId":"xmqq7fami8nj.fsf@gitster.mtv.corp.google.com","threadId":"44016","inReplyTo":"20160907151251.30978-5-szeder@ira.uka.de","subject":"Re: [PATCH 4/5] versioncmp: pass full tagnames to swap_prereleases()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T17:49:36Z","receivedAt":"2016-09-08T17:49:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> - * Note that we don't have to deal with the situation when both p1 and\n> - * p2 start with the same suffix because the common part is already\n> + * Note that we don't have to deal with the situation when both s1 and\n> + * s2 contain the same suffix because the common part is already\n> * consumed by the caller.\n\n\"The common part is already consumed\" was relevant while the\nfunction was fed p1 and p2, i.e. the first difference, but the whole\npoint of passing the original s1 and s2 with ofs is so that the\nfunction can look behind ofs as necessary. Is \"already consumed\"\nstill correct (or relevant) with s/p/s/ you did to its calling\nconvention?\n\n"},{"id":"301444","messageId":"20160908223727.Horde.jVOOJ278ssZ3qkyjkmyqZD-@webmail.informatik.kit.edu","threadId":"44016","inReplyTo":"xmqq7fami8nj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/5] versioncmp: pass full tagnames to swap_prereleases()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-09-08T20:37:27Z","receivedAt":"2016-09-08T20:37:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n> SZEDER Gábor writes:\n>\n>> - * Note that we don't have to deal with the situation when both p1 and\n>> - * p2 start with the same suffix because the common part is already\n>> + * Note that we don't have to deal with the situation when both s1 and\n>> + * s2 contain the same suffix because the common part is already\n>> * consumed by the caller.\n>\n> \"The common part is already consumed\" was relevant while the\n> function was fed p1 and p2, i.e. the first difference, but the whole\n> point of passing the original s1 and s2 with ofs is so that the\n> function can look behind ofs as necessary. Is \"already consumed\"\n> still correct (or relevant) with s/p/s/ you did to its calling\n> convention?\n\nWell, it's still correct in the sense that we don't have to worry about\nfinding the same suffix in both strings. However, \"consume\" is not the\nright word to use here, as incrementing an offset until it points past\nthe common part doesn't count as \"consumption\", so more rewording would\nbe necessary.\n\nI'm not sure about the relevancy of this pararaph, or the relevancy of\nthe original version for that matter. I mean, there is a different\ncharacter for sure, so it's really rather obvious that it can't\npossibly be the same suffix in both, isn't it? So I don't think it\nadds much value, and don't mind deleting it in the reroll.\n\n\nBest,\nGábor\n\n"},{"id":"301454","messageId":"xmqq4m5qf588.fsf@gitster.mtv.corp.google.com","threadId":"44016","inReplyTo":"20160908223727.Horde.jVOOJ278ssZ3qkyjkmyqZD-@webmail.informatik.kit.edu","subject":"Re: [PATCH 4/5] versioncmp: pass full tagnames to swap_prereleases()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T21:31:51Z","receivedAt":"2016-09-08T21:31:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> I'm not sure about the relevancy of this pararaph, or the relevancy of\n> the original version for that matter. I mean, there is a different\n> character for sure, so it's really rather obvious that it can't\n> possibly be the same suffix in both, isn't it? So I don't think it\n> adds much value, and don't mind deleting it in the reroll.\n\nConcurred. Let's lose this confusing statement.\n\nThanks.\n"},{"id":"301482","messageId":"CACsJy8DZtAOL9OA_6eqKN7bfS2up+LLEYOWDg=Nh0Quy3coAVQ@mail.gmail.com","threadId":"44016","inReplyTo":"20160907174841.Horde.Ru1LBEeLKomznlWVG-ZnS-Q@webmail.informatik.kit.edu","subject":"Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-09-09T10:43:25Z","receivedAt":"2016-09-09T10:44:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 7, 2016 at 10:48 PM, SZEDER Gábor wrote:\n> Now, while I believe this is the right thing to do to fix this bug,\n> there is a corner case, where multiple configured prerelease suffixes\n> might match the same tagname:\n>\n> $ git config --get-all versionsort.prereleaseSuffix\n> -bar\n> -baz\n> -foo-bar\n> $ ~/src/git/git tag -l --sort=version:refname\n> v1.0-foo-bar\n> v1.0-foo-baz\n>\n> I.e. when comparing these two tags, both \"-bar\" and \"-foo-bar\" would\n> match \"v1.0-foo-bar\", and as \"-bar\" comes first in the config file,\n> it wins, and \"v1.0-foo-bar\" is ordered first. An argument could be\n> made to prefer longer matches, in which case \"v1.0-foo-bar\" would be\n> ordered according to \"-foo-bar\", i.e. as second. However, I don't\n> know what that argument could be, to me neither behavior is better\n> than the other, but the implementation of the \"longest match counts\"\n> would certainly be more complicated.\n>\n> The argument I would make is that this is a pathological corner case\n> that doesn't worth worrying about.\n\nMaybe we should keep a note about this in config.txt? If it's not\nworth bothering/scaring the users about, I suggest you keep this in\nthe commit message.\n-- \nDuy\n"},{"id":"303336","messageId":"20161005033353.Horde.33pf2naqnF4HgwPWSy9DaHV@webmail.informatik.kit.edu","threadId":"44016","inReplyTo":"20160907174841.Horde.Ru1LBEeLKomznlWVG-ZnS-Q@webmail.informatik.kit.edu","subject":"Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-10-05T01:33:53Z","receivedAt":"2016-10-05T01:34:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting SZEDER Gábor :\n\n> Quoting SZEDER Gábor :\n>\n>> Version sort with prerelease reordering sometimes puts tagnames in the\n>> wrong order, when the common part of two compared tagnames ends with\n>> the leading character(s) of one or more configured prerelease\n>> suffixes.\n>>\n>> $ git config --get-all versionsort.prereleaseSuffix\n>> -beta\n>> $ git tag -l --sort=version:refname v2.1.*\n>> v2.1.0-beta-2\n>> v2.1.0-beta-3\n>> v2.1.0\n>> v2.1.0-RC1\n>> v2.1.0-RC2\n>> v2.1.0-beta-1\n>> v2.1.1\n>> v2.1.2\n>>\n>> The reason is that when comparing a pair of tagnames, first\n>> versioncmp() looks for the first different character in a pair of\n>> tagnames, and then the swap_prereleases() helper function checks for\n>> prerelease suffixes _starting at_ that character. Thus, when in the\n>> above example the sorting algorithm happens to compare the tagnames\n>> \"v2.1.0-beta-1\" and \"v2.1.0-RC2\", swap_prereleases() will try to match\n>> the suffix \"-beta\" against \"beta-1\" to no avail, and the two tagnames\n>> erroneously end up being ordered lexicographically.\n>>\n>> To fix this issue change swap_prereleases() to look for configured\n>> prerelease suffixes containing that first different character.\n>\n> Now, while I believe this is the right thing to do to fix this bug,\n> there is a corner case, where multiple configured prerelease suffixes\n> might match the same tagname:\n>\n> $ git config --get-all versionsort.prereleaseSuffix\n> -bar\n> -baz\n> -foo-bar\n> $ ~/src/git/git tag -l --sort=version:refname\n> v1.0-foo-bar\n> v1.0-foo-baz\n>\n> I.e. when comparing these two tags, both \"-bar\" and \"-foo-bar\" would\n> match \"v1.0-foo-bar\", and as \"-bar\" comes first in the config file,\n> it wins, and \"v1.0-foo-bar\" is ordered first. An argument could be\n> made to prefer longer matches, in which case \"v1.0-foo-bar\" would be\n> ordered according to \"-foo-bar\", i.e. as second. However, I don't\n> know what that argument could be, to me neither behavior is better\n> than the other, but the implementation of the \"longest match counts\"\n> would certainly be more complicated.\n>\n> The argument I would make is that this is a pathological corner case\n> that doesn't worth worrying about.\n\nAfter having slept on this a couple of times, I think the longest\nmatching prerelease suffix should determine the sorting order.\n\nA release tag usually consists of an optional prefix, e.g. 'v' or\n'snapshot-', followed by the actual version number, followed by an\noptional suffix. In the contrived example quoted above this suffix\nis '-foo-bar', thus it feels wrong to match '-bar' against it, when\nthe user explicitly configured '-foo-bar' as prerelease suffix as\nwell.\n\nThen here is a more realistic case for sorting based on the longest\nmatching suffix, where\n\n - a longer suffix starts with the shorter one,\n - and the longer suffix comes after the shorter one in the\n configuration.\n\nWith my patches it looks like this:\n\n $ git -c versionsort.prereleasesuffix=-pre \\\n -c versionsort.prereleasesuffix=-prerelease \\\n tag -l --sort=version:refname\n v1.0.0-prerelease1\n v1.0.0-pre1\n v1.0.0-pre2\n v1.0.0\n\nYeah, having both '-pre' and '-prerelease' suffixes seems somewhat\nsilly, but who knows what similar but more reasonable prerelease\nsuffixes e.g. non-English speaking users might use in their native\nlanguage.\n\nAnyway, my intuition says that any '-prereleaseX' tags should come\nafter all the '-preX' tags. (Ironically, current git just happens\nto get this particular case right.)\n\nMy patches get this wrong, because they look for prerelease suffixes\n_containing_ the first different character. However, when a '-preX'\nand a '-prereleaseX' tag are compared, then the whole '-pre' suffix\nis part of the common part, thus it doesn't match, only '-prerelease'\nmatches.\n\nSo, to sort this case right the implementation should\n\n - look for a prerelease suffix containing the first different\n character or ending right before the first different character,\n (This means that when comparing 'v1.0.0-pre1' and 'v1.0.0-pre2'\n swap_prereleases() would match '-pre' in both: no big deal, it\n should return \"undecided\" and let the caller do the right thing\n by sorting based on '1' and '2')\n\n - and sort a tag based on the longest matching prerelease suffix.\n (In my quoted email above I alluded that its implementation must\n be more complicated. No, it turns out that it actually isn't.)\n\n(Just for the record: it's still not 100% foolproof, though. Someone\nasking for trouble might use letters instead of numbers to indicate\nsubsequent prereleases, and might have tags 'v1.0-prea', 'v1.0-preb',\n..., 'v1.0-prer', and 'v1.0-prerelease'. In this case the sorting\nalgorithm might happen to decide to compare 'v1.0-prer' and\n'v1.0-prerelease', and since the common part is 'v1.0-prer' it will\nfail to recognize the '-pre' suffix. I don't see how this case could\nbe supported in a sane way, or why it's worth to support it.)\n\nAnd a final sidenote: sorting based on the longest matching suffix\nalso allows us to (ab)use version sort with prerelease suffixes to\nsort postrelease tags as we please, too:\n\n $ ~/src/git/git -c versionsort.prereleasesuffix=-alpha \\\n -c versionsort.prereleasesuffix=-beta \\\n -c versionsort.prereleasesuffix= \\\n -c versionsort.prereleasesuffix=-gamma \\\n -c versionsort.prereleasesuffix=-delta \\\n tag -l --sort=version:refname 'v3.0*'\n v3.0-alpha1\n v3.0-beta1\n v3.0\n v3.0-gamma1\n v3.0-delta1\n\nSo, unless I hear a counterargument, I will submit a reroll with\nlongest matching suffix determining sort order added on top once I\nget around to finish up its commit message.\n\nBest,\nGábor\n\n"},{"id":"303392","messageId":"xmqq4m4qrapv.fsf@gitster.mtv.corp.google.com","threadId":"44016","inReplyTo":"20161005033353.Horde.33pf2naqnF4HgwPWSy9DaHV@webmail.informatik.kit.edu","subject":"Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-05T17:01:48Z","receivedAt":"2016-10-05T17:01:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> And a final sidenote: sorting based on the longest matching suffix\n> also allows us to (ab)use version sort with prerelease suffixes to\n> sort postrelease tags as we please, too:\n>\n> $ ~/src/git/git -c versionsort.prereleasesuffix=-alpha \\\n> -c versionsort.prereleasesuffix=-beta \\\n> -c versionsort.prereleasesuffix= \\\n> -c versionsort.prereleasesuffix=-gamma \\\n> -c versionsort.prereleasesuffix=-delta \\\n> tag -l --sort=version:refname 'v3.0*'\n> v3.0-alpha1\n> v3.0-beta1\n> v3.0\n> v3.0-gamma1\n> v3.0-delta1\n\nAssuming that gamma and delta are meant to indicate \"these are\npost-release updates\", I think a mechanism that can yield the above\nresult is fantastic ;-)\n"},{"id":"303443","messageId":"20161005232609.Horde.VetzEIKHDJUdcaOod9sHxuK@webmail.informatik.kit.edu","threadId":"44016","inReplyTo":"xmqq4m4qrapv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-10-05T21:26:09Z","receivedAt":"2016-10-05T21:26:20Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n> SZEDER Gábor writes:\n>\n>> And a final sidenote: sorting based on the longest matching suffix\n>> also allows us to (ab)use version sort with prerelease suffixes to\n>> sort postrelease tags as we please, too:\n>>\n>> $ ~/src/git/git -c versionsort.prereleasesuffix=-alpha \\\n>> -c versionsort.prereleasesuffix=-beta \\\n>> -c versionsort.prereleasesuffix= \\\n>> -c versionsort.prereleasesuffix=-gamma \\\n>> -c versionsort.prereleasesuffix=-delta \\\n>> tag -l --sort=version:refname 'v3.0*'\n>> v3.0-alpha1\n>> v3.0-beta1\n>> v3.0\n>> v3.0-gamma1\n>> v3.0-delta1\n>\n> Assuming that gamma and delta are meant to indicate \"these are\n> post-release updates\",\n\nIndeed they were meant as post-release suffixes. Naturally following\nalpha and beta, they were the first to spring to mind that should be\nsorted in non-lexicographical order, so I could show of postrelease\nreordering. It's just that we don't have a config like\n'versionsort.postreleasesuffix', which is half the abuse. The other\nhalf of the abuse is that I had to explicitly indicate the position\nof suffixless versions with an empty suffix between pre- and\npostrelease suffixes. The empty suffix matches on every tag, but\nthen it's overridden by all configured suffixes, so such a version\njust stays in the middle.\n\n> I think a mechanism that can yield the above\n> result is fantastic ;-)\n\nHeh.\n\nGut feeling tells me that I should take this as a subtle\nencouragement to look into adding 'versionsort.postreleasesuffix',\nshouldn't I ;)\n\n"},{"id":"303450","messageId":"xmqqwphmmoi6.fsf@gitster.mtv.corp.google.com","threadId":"44016","inReplyTo":"20161005232609.Horde.VetzEIKHDJUdcaOod9sHxuK@webmail.informatik.kit.edu","subject":"Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-05T22:15:13Z","receivedAt":"2016-10-05T22:15:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Gut feeling tells me that I should take this as a subtle\n> encouragement to look into adding 'versionsort.postreleasesuffix',\n> shouldn't I ;)\n\nIt is more like \"this made me realize that these are merely 'suffix'\nafter the real release name, no pre- or post- about them\", also\nknown as \"I think PREreleasesuffix was a mistake and we weren't\nthinking clearly enough when we added it.\"\n\nTo me, this looks like a list of possible suffixes that can include\nan empty suffix to denote \"the real thing\", e.g.\n\n versionsort.suffix = \"-alpha\" \"-beta\" \"\" \"-gamma\" \"-delta\"\n\nand that position in the list determines the order of things inside\nthe same family of versions that share the same \"non-suffix\" part.\n"},{"id":"303454","messageId":"CA+P7+xr4tYG6xoq4jTSxWyoQBwJSfZp7z=2BstTgaBSsgrs5+A@mail.gmail.com","threadId":"44016","inReplyTo":"xmqqwphmmoi6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-06T00:40:39Z","receivedAt":"2016-10-06T00:43:26Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Oct 5, 2016 at 3:15 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>\n>> Gut feeling tells me that I should take this as a subtle\n>> encouragement to look into adding 'versionsort.postreleasesuffix',\n>> shouldn't I ;)\n>\n> It is more like \"this made me realize that these are merely 'suffix'\n> after the real release name, no pre- or post- about them\", also\n> known as \"I think PREreleasesuffix was a mistake and we weren't\n> thinking clearly enough when we added it.\"\n>\n> To me, this looks like a list of possible suffixes that can include\n> an empty suffix to denote \"the real thing\", e.g.\n>\n> versionsort.suffix = \"-alpha\" \"-beta\" \"\" \"-gamma\" \"-delta\"\n>\n> and that position in the list determines the order of things inside\n> the same family of versions that share the same \"non-suffix\" part.\n\nThis is what makes sense to me. Perhaps migrade to \"releasesuffix\" and\ndeprecate \"prereleasesuffix\"? I don't think that's too painful\nespecially if git accepts the old value and warns about it's\ndeprecation? That or we can just stick with calling it pre-release and\nthey sort based on order with '' being the empty value?\n\nThanks,\nJake\n"},{"id":"303458","messageId":"CACsJy8BpYQJjiJ80-9kM7LD35brahctq=yYYg+9j_BxF3YQNEg@mail.gmail.com","threadId":"44016","inReplyTo":"CA+P7+xr4tYG6xoq4jTSxWyoQBwJSfZp7z=2BstTgaBSsgrs5+A@mail.gmail.com","subject":"Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-06T05:48:37Z","receivedAt":"2016-10-06T05:49:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 6, 2016 at 7:40 AM, Jacob Keller wrote:\n> On Wed, Oct 5, 2016 at 3:15 PM, Junio C Hamano wrote:\n>> SZEDER Gábor writes:\n>>\n>>> Gut feeling tells me that I should take this as a subtle\n>>> encouragement to look into adding 'versionsort.postreleasesuffix',\n>>> shouldn't I ;)\n>>\n>> It is more like \"this made me realize that these are merely 'suffix'\n>> after the real release name, no pre- or post- about them\", also\n>> known as \"I think PREreleasesuffix was a mistake and we weren't\n>> thinking clearly enough when we added it.\"\n>>\n>> To me, this looks like a list of possible suffixes that can include\n>> an empty suffix to denote \"the real thing\", e.g.\n>>\n>> versionsort.suffix = \"-alpha\" \"-beta\" \"\" \"-gamma\" \"-delta\"\n>>\n>> and that position in the list determines the order of things inside\n>> the same family of versions that share the same \"non-suffix\" part.\n>\n> This is what makes sense to me. Perhaps migrade to \"releasesuffix\" and\n> deprecate \"prereleasesuffix\"?\n\nEven though this is mostly used with tag listing, it can be used with\nbranch listing and general refs as well, where \"release\" does not\nreally make sense. So I prefer the name versionsort.suffix to\nversionsort.releasesuffix.\n\n> I don't think that's too painful\n> especially if git accepts the old value and warns about it's\n> deprecation? That or we can just stick with calling it pre-release and\n> they sort based on order with '' being the empty value?\n\nYeah deprecation is good, we don't want to support to config keys with\ndifferent algorithms forever.\n-- \nDuy\n"},{"id":"307242","messageId":"20161208142401.1329-2-szeder.dev@gmail.com","threadId":"44016","inReplyTo":"20161208142401.1329-1-szeder.dev@gmail.com","subject":"[PATCHv2 1/7] t7004-tag: delete unnecessary tags with test_when_finished","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-08T14:23:55Z","receivedAt":"2016-12-08T14:24:35Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The '--force is moot with a non-existing tag name' test creates two\nnew tags, which are then deleted right after the test is finished,\noutside the test_expect_success block, allowing 'git tag -d's output to\npollute the test output.\n\nUse test_when_finished to delete those tags.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t7004-tag.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 8b0f71a2a..396cffeeb 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -122,11 +122,11 @@ test_expect_success '--force can create a tag with the name of one existing' '\n \ttag_exists mytag'\n \n test_expect_success '--force is moot with a non-existing tag name' '\n+\ttest_when_finished git tag -d newtag forcetag &&\n \tgit tag newtag >expect &&\n \tgit tag --force forcetag >actual &&\n \ttest_cmp expect actual\n '\n-git tag -d newtag forcetag\n \n # deleting tags:\n \n-- \n2.11.0.78.g5a2d011\n\n"},{"id":"307243","messageId":"20161208142401.1329-4-szeder.dev@gmail.com","threadId":"44016","inReplyTo":"20161208142401.1329-1-szeder.dev@gmail.com","subject":"[PATCHv2 3/7] t7004-tag: add version sort tests to show prerelease reordering issues","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-08T14:23:57Z","receivedAt":"2016-12-08T14:24:38Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Version sort with prerelease reordering sometimes puts tagnames in the\nwrong order, when the common part of two compared tagnames ends with\nthe leading character(s) of one or more configured prerelease\nsuffixes. Add tests that demonstrate these issues.\n\nThe unrelated '--format should list tags as per format given' test\nlater uses tags matching the same prefix as the version sort tests,\nthus was affected by the new tags added for the new tests in this\npatch. Change that test to perform its checks on a different set of\ntags.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t7004-tag.sh | 36 ++++++++++++++++++++++++++++++------\n 1 file changed, 30 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 920a1b4b2..6445aae29 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1538,6 +1538,32 @@ test_expect_success 'reverse version sort with prerelease reordering' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'version sort with prerelease reordering and common leading character' '\n+\ttest_config versionsort.prereleaseSuffix -before &&\n+\tgit tag foo1.7-before1 &&\n+\tgit tag foo1.7 &&\n+\tgit tag foo1.7-after1 &&\n+\tgit tag -l --sort=version:refname \"foo1.7*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.7-before1\n+\tfoo1.7\n+\tfoo1.7-after1\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'version sort with prerelease reordering, multiple suffixes and common leading character' '\n+\ttest_config versionsort.prereleaseSuffix -before &&\n+\tgit config --add versionsort.prereleaseSuffix -after &&\n+\tgit tag -l --sort=version:refname \"foo1.7*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.7-before1\n+\tfoo1.7-after1\n+\tfoo1.7\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n run_with_limited_stack () {\n \t(ulimit -s 128 && \"$@\")\n }\n@@ -1566,13 +1592,11 @@ EOF\"\n \n test_expect_success '--format should list tags as per format given' '\n \tcat >expect <<-\\EOF &&\n-\trefname : refs/tags/foo1.10\n-\trefname : refs/tags/foo1.3\n-\trefname : refs/tags/foo1.6\n-\trefname : refs/tags/foo1.6-rc1\n-\trefname : refs/tags/foo1.6-rc2\n+\trefname : refs/tags/v1.0\n+\trefname : refs/tags/v1.0.1\n+\trefname : refs/tags/v1.1.3\n \tEOF\n-\tgit tag -l --format=\"refname : %(refname)\" \"foo*\" >actual &&\n+\tgit tag -l --format=\"refname : %(refname)\" \"v1*\" >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.11.0.78.g5a2d011\n\n"},{"id":"307244","messageId":"20161208142401.1329-6-szeder.dev@gmail.com","threadId":"44016","inReplyTo":"20161208142401.1329-1-szeder.dev@gmail.com","subject":"[PATCHv2 5/7] versioncmp: cope with common part overlapping with prerelease suffix","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-08T14:23:59Z","receivedAt":"2016-12-08T14:24:46Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Version sort with prerelease reordering sometimes puts tagnames in the\nwrong order, when the common part of two compared tagnames overlaps\nwith the leading character(s) of one or more configured prerelease\nsuffixes. Note the position of \"v2.1.0-beta-1\":\n\n $ git -c versionsort.prereleaseSuffix=-beta \\\n tag -l --sort=version:refname v2.1.*\n v2.1.0-beta-2\n v2.1.0-beta-3\n v2.1.0\n v2.1.0-RC1\n v2.1.0-RC2\n v2.1.0-beta-1\n v2.1.1\n v2.1.2\n\nThe reason is that when comparing a pair of tagnames, first\nversioncmp() looks for the first different character in a pair of\ntagnames, and then the swap_prereleases() helper function looks for a\nconfigured prerelease suffix _starting at_ that character. Thus, when\nin the above example the sorting algorithm happens to compare the\ntagnames \"v2.1.0-beta-1\" and \"v2.1.0-RC2\", swap_prereleases() tries to\nmatch the suffix \"-beta\" against \"beta-1\" to no avail, and the two\ntagnames erroneously end up being ordered lexicographically.\n\nTo fix this issue change swap_prereleases() to look for configured\nprerelease suffixes _containing_ the position of that first different\ncharacter.\n\nCare must be taken, when a configured suffix is longer than the\ntagnames' common part up to the first different character, to avoid\nreading memory before the beginning of the tagnames. Add a test that\nuses an exceptionally long prerelease suffix to check for this, in the\nhope that in case of a regression the illegal memory access causes a\nsegfault in 'git tag' on one of the commonly used platforms (the test\nhappens to pass successfully on my Linux system with the safety check\nremoved), or at least makes valgrind complain.\n\nUnder some circumstances it's possible that more than one prerelease\nsuffixes can be found in the same tagname around that first different\ncharacter. With this simple bugfix patch such a tagname is sorted\naccording to the contained suffix that comes first in the\nconfiguration for now. This is less than ideal in some cases, and the\nfollowing patch will take care of those.\n\nReported-by: Leho Kraav \nSigned-off-by: SZEDER Gábor \n---\n Documentation/config.txt | 8 ++++++--\n t/t7004-tag.sh | 9 +++++++--\n versioncmp.c | 28 +++++++++++++++++++++-------\n 3 files changed, 34 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb6790d..c1a9616e9 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3008,8 +3008,12 @@ versionsort.prereleaseSuffix::\n This variable can be specified multiple times, once per suffix. The\n order of suffixes in the config file determines the sorting order\n (e.g. if \"-pre\" appears before \"-rc\" in the config file then 1.0-preXX\n-is sorted before 1.0-rcXX). The sorting order between different\n-suffixes is undefined if they are in multiple config files.\n+is sorted before 1.0-rcXX).\n+If more than one suffixes match the same tagname, then that tagname will\n+be sorted according to the matching suffix which comes first in the\n+configuration.\n+The sorting order between different suffixes is undefined if they are\n+in multiple config files.\n \n web.browser::\n \tSpecify a web browser that may be used by some commands.\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 6445aae29..c7aaace8c 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1538,7 +1538,7 @@ test_expect_success 'reverse version sort with prerelease reordering' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'version sort with prerelease reordering and common leading character' '\n+test_expect_success 'version sort with prerelease reordering and common leading character' '\n \ttest_config versionsort.prereleaseSuffix -before &&\n \tgit tag foo1.7-before1 &&\n \tgit tag foo1.7 &&\n@@ -1552,7 +1552,7 @@ test_expect_failure 'version sort with prerelease reordering and common leading\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'version sort with prerelease reordering, multiple suffixes and common leading character' '\n+test_expect_success 'version sort with prerelease reordering, multiple suffixes and common leading character' '\n \ttest_config versionsort.prereleaseSuffix -before &&\n \tgit config --add versionsort.prereleaseSuffix -after &&\n \tgit tag -l --sort=version:refname \"foo1.7*\" >actual &&\n@@ -1564,6 +1564,11 @@ test_expect_failure 'version sort with prerelease reordering, multiple suffixes\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'version sort with very long prerelease suffix' '\n+\ttest_config versionsort.prereleaseSuffix -very-looooooooooooooooooooooooong-prerelease-suffix &&\n+\tgit tag -l --sort=version:refname\n+'\n+\n run_with_limited_stack () {\n \t(ulimit -s 128 && \"$@\")\n }\ndiff --git a/versioncmp.c b/versioncmp.c\nindex a55c23ad5..f86ac562e 100644\n--- a/versioncmp.c\n+++ b/versioncmp.c\n@@ -26,12 +26,15 @@ static int initialized;\n \n /*\n * off is the offset of the first different character in the two strings\n- * s1 and s2. If either s1 or s2 contains a prerelease suffix starting\n- * at that offset, it will be forced to be on top.\n+ * s1 and s2. If either s1 or s2 contains a prerelease suffix containing\n+ * that offset, then that string will be forced to be on top.\n *\n- * If both s1 and s2 contain a (different) suffix at that position,\n+ * If both s1 and s2 contain a (different) suffix around that position,\n * their order is determined by the order of those two suffixes in the\n * configuration.\n+ * If any of the strings contains more than one different suffixes around\n+ * that position, then that string is sorted according to the contained\n+ * suffix which comes first in the configuration.\n *\n * Return non-zero if *diff contains the return value for versioncmp()\n */\n@@ -44,10 +47,21 @@ static int swap_prereleases(const char *s1,\n \n \tfor (i = 0; i < prereleases->nr; i++) {\n \t\tconst char *suffix = prereleases->items[i].string;\n-\t\tif (i1 == -1 && starts_with(s1 + off, suffix))\n-\t\t\ti1 = i;\n-\t\tif (i2 == -1 && starts_with(s2 + off, suffix))\n-\t\t\ti2 = i;\n+\t\tint j, start, suffix_len = strlen(suffix);\n+\t\tif (suffix_len < off)\n+\t\t\tstart = off - suffix_len + 1;\n+\t\telse\n+\t\t\tstart = 0;\n+\t\tfor (j = start; j <= off; j++)\n+\t\t\tif (i1 == -1 && starts_with(s1 + j, suffix)) {\n+\t\t\t\ti1 = i;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\tfor (j = start; j <= off; j++)\n+\t\t\tif (i2 == -1 && starts_with(s2 + j, suffix)) {\n+\t\t\t\ti2 = i;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t}\n \tif (i1 == -1 && i2 == -1)\n \t\treturn 0;\n-- \n2.11.0.78.g5a2d011\n\n"},{"id":"307245","messageId":"20161208142401.1329-8-szeder.dev@gmail.com","threadId":"44016","inReplyTo":"20161208142401.1329-1-szeder.dev@gmail.com","subject":"[PATCHv2 7/7] versioncmp: generalize version sort suffix reordering","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-08T14:24:01Z","receivedAt":"2016-12-08T14:24:49Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The 'versionsort.prereleaseSuffix' configuration variable, as its name\nsuggests, is supposed to only deal with tagnames with prerelease\nsuffixes, and allows sorting those prerelease tags in a user-defined\norder before the suffixless main release tag, instead of sorting them\nsimply lexicographically.\n\nHowever, the previous changes in this series resulted in an\ninteresting and useful property of version sort:\n\n - The empty string as a configured suffix matches all tagnames,\n including tagnames without any suffix, but\n\n - tagnames containing a \"real\" configured suffix are still ordered\n according to that real suffix, because any longer suffix takes\n precedence over the empty string.\n\nExploiting this property we can easily generalize suffix reordering\nand specify the order of tags with given suffixes not only before but\neven after a main release tag by using the empty suffix to denote the\nposition of the main release tag, without any algorithm changes:\n\n $ git -c versionsort.prereleaseSuffix=-alpha \\\n -c versionsort.prereleaseSuffix=-beta \\\n -c versionsort.prereleaseSuffix=\"\" \\\n -c versionsort.prereleaseSuffix=-gamma \\\n -c versionsort.prereleaseSuffix=-delta \\\n tag -l --sort=version:refname 'v3.0*'\n v3.0-alpha1\n v3.0-beta1\n v3.0\n v3.0-gamma1\n v3.0-delta1\n\nSince 'versionsort.prereleaseSuffix' is not a fitting name for a\nconfiguration variable to control this more general suffix reordering,\nintroduce the new variable 'versionsort.suffix'. Still keep the old\nconfiguration variable name as a deprecated alias, though, to avoid\nsuddenly breaking setups already using it. Ignore the old variable if\nboth old and new configuration variables are set, but emit a warning\nso users will be aware of it and can fix their configuration. Extend\nthe documentation to describe and add a test to check this more\ngeneral behavior.\n\nNote: since the empty suffix matches all tagnames, tagnames with\nsuffixes not included in the configuration are listed together with\nthe suffixless main release tag, ordered lexicographically right after\nthat, i.e. before tags with suffixes listed in the configuration\nfollowing the empty suffix.\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/config.txt | 36 ++++++++++++++++++++++++++----------\n Documentation/git-tag.txt | 4 ++--\n t/t7004-tag.sh | 35 +++++++++++++++++++++++++++++++++++\n versioncmp.c | 9 ++++++++-\n 4 files changed, 71 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 58009c70c..ae85d4b9a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2999,16 +2999,32 @@ user.signingKey::\n \tThis option is passed unchanged to gpg's --local-user parameter,\n \tso you may specify a key using any method that gpg supports.\n \n-versionsort.prereleaseSuffix::\n-\tWhen version sort is used in linkgit:git-tag[1], prerelease\n-\ttags (e.g. \"1.0-rc1\") may appear after the main release\n-\t\"1.0\". By specifying the suffix \"-rc\" in this variable,\n-\t\"1.0-rc1\" will appear before \"1.0\".\n-+\n-This variable can be specified multiple times, once per suffix. The\n-order of suffixes in the config file determines the sorting order\n-(e.g. if \"-pre\" appears before \"-rc\" in the config file then 1.0-preXX\n-is sorted before 1.0-rcXX).\n+versionsort.prereleaseSuffix (deprecated)::\n+\tDeprecated alias for `versionsort.suffix`. Ignored if\n+\t`versionsort.suffix` is set.\n+\n+versionsort.suffix::\n+\tEven when version sort is used in linkgit:git-tag[1], tagnames\n+\twith the same base version but different suffixes are still sorted\n+\tlexicographically, resulting e.g. in prerelease tags appearing\n+\tafter the main release (e.g. \"1.0-rc1\" after \"1.0\"). This\n+\tvariable can be specified to determine the sorting order of tags\n+\twith different suffixes.\n++\n+By specifying a single suffix in this variable, any tagname containing\n+that suffix will appear before the corresponding main release. E.g. if\n+the variable is set to \"-rc\", then all \"1.0-rcX\" tags will appear before\n+\"1.0\". If specified multiple times, once per suffix, then the order of\n+suffixes in the configuration will determine the sorting order of tagnames\n+with those suffixes. E.g. if \"-pre\" appears before \"-rc\" in the\n+configuration, then all \"1.0-preX\" tags will be listed before any\n+\"1.0-rcX\" tags. The placement of the main release tag relative to tags\n+with various suffixes can be determined by specifying the empty suffix\n+among those other suffixes. E.g. if the suffixes \"-rc\", \"\", \"-ck\" and\n+\"-bfs\" appear in the configuration in this order, then all \"v4.8-rcX\" tags\n+are listed first, followed by \"v4.8\", then \"v4.8-ckX\" and finally\n+\"v4.8-bfsX\".\n++\n If more than one suffixes match the same tagname, then that tagname will\n be sorted according to the suffix which starts at the earliest position in\n the tagname. If more than one different matching suffixes start at\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 7ecca8e24..19990850d 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -101,8 +101,8 @@ OPTIONS\n \tmultiple times, in which case the last key becomes the primary\n \tkey. Also supports \"version:refname\" or \"v:refname\" (tag\n \tnames are treated as versions). The \"version:refname\" sort\n-\torder can also be affected by the\n-\t\"versionsort.prereleaseSuffix\" configuration variable.\n+\torder can also be affected by the \"versionsort.suffix\"\n+\tconfiguration variable.\n \tThe keys supported are the same as those in `git for-each-ref`.\n \tSort order defaults to the value configured for the `tag.sort`\n \tvariable if it exists, or lexicographic order otherwise. See\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e2efe312d..bdd28dad1 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1595,6 +1595,41 @@ test_expect_success 'version sort with prerelease reordering, multiple suffixes\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'version sort with general suffix reordering' '\n+\ttest_config versionsort.suffix -alpha &&\n+\tgit config --add versionsort.suffix -beta &&\n+\tgit config --add versionsort.suffix \"\" &&\n+\tgit config --add versionsort.suffix -gamma &&\n+\tgit config --add versionsort.suffix -delta &&\n+\tgit tag foo1.10-alpha &&\n+\tgit tag foo1.10-beta &&\n+\tgit tag foo1.10-gamma &&\n+\tgit tag foo1.10-delta &&\n+\tgit tag foo1.10-unlisted-suffix &&\n+\tgit tag -l --sort=version:refname \"foo1.10*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.10-alpha\n+\tfoo1.10-beta\n+\tfoo1.10\n+\tfoo1.10-unlisted-suffix\n+\tfoo1.10-gamma\n+\tfoo1.10-delta\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'versionsort.suffix overrides versionsort.prereleaseSuffix' '\n+\ttest_config versionsort.suffix -before &&\n+\ttest_config versionsort.prereleaseSuffix -after &&\n+\tgit tag -l --sort=version:refname \"foo1.7*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.7-before1\n+\tfoo1.7\n+\tfoo1.7-after1\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'version sort with very long prerelease suffix' '\n \ttest_config versionsort.prereleaseSuffix -very-looooooooooooooooooooooooong-prerelease-suffix &&\n \tgit tag -l --sort=version:refname\ndiff --git a/versioncmp.c b/versioncmp.c\nindex ae0a9199b..62c8d69dc 100644\n--- a/versioncmp.c\n+++ b/versioncmp.c\n@@ -145,8 +145,15 @@ int versioncmp(const char *s1, const char *s2)\n \t}\n \n \tif (!initialized) {\n+\t\tconst struct string_list *deprecated_prereleases;\n \t\tinitialized = 1;\n-\t\tprereleases = git_config_get_value_multi(\"versionsort.prereleasesuffix\");\n+\t\tprereleases = git_config_get_value_multi(\"versionsort.suffix\");\n+\t\tdeprecated_prereleases = git_config_get_value_multi(\"versionsort.prereleasesuffix\");\n+\t\tif (prereleases) {\n+\t\t\tif (deprecated_prereleases)\n+\t\t\t\twarning(\"ignoring versionsort.prereleasesuffix because versionsort.suffix is set\");\n+\t\t} else\n+\t\t\tprereleases = deprecated_prereleases;\n \t}\n \tif (prereleases && swap_prereleases(s1, s2, (const char *) p1 - s1 - 1,\n \t\t\t\t\t &diff))\n-- \n2.11.0.78.g5a2d011\n\n"},{"id":"307246","messageId":"20161208142401.1329-7-szeder.dev@gmail.com","threadId":"44016","inReplyTo":"20161208142401.1329-1-szeder.dev@gmail.com","subject":"[PATCHv2 6/7] versioncmp: use earliest-longest contained suffix to determine sorting order","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-08T14:24:00Z","receivedAt":"2016-12-08T14:24:50Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When comparing tagnames, it is possible that a tagname contains more\nthan one of the configured prerelease suffixes around the first\ndifferent character. After fixing a bug in the previous commit such a\ntagname is sorted according to the contained suffix which comes first\nin the configuration. This is, however, not quite the right thing to\ndo in the following corner cases:\n\n 1. $ git -c versionsort.suffix=-bar\n -c versionsort.suffix=-foo-baz\n -c versionsort.suffix=-foo-bar\n tag -l --sort=version:refname 'v1*'\n v1.0-foo-bar\n v1.0-foo-baz\n\n The suffix of the tagname 'v1.0-foo-bar' is clearly '-foo-bar',\n so it should be listed last. However, as it also contains '-bar'\n around the first different character, it is listed first instead,\n because that '-bar' suffix comes first the configuration.\n\n 2. One of the configured suffixes starts with the other:\n\n $ git -c versionsort.prereleasesuffix=-pre \\\n -c versionsort.prereleasesuffix=-prerelease \\\n tag -l --sort=version:refname 'v2*'\n v2.0-prerelease1\n v2.0-pre1\n v2.0-pre2\n\n Here the tagname 'v2.0-prerelease1' should be the last. When\n comparing 'v2.0-pre1' and 'v2.0-prerelease1' the first different\n characters are '1' and 'r', respectively. Since this first\n different character must be part of the configured suffix, the\n '-pre' suffix is not recognized in the first tagname. OTOH, the\n '-prerelease' suffix is properly recognized in\n 'v2.0-prerelease1', thus it is listed first.\n\nImprove version sort in these corner cases, and\n\n - look for a configured prerelease suffix containing the first\n different character or ending right before it, so the '-pre'\n suffixes are recognized in case (2). This also means that\n when comparing tagnames 'v2.0-pre1' and 'v2.0-pre2',\n swap_prereleases() would find the '-pre' suffix in both, but then\n it will return \"undecided\" and the caller will do the right thing\n by sorting based in '1' and '2'.\n\n - If the tagname contains more than one suffix, then give precedence\n to the contained suffix that starts at the earliest offset in the\n tagname to address (1).\n\n - If there are more than one suffixes starting at that earliest\n position, then give precedence to the longest of those suffixes,\n thus ensuring that in (2) the tagname 'v2.0-prerelease1' won't be\n sorted based on the '-pre' suffix.\n\nAdd tests for these corner cases and adjust the documentation\naccordingly.\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/config.txt | 6 ++++--\n t/t7004-tag.sh | 31 +++++++++++++++++++++++++++++++\n versioncmp.c | 33 +++++++++++++++++++++++++--------\n 3 files changed, 60 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c1a9616e9..58009c70c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3010,8 +3010,10 @@ order of suffixes in the config file determines the sorting order\n (e.g. if \"-pre\" appears before \"-rc\" in the config file then 1.0-preXX\n is sorted before 1.0-rcXX).\n If more than one suffixes match the same tagname, then that tagname will\n-be sorted according to the matching suffix which comes first in the\n-configuration.\n+be sorted according to the suffix which starts at the earliest position in\n+the tagname. If more than one different matching suffixes start at\n+that earliest position, then that tagname will be sorted according to the\n+longest of those suffixes.\n The sorting order between different suffixes is undefined if they are\n in multiple config files.\n \ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex c7aaace8c..e2efe312d 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1564,6 +1564,37 @@ test_expect_success 'version sort with prerelease reordering, multiple suffixes\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'version sort with prerelease reordering, multiple suffixes match the same tag' '\n+\ttest_config versionsort.prereleaseSuffix -bar &&\n+\tgit config --add versionsort.prereleaseSuffix -foo-baz &&\n+\tgit config --add versionsort.prereleaseSuffix -foo-bar &&\n+\tgit tag foo1.8-foo-bar &&\n+\tgit tag foo1.8-foo-baz &&\n+\tgit tag foo1.8 &&\n+\tgit tag -l --sort=version:refname \"foo1.8*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.8-foo-baz\n+\tfoo1.8-foo-bar\n+\tfoo1.8\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'version sort with prerelease reordering, multiple suffixes match starting at the same position' '\n+\ttest_config versionsort.prereleaseSuffix -pre &&\n+\tgit config --add versionsort.prereleaseSuffix -prerelease &&\n+\tgit tag foo1.9-pre1 &&\n+\tgit tag foo1.9-pre2 &&\n+\tgit tag foo1.9-prerelease1 &&\n+\tgit tag -l --sort=version:refname \"foo1.9*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.9-pre1\n+\tfoo1.9-pre2\n+\tfoo1.9-prerelease1\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'version sort with very long prerelease suffix' '\n \ttest_config versionsort.prereleaseSuffix -very-looooooooooooooooooooooooong-prerelease-suffix &&\n \tgit tag -l --sort=version:refname\ndiff --git a/versioncmp.c b/versioncmp.c\nindex f86ac562e..ae0a9199b 100644\n--- a/versioncmp.c\n+++ b/versioncmp.c\n@@ -27,14 +27,18 @@ static int initialized;\n /*\n * off is the offset of the first different character in the two strings\n * s1 and s2. If either s1 or s2 contains a prerelease suffix containing\n- * that offset, then that string will be forced to be on top.\n+ * that offset or a suffix ends right before that offset, then that\n+ * string will be forced to be on top.\n *\n * If both s1 and s2 contain a (different) suffix around that position,\n * their order is determined by the order of those two suffixes in the\n * configuration.\n * If any of the strings contains more than one different suffixes around\n * that position, then that string is sorted according to the contained\n- * suffix which comes first in the configuration.\n+ * suffix which starts at the earliest offset in that string.\n+ * If more than one different contained suffixes start at that earliest\n+ * offset, then that string is sorted according to the longest of those\n+ * suffixes.\n *\n * Return non-zero if *diff contains the return value for versioncmp()\n */\n@@ -44,27 +48,40 @@ static int swap_prereleases(const char *s1,\n \t\t\t int *diff)\n {\n \tint i, i1 = -1, i2 = -1;\n+\tint start_at1 = off, start_at2 = off, match_len1 = -1, match_len2 = -1;\n \n \tfor (i = 0; i < prereleases->nr; i++) {\n \t\tconst char *suffix = prereleases->items[i].string;\n-\t\tint j, start, suffix_len = strlen(suffix);\n+\t\tint j, start, end, suffix_len = strlen(suffix);\n \t\tif (suffix_len < off)\n-\t\t\tstart = off - suffix_len + 1;\n+\t\t\tstart = off - suffix_len;\n \t\telse\n \t\t\tstart = 0;\n-\t\tfor (j = start; j <= off; j++)\n-\t\t\tif (i1 == -1 && starts_with(s1 + j, suffix)) {\n+\t\tend = match_len1 < suffix_len ? start_at1 : start_at1-1;\n+\t\tfor (j = start; j <= end; j++)\n+\t\t\tif (starts_with(s1 + j, suffix)) {\n \t\t\t\ti1 = i;\n+\t\t\t\tstart_at1 = j;\n+\t\t\t\tmatch_len1 = suffix_len;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\tfor (j = start; j <= off; j++)\n-\t\t\tif (i2 == -1 && starts_with(s2 + j, suffix)) {\n+\t\tend = match_len2 < suffix_len ? start_at2 : start_at2-1;\n+\t\tfor (j = start; j <= end; j++)\n+\t\t\tif (starts_with(s2 + j, suffix)) {\n \t\t\t\ti2 = i;\n+\t\t\t\tstart_at2 = j;\n+\t\t\t\tmatch_len2 = suffix_len;\n \t\t\t\tbreak;\n \t\t\t}\n \t}\n \tif (i1 == -1 && i2 == -1)\n \t\treturn 0;\n+\tif (i1 == i2)\n+\t\t/* Found the same suffix in both, e.g. \"-rc\" in \"v1.0-rcX\"\n+\t\t * and \"v1.0-rcY\": the caller should decide based on \"X\"\n+\t\t * and \"Y\". */\n+\t\treturn 0;\n+\n \tif (i1 >= 0 && i2 >= 0)\n \t\t*diff = i1 - i2;\n \telse if (i1 >= 0)\n-- \n2.11.0.78.g5a2d011\n\n"},{"id":"307247","messageId":"20161208142401.1329-3-szeder.dev@gmail.com","threadId":"44016","inReplyTo":"20161208142401.1329-1-szeder.dev@gmail.com","subject":"[PATCHv2 2/7] t7004-tag: use test_config helper","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-08T14:23:56Z","receivedAt":"2016-12-08T14:24:53Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"... instead of setting and then manually unsetting configuration\nvariables, on one occasion even outside the test_expect_success block.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t7004-tag.sh | 23 ++++++++++-------------\n 1 file changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 396cffeeb..920a1b4b2 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -297,11 +297,9 @@ EOF\n '\n \n test_expect_success 'listing tags in column with column.*' '\n-\tgit config column.tag row &&\n-\tgit config column.ui dense &&\n+\ttest_config column.tag row &&\n+\ttest_config column.ui dense &&\n \tCOLUMNS=40 git tag -l >actual &&\n-\tgit config --unset column.ui &&\n-\tgit config --unset column.tag &&\n \tcat >expected <<\\EOF &&\n a1 aa1 cba t210 t211\n v0.2.1 v1.0 v1.0.1 v1.1.3\n@@ -314,9 +312,8 @@ test_expect_success 'listing tag with -n --column should fail' '\n '\n \n test_expect_success 'listing tags -n in column with column.ui ignored' '\n-\tgit config column.ui \"row dense\" &&\n+\ttest_config column.ui \"row dense\" &&\n \tCOLUMNS=40 git tag -l -n >actual &&\n-\tgit config --unset column.ui &&\n \tcat >expected <<\\EOF &&\n a1 Foo\n aa1 Foo\n@@ -1200,11 +1197,10 @@ test_expect_success GPG,RFC1991 \\\n '\n \n # try to sign with bad user.signingkey\n-git config user.signingkey BobTheMouse\n test_expect_success GPG \\\n \t'git tag -s fails if gpg is misconfigured (bad key)' \\\n-\t'test_must_fail git tag -s -m tail tag-gpg-failure'\n-git config --unset user.signingkey\n+\t'test_config user.signingkey BobTheMouse &&\n+\ttest_must_fail git tag -s -m tail tag-gpg-failure'\n \n # try to produce invalid signature\n test_expect_success GPG \\\n@@ -1484,7 +1480,7 @@ test_expect_success 'reverse lexical sort' '\n '\n \n test_expect_success 'configured lexical sort' '\n-\tgit config tag.sort \"v:refname\" &&\n+\ttest_config tag.sort \"v:refname\" &&\n \tgit tag -l \"foo*\" >actual &&\n \tcat >expect <<-\\EOF &&\n \tfoo1.3\n@@ -1495,6 +1491,7 @@ test_expect_success 'configured lexical sort' '\n '\n \n test_expect_success 'option override configured sort' '\n+\ttest_config tag.sort \"v:refname\" &&\n \tgit tag -l --sort=-refname \"foo*\" >actual &&\n \tcat >expect <<-\\EOF &&\n \tfoo1.6\n@@ -1509,13 +1506,12 @@ test_expect_success 'invalid sort parameter on command line' '\n '\n \n test_expect_success 'invalid sort parameter in configuratoin' '\n-\tgit config tag.sort \"v:notvalid\" &&\n+\ttest_config tag.sort \"v:notvalid\" &&\n \ttest_must_fail git tag -l \"foo*\"\n '\n \n test_expect_success 'version sort with prerelease reordering' '\n-\tgit config --unset tag.sort &&\n-\tgit config versionsort.prereleaseSuffix -rc &&\n+\ttest_config versionsort.prereleaseSuffix -rc &&\n \tgit tag foo1.6-rc1 &&\n \tgit tag foo1.6-rc2 &&\n \tgit tag -l --sort=version:refname \"foo*\" >actual &&\n@@ -1530,6 +1526,7 @@ test_expect_success 'version sort with prerelease reordering' '\n '\n \n test_expect_success 'reverse version sort with prerelease reordering' '\n+\ttest_config versionsort.prereleaseSuffix -rc &&\n \tgit tag -l --sort=-version:refname \"foo*\" >actual &&\n \tcat >expect <<-\\EOF &&\n \tfoo1.10\n-- \n2.11.0.78.g5a2d011\n\n"},{"id":"307248","messageId":"20161208142401.1329-1-szeder.dev@gmail.com","threadId":"44016","inReplyTo":"20161005033353.Horde.33pf2naqnF4HgwPWSy9DaHV@webmail.informatik.kit.edu","subject":"[PATCHv2 0/7] Fix and generalize version sort reordering","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-08T14:23:54Z","receivedAt":"2016-12-08T14:24:54Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> After having slept on this a couple of times\n\nAfter having slept on it a few (erm...) more times...\n\n> I think the longest\n> matching prerelease suffix should determine the sorting order.\n> \n> A release tag usually consists of an optional prefix, e.g. 'v' or\n> 'snapshot-', followed by the actual version number, followed by an\n> optional suffix. In the contrived example quoted above this suffix\n> is '-foo-bar', thus it feels wrong to match '-bar' against it, when\n> the user explicitly configured '-foo-bar' as prerelease suffix as\n> well.\n\nAt the risk of overthinking it: I think it would be more correct to\nsay that the earliest starting matching prerelease suffix should\ndetermine the sorting order in the first place.\n\n> Then here is a more realistic case for sorting based on the longest\n> matching suffix, where\n> \n> - a longer suffix starts with the shorter one,\n> - and the longer suffix comes after the shorter one in the\n> configuration.\n> \n> With my patches it looks like this:\n> \n> $ git -c versionsort.prereleasesuffix=-pre \\\n> -c versionsort.prereleasesuffix=-prerelease \\\n> tag -l --sort=version:refname\n> v1.0.0-prerelease1\n> v1.0.0-pre1\n> v1.0.0-pre2\n> v1.0.0\n\nAnd when there happen to be more than one matching suffixes starting\nat the same earliest position, then we should pick the longest of\nthem. The new patch 6/7 implements this behavior.\n\nChanges since v1:\n - Documentation, in-code comment and commit message updates.\n - Use different tagnames and suffixes in the tests, keeping the new\n tests simpler and changes to existing tests smaller.\n - Added a test of questionable value to try to check that we don't\n read memory before the tagname strings in case of an unusually long\n suffix.\n - Removed unnecessary {}.\n - Added two patches on top to address the corner cases and to\n introduce 'versionsort.suffix' for the resulting more general\n suffix reordering behavior.\n\nThe interdiff at the bottom is between v1 and only the first five\npatches of v2, i.e. not including the two new patches. I think it's\neasier to judge the changes this way.\n\nSZEDER Gábor (7):\n t7004-tag: delete unnecessary tags with test_when_finished\n t7004-tag: use test_config helper\n t7004-tag: add version sort tests to show prerelease reordering issues\n versioncmp: pass full tagnames to swap_prereleases()\n versioncmp: cope with common part overlapping with prerelease suffix\n versioncmp: use earliest-longest contained suffix to determine sorting\n order\n versioncmp: generalize version sort suffix reordering\n\n Documentation/config.txt | 44 ++++++++++++----\n Documentation/git-tag.txt | 4 +-\n t/t7004-tag.sh | 132 +++++++++++++++++++++++++++++++++++++++-------\n versioncmp.c | 68 +++++++++++++++++-------\n 4 files changed, 196 insertions(+), 52 deletions(-)\n\n-- \n2.11.0.78.g5a2d011\n\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb6790d..c1a9616e9 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3008,8 +3008,12 @@ versionsort.prereleaseSuffix::\n This variable can be specified multiple times, once per suffix. The\n order of suffixes in the config file determines the sorting order\n (e.g. if \"-pre\" appears before \"-rc\" in the config file then 1.0-preXX\n-is sorted before 1.0-rcXX). The sorting order between different\n-suffixes is undefined if they are in multiple config files.\n+is sorted before 1.0-rcXX).\n+If more than one suffixes match the same tagname, then that tagname will\n+be sorted according to the matching suffix which comes first in the\n+configuration.\n+The sorting order between different suffixes is undefined if they are\n+in multiple config files.\n \n web.browser::\n \tSpecify a web browser that may be used by some commands.\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex f0cfe1fa3..c7aaace8c 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1511,14 +1511,14 @@ test_expect_success 'invalid sort parameter in configuratoin' '\n '\n \n test_expect_success 'version sort with prerelease reordering' '\n-\ttest_config versionsort.prereleaseSuffix -beta &&\n-\tgit tag foo1.6-beta1 &&\n-\tgit tag foo1.6-beta2 &&\n+\ttest_config versionsort.prereleaseSuffix -rc &&\n+\tgit tag foo1.6-rc1 &&\n+\tgit tag foo1.6-rc2 &&\n \tgit tag -l --sort=version:refname \"foo*\" >actual &&\n \tcat >expect <<-\\EOF &&\n \tfoo1.3\n-\tfoo1.6-beta1\n-\tfoo1.6-beta2\n+\tfoo1.6-rc1\n+\tfoo1.6-rc2\n \tfoo1.6\n \tfoo1.10\n \tEOF\n@@ -1526,54 +1526,49 @@ test_expect_success 'version sort with prerelease reordering' '\n '\n \n test_expect_success 'reverse version sort with prerelease reordering' '\n-\ttest_config versionsort.prereleaseSuffix -beta &&\n+\ttest_config versionsort.prereleaseSuffix -rc &&\n \tgit tag -l --sort=-version:refname \"foo*\" >actual &&\n \tcat >expect <<-\\EOF &&\n \tfoo1.10\n \tfoo1.6\n-\tfoo1.6-beta2\n-\tfoo1.6-beta1\n+\tfoo1.6-rc2\n+\tfoo1.6-rc1\n \tfoo1.3\n \tEOF\n \ttest_cmp expect actual\n '\n \n test_expect_success 'version sort with prerelease reordering and common leading character' '\n-\ttest_config versionsort.prereleaseSuffix -beta &&\n-\tgit tag foo1.6-after1 &&\n-\tgit tag -l --sort=version:refname \"foo*\" >actual &&\n+\ttest_config versionsort.prereleaseSuffix -before &&\n+\tgit tag foo1.7-before1 &&\n+\tgit tag foo1.7 &&\n+\tgit tag foo1.7-after1 &&\n+\tgit tag -l --sort=version:refname \"foo1.7*\" >actual &&\n \tcat >expect <<-\\EOF &&\n-\tfoo1.3\n-\tfoo1.6-beta1\n-\tfoo1.6-beta2\n-\tfoo1.6\n-\tfoo1.6-after1\n-\tfoo1.10\n+\tfoo1.7-before1\n+\tfoo1.7\n+\tfoo1.7-after1\n \tEOF\n \ttest_cmp expect actual\n '\n \n-# Capitalization of suffixes is important here, because \"-RC\" would normally\n-# be sorted before \"-beta\" and the config settings should override that.\n test_expect_success 'version sort with prerelease reordering, multiple suffixes and common leading character' '\n-\ttest_config versionsort.prereleaseSuffix -beta &&\n-\tgit config --add versionsort.prereleaseSuffix -RC &&\n-\tgit tag foo1.6-RC1 &&\n-\tgit tag foo1.6-RC2 &&\n-\tgit tag -l --sort=version:refname \"foo*\" >actual &&\n+\ttest_config versionsort.prereleaseSuffix -before &&\n+\tgit config --add versionsort.prereleaseSuffix -after &&\n+\tgit tag -l --sort=version:refname \"foo1.7*\" >actual &&\n \tcat >expect <<-\\EOF &&\n-\tfoo1.3\n-\tfoo1.6-beta1\n-\tfoo1.6-beta2\n-\tfoo1.6-RC1\n-\tfoo1.6-RC2\n-\tfoo1.6\n-\tfoo1.6-after1\n-\tfoo1.10\n+\tfoo1.7-before1\n+\tfoo1.7-after1\n+\tfoo1.7\n \tEOF\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'version sort with very long prerelease suffix' '\n+\ttest_config versionsort.prereleaseSuffix -very-looooooooooooooooooooooooong-prerelease-suffix &&\n+\tgit tag -l --sort=version:refname\n+'\n+\n run_with_limited_stack () {\n \t(ulimit -s 128 && \"$@\")\n }\ndiff --git a/versioncmp.c b/versioncmp.c\nindex 87b49a622..f86ac562e 100644\n--- a/versioncmp.c\n+++ b/versioncmp.c\n@@ -26,16 +26,15 @@ static int initialized;\n \n /*\n * off is the offset of the first different character in the two strings\n- * s1 and s2. If either s1 or s2 contains a prerelease suffix starting\n- * at that offset or the character at that offset is part of a\n- * prerelease suffix, then that string will be forced to be on top.\n+ * s1 and s2. If either s1 or s2 contains a prerelease suffix containing\n+ * that offset, then that string will be forced to be on top.\n *\n- * If both s1 and s2 contain a (different) suffix at that position, the\n- * order is determined by config file.\n- *\n- * Note that we don't have to deal with the situation when both s1 and\n- * s2 contain the same suffix because the common part is already\n- * consumed by the caller.\n+ * If both s1 and s2 contain a (different) suffix around that position,\n+ * their order is determined by the order of those two suffixes in the\n+ * configuration.\n+ * If any of the strings contains more than one different suffixes around\n+ * that position, then that string is sorted according to the contained\n+ * suffix which comes first in the configuration.\n *\n * Return non-zero if *diff contains the return value for versioncmp()\n */\n@@ -53,18 +52,16 @@ static int swap_prereleases(const char *s1,\n \t\t\tstart = off - suffix_len + 1;\n \t\telse\n \t\t\tstart = 0;\n-\t\tfor (j = start; j <= off; j++) {\n+\t\tfor (j = start; j <= off; j++)\n \t\t\tif (i1 == -1 && starts_with(s1 + j, suffix)) {\n \t\t\t\ti1 = i;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t}\n-\t\tfor (j = start; j <= off; j++) {\n+\t\tfor (j = start; j <= off; j++)\n \t\t\tif (i2 == -1 && starts_with(s2 + j, suffix)) {\n \t\t\t\ti2 = i;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t}\n \t}\n \tif (i1 == -1 && i2 == -1)\n \t\treturn 0;\n"},{"id":"307249","messageId":"20161208142401.1329-5-szeder.dev@gmail.com","threadId":"44016","inReplyTo":"20161208142401.1329-1-szeder.dev@gmail.com","subject":"[PATCHv2 4/7] versioncmp: pass full tagnames to swap_prereleases()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-08T14:23:58Z","receivedAt":"2016-12-08T14:24:56Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The swap_prereleases() helper function is responsible for finding\nconfigured prerelease suffixes in a pair of tagnames to be compared,\nbut this function currently gets to see only the parts of those two\ntagnames starting at the first different character. To fix some\nissues related to the common part of two tagnames overlapping with\nleading part of a prerelease suffix, this helper function must see\nboth full tagnames.\n\nIn preparation for the fix in the following patch, refactor\nswap_prereleases() and its caller to pass two full tagnames and an\nadditional offset indicating the position of the first different\ncharacter.\n\nWhile updating the comment describing that function, remove the\nsentence about not dealing with both tagnames having the same suffix.\nCurrently it doesn't add much value (we know that there is a different\ncharacter, so it's obvious that it can't possibly be the same suffix\nin both), and at the end of this patch series it won't even be true\nanymore.\n\nSigned-off-by: SZEDER Gábor \n---\n versioncmp.c | 29 +++++++++++++----------------\n 1 file changed, 13 insertions(+), 16 deletions(-)\n\ndiff --git a/versioncmp.c b/versioncmp.c\nindex 80bfd109f..a55c23ad5 100644\n--- a/versioncmp.c\n+++ b/versioncmp.c\n@@ -25,32 +25,28 @@ static const struct string_list *prereleases;\n static int initialized;\n \n /*\n- * p1 and p2 point to the first different character in two strings. If\n- * either p1 or p2 starts with a prerelease suffix, it will be forced\n- * to be on top.\n+ * off is the offset of the first different character in the two strings\n+ * s1 and s2. If either s1 or s2 contains a prerelease suffix starting\n+ * at that offset, it will be forced to be on top.\n *\n- * If both p1 and p2 start with (different) suffix, the order is\n- * determined by config file.\n- *\n- * Note that we don't have to deal with the situation when both p1 and\n- * p2 start with the same suffix because the common part is already\n- * consumed by the caller.\n+ * If both s1 and s2 contain a (different) suffix at that position,\n+ * their order is determined by the order of those two suffixes in the\n+ * configuration.\n *\n * Return non-zero if *diff contains the return value for versioncmp()\n */\n-static int swap_prereleases(const void *p1_,\n-\t\t\t const void *p2_,\n+static int swap_prereleases(const char *s1,\n+\t\t\t const char *s2,\n+\t\t\t int off,\n \t\t\t int *diff)\n {\n-\tconst char *p1 = p1_;\n-\tconst char *p2 = p2_;\n \tint i, i1 = -1, i2 = -1;\n \n \tfor (i = 0; i < prereleases->nr; i++) {\n \t\tconst char *suffix = prereleases->items[i].string;\n-\t\tif (i1 == -1 && starts_with(p1, suffix))\n+\t\tif (i1 == -1 && starts_with(s1 + off, suffix))\n \t\t\ti1 = i;\n-\t\tif (i2 == -1 && starts_with(p2, suffix))\n+\t\tif (i2 == -1 && starts_with(s2 + off, suffix))\n \t\t\ti2 = i;\n \t}\n \tif (i1 == -1 && i2 == -1)\n@@ -121,7 +117,8 @@ int versioncmp(const char *s1, const char *s2)\n \t\tinitialized = 1;\n \t\tprereleases = git_config_get_value_multi(\"versionsort.prereleasesuffix\");\n \t}\n-\tif (prereleases && swap_prereleases(p1 - 1, p2 - 1, &diff))\n+\tif (prereleases && swap_prereleases(s1, s2, (const char *) p1 - s1 - 1,\n+\t\t\t\t\t &diff))\n \t\treturn diff;\n \n \tstate = result_type[state * 3 + (((c2 == '0') + (isdigit (c2) != 0)))];\n-- \n2.11.0.78.g5a2d011\n\n"},{"id":"307252","messageId":"20161208144811.4372-1-szeder.dev@gmail.com","threadId":"44016","inReplyTo":"20161208142401.1329-7-szeder.dev@gmail.com","subject":"[PATCHv2 6.5/7] squash! versioncmp: use earliest-longest contained suffix to determine sorting order","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-08T14:48:11Z","receivedAt":"2016-12-08T14:48:38Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"As the number of identical steps to be done for both tagnames grows,\nextract them into a helper function, with the additional benefit that\nthe conditionals near the end of swap_prereleases() will use more\nmeaningful variable names.\n\nSigned-off-by: SZEDER Gábor \n---\n\nI was not particularly happy with the amount of code duplication this\nseries added to swap_prereleases() in patches 5 and 6, hence this bit\nof refactoring. Which I'm not particularly happy with either,\nbecause, well, look at that diffstat.\n\nI don't have strong feelings either way, but here it is, take it or\nleave it.\n\n versioncmp.c | 62 ++++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 37 insertions(+), 25 deletions(-)\n\ndiff --git a/versioncmp.c b/versioncmp.c\nindex 62c8d69dc..601ceddef 100644\n--- a/versioncmp.c\n+++ b/versioncmp.c\n@@ -24,6 +24,29 @@\n static const struct string_list *prereleases;\n static int initialized;\n \n+struct suffix_match {\n+\tint conf_pos;\n+\tint start;\n+\tint len;\n+};\n+\n+static void find_better_matching_suffix(const char *tagname, const char *suffix,\n+\t\t\t\t\tint suffix_len, int start, int conf_pos,\n+\t\t\t\t\tstruct suffix_match *match)\n+{\n+\t/* A better match either starts earlier or starts at the same offset\n+\t * but is longer. */\n+\tint end = match->len < suffix_len ? match->start : match->start-1;\n+\tint i;\n+\tfor (i = start; i <= end; i++)\n+\t\tif (starts_with(tagname + i, suffix)) {\n+\t\t\tmatch->conf_pos = conf_pos;\n+\t\t\tmatch->start = i;\n+\t\t\tmatch->len = suffix_len;\n+\t\t\tbreak;\n+\t\t}\n+}\n+\n /*\n * off is the offset of the first different character in the two strings\n * s1 and s2. If either s1 or s2 contains a prerelease suffix containing\n@@ -47,46 +70,35 @@ static int swap_prereleases(const char *s1,\n \t\t\t int off,\n \t\t\t int *diff)\n {\n-\tint i, i1 = -1, i2 = -1;\n-\tint start_at1 = off, start_at2 = off, match_len1 = -1, match_len2 = -1;\n+\tint i;\n+\tstruct suffix_match match1 = { -1, off, -1 };\n+\tstruct suffix_match match2 = { -1, off, -1 };\n \n \tfor (i = 0; i < prereleases->nr; i++) {\n \t\tconst char *suffix = prereleases->items[i].string;\n-\t\tint j, start, end, suffix_len = strlen(suffix);\n+\t\tint start, suffix_len = strlen(suffix);\n \t\tif (suffix_len < off)\n \t\t\tstart = off - suffix_len;\n \t\telse\n \t\t\tstart = 0;\n-\t\tend = match_len1 < suffix_len ? start_at1 : start_at1-1;\n-\t\tfor (j = start; j <= end; j++)\n-\t\t\tif (starts_with(s1 + j, suffix)) {\n-\t\t\t\ti1 = i;\n-\t\t\t\tstart_at1 = j;\n-\t\t\t\tmatch_len1 = suffix_len;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\tend = match_len2 < suffix_len ? start_at2 : start_at2-1;\n-\t\tfor (j = start; j <= end; j++)\n-\t\t\tif (starts_with(s2 + j, suffix)) {\n-\t\t\t\ti2 = i;\n-\t\t\t\tstart_at2 = j;\n-\t\t\t\tmatch_len2 = suffix_len;\n-\t\t\t\tbreak;\n-\t\t\t}\n+\t\tfind_better_matching_suffix(s1, suffix, suffix_len, start,\n+\t\t\t\t\t i, &match1);\n+\t\tfind_better_matching_suffix(s2, suffix, suffix_len, start,\n+\t\t\t\t\t i, &match2);\n \t}\n-\tif (i1 == -1 && i2 == -1)\n+\tif (match1.conf_pos == -1 && match2.conf_pos == -1)\n \t\treturn 0;\n-\tif (i1 == i2)\n+\tif (match1.conf_pos == match2.conf_pos)\n \t\t/* Found the same suffix in both, e.g. \"-rc\" in \"v1.0-rcX\"\n \t\t * and \"v1.0-rcY\": the caller should decide based on \"X\"\n \t\t * and \"Y\". */\n \t\treturn 0;\n \n-\tif (i1 >= 0 && i2 >= 0)\n-\t\t*diff = i1 - i2;\n-\telse if (i1 >= 0)\n+\tif (match1.conf_pos >= 0 && match2.conf_pos >= 0)\n+\t\t*diff = match1.conf_pos - match2.conf_pos;\n+\telse if (match1.conf_pos >= 0)\n \t\t*diff = -1;\n-\telse /* if (i2 >= 0) */\n+\telse /* if (match2.conf_pos >= 0) */\n \t\t*diff = 1;\n \treturn 1;\n }\n-- \n2.11.0.78.g5a2d011\n\n"},{"id":"307302","messageId":"xmqq60mutdpi.fsf@gitster.mtv.corp.google.com","threadId":"44016","inReplyTo":"20161208142401.1329-8-szeder.dev@gmail.com","subject":"Re: [PATCHv2 7/7] versioncmp: generalize version sort suffix reordering","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-08T19:36:57Z","receivedAt":"2016-12-08T19:37:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> The 'versionsort.prereleaseSuffix' configuration variable, as its name\n> suggests, is supposed to only deal with tagnames with prerelease\n> suffixes, and allows sorting those prerelease tags in a user-defined\n> order before the suffixless main release tag, instead of sorting them\n> simply lexicographically.\n>\n> However, the previous changes in this series resulted in an\n> interesting and useful property of version sort:\n>\n> - The empty string as a configured suffix matches all tagnames,\n> including tagnames without any suffix, but\n>\n> - tagnames containing a \"real\" configured suffix are still ordered\n> according to that real suffix, because any longer suffix takes\n> precedence over the empty string.\n>\n> Exploiting this property we can easily generalize suffix reordering\n> and specify the order of tags with given suffixes not only before but\n> even after a main release tag by using the empty suffix to denote the\n> position of the main release tag, without any algorithm changes:\n>\n> $ git -c versionsort.prereleaseSuffix=-alpha \\\n> -c versionsort.prereleaseSuffix=-beta \\\n> -c versionsort.prereleaseSuffix=\"\" \\\n> -c versionsort.prereleaseSuffix=-gamma \\\n> -c versionsort.prereleaseSuffix=-delta \\\n> tag -l --sort=version:refname 'v3.0*'\n> v3.0-alpha1\n> v3.0-beta1\n> v3.0\n> v3.0-gamma1\n> v3.0-delta1\n>\n> Since 'versionsort.prereleaseSuffix' is not a fitting name for a\n> configuration variable to control this more general suffix reordering,\n> introduce the new variable 'versionsort.suffix'. Still keep the old\n> configuration variable name as a deprecated alias, though, to avoid\n> suddenly breaking setups already using it. Ignore the old variable if\n> both old and new configuration variables are set, but emit a warning\n> so users will be aware of it and can fix their configuration. Extend\n> the documentation to describe and add a test to check this more\n> general behavior.\n>\n> Note: since the empty suffix matches all tagnames, tagnames with\n> suffixes not included in the configuration are listed together with\n> the suffixless main release tag, ordered lexicographically right after\n> that, i.e. before tags with suffixes listed in the configuration\n> following the empty suffix.\n\nThanks. Will comment on the individual patches later, but the end\nresult looks very nice.\n"},{"id":"307547","messageId":"xmqqshps96ti.fsf@gitster.mtv.corp.google.com","threadId":"44016","inReplyTo":"20161208142401.1329-6-szeder.dev@gmail.com","subject":"Re: [PATCHv2 5/7] versioncmp: cope with common part overlapping with prerelease suffix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-12T21:27:21Z","receivedAt":"2016-12-12T21:27:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> diff --git a/versioncmp.c b/versioncmp.c\n> index a55c23ad5..f86ac562e 100644\n> --- a/versioncmp.c\n> +++ b/versioncmp.c\n> @@ -26,12 +26,15 @@ static int initialized;\n> \n> /*\n> * off is the offset of the first different character in the two strings\n> - * s1 and s2. If either s1 or s2 contains a prerelease suffix starting\n> - * at that offset, it will be forced to be on top.\n> + * s1 and s2. If either s1 or s2 contains a prerelease suffix containing\n> + * that offset, then that string will be forced to be on top.\n> *\n> - * If both s1 and s2 contain a (different) suffix at that position,\n> + * If both s1 and s2 contain a (different) suffix around that position,\n> * their order is determined by the order of those two suffixes in the\n> * configuration.\n> + * If any of the strings contains more than one different suffixes around\n> + * that position, then that string is sorted according to the contained\n> + * suffix which comes first in the configuration.\n> *\n> * Return non-zero if *diff contains the return value for versioncmp()\n> */\n> @@ -44,10 +47,21 @@ static int swap_prereleases(const char *s1,\n> \n> \tfor (i = 0; i < prereleases->nr; i++) {\n> \t\tconst char *suffix = prereleases->items[i].string;\n> -\t\tif (i1 == -1 && starts_with(s1 + off, suffix))\n> -\t\t\ti1 = i;\n> -\t\tif (i2 == -1 && starts_with(s2 + off, suffix))\n> -\t\t\ti2 = i;\n> +\t\tint j, start, suffix_len = strlen(suffix);\n> +\t\tif (suffix_len < off)\n> +\t\t\tstart = off - suffix_len + 1;\n> +\t\telse\n> +\t\t\tstart = 0;\n\nNow that this function has to rewind the beginning of the comparison\nearlier than the given 'off', it makes me wonder if it still makes\nsense for the caller to compute it in the first place.\n\n> +\t\tfor (j = start; j <= off; j++)\n> +\t\t\tif (i1 == -1 && starts_with(s1 + j, suffix)) {\n> +\t\t\t\ti1 = i;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\tfor (j = start; j <= off; j++)\n> +\t\t\tif (i2 == -1 && starts_with(s2 + j, suffix)) {\n> +\t\t\t\ti2 = i;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \t}\n> \tif (i1 == -1 && i2 == -1)\n> \t\treturn 0;\n"},{"id":"307562","messageId":"CAM0VKjnPdt3keodXRFNit9=WKeY330N4T2t_dJuArgch7L6BNg@mail.gmail.com","threadId":"44016","inReplyTo":"xmqqshps96ti.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 5/7] versioncmp: cope with common part overlapping with prerelease suffix","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-13T00:27:35Z","receivedAt":"2016-12-13T00:27:41Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Dec 12, 2016 at 10:27 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>\n>> diff --git a/versioncmp.c b/versioncmp.c\n>> index a55c23ad5..f86ac562e 100644\n>> --- a/versioncmp.c\n>> +++ b/versioncmp.c\n>> @@ -26,12 +26,15 @@ static int initialized;\n>>\n>> /*\n>> * off is the offset of the first different character in the two strings\n>> - * s1 and s2. If either s1 or s2 contains a prerelease suffix starting\n>> - * at that offset, it will be forced to be on top.\n>> + * s1 and s2. If either s1 or s2 contains a prerelease suffix containing\n>> + * that offset, then that string will be forced to be on top.\n>> *\n>> - * If both s1 and s2 contain a (different) suffix at that position,\n>> + * If both s1 and s2 contain a (different) suffix around that position,\n>> * their order is determined by the order of those two suffixes in the\n>> * configuration.\n>> + * If any of the strings contains more than one different suffixes around\n>> + * that position, then that string is sorted according to the contained\n>> + * suffix which comes first in the configuration.\n>> *\n>> * Return non-zero if *diff contains the return value for versioncmp()\n>> */\n>> @@ -44,10 +47,21 @@ static int swap_prereleases(const char *s1,\n>>\n>> for (i = 0; i < prereleases->nr; i++) {\n>> const char *suffix = prereleases->items[i].string;\n>> - if (i1 == -1 && starts_with(s1 + off, suffix))\n>> - i1 = i;\n>> - if (i2 == -1 && starts_with(s2 + off, suffix))\n>> - i2 = i;\n>> + int j, start, suffix_len = strlen(suffix);\n>> + if (suffix_len < off)\n>> + start = off - suffix_len + 1;\n>> + else\n>> + start = 0;\n>\n> Now that this function has to rewind the beginning of the comparison\n> earlier than the given 'off', it makes me wonder if it still makes\n> sense for the caller to compute it in the first place.\n\nThe caller has to compute it anyway, because it must deal with all the\ncases when the two compared tagnames are not reordered based on their\n(prerelease)suffix.\n"},{"id":"307573","messageId":"xmqqbmwg72oo.fsf@gitster.mtv.corp.google.com","threadId":"44016","inReplyTo":"CAM0VKjnPdt3keodXRFNit9=WKeY330N4T2t_dJuArgch7L6BNg@mail.gmail.com","subject":"Re: [PATCHv2 5/7] versioncmp: cope with common part overlapping with prerelease suffix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-13T06:39:35Z","receivedAt":"2016-12-13T06:39:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n>>> - if (i1 == -1 && starts_with(s1 + off, suffix))\n>>> - i1 = i;\n>>> - if (i2 == -1 && starts_with(s2 + off, suffix))\n>>> - i2 = i;\n>>> + int j, start, suffix_len = strlen(suffix);\n>>> + if (suffix_len < off)\n>>> + start = off - suffix_len + 1;\n>>> + else\n>>> + start = 0;\n>>\n>> Now that this function has to rewind the beginning of the comparison\n>> earlier than the given 'off', it makes me wonder if it still makes\n>> sense for the caller to compute it in the first place.\n>\n> The caller has to compute it anyway, because it must deal with all the\n> cases when the two compared tagnames are not reordered based on their\n> (prerelease)suffix.\n\nSure.\n"},{"id":"307787","messageId":"20161214170852.bzh5pyl4bov6rwbt@sigill.intra.peff.net","threadId":"44016","inReplyTo":"20161208142401.1329-1-szeder.dev@gmail.com","subject":"Re: [PATCHv2 0/7] Fix and generalize version sort reordering","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-14T17:08:52Z","receivedAt":"2016-12-14T17:09:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 08, 2016 at 03:23:54PM +0100, SZEDER Gábor wrote:\n\n> > With my patches it looks like this:\n> > \n> > $ git -c versionsort.prereleasesuffix=-pre \\\n> > -c versionsort.prereleasesuffix=-prerelease \\\n> > tag -l --sort=version:refname\n> > v1.0.0-prerelease1\n> > v1.0.0-pre1\n> > v1.0.0-pre2\n> > v1.0.0\n> \n> And when there happen to be more than one matching suffixes starting\n> at the same earliest position, then we should pick the longest of\n> them. The new patch 6/7 implements this behavior.\n\nThe whole approach taken by the suffix code (before your patches) leaves\nme with the nagging feeling that the comparison is not always going to\nbe transitive (i.e., that \"a < b && b < c\" does not always imply \"a <\nc\"), which is going to cause nonsensical sorting results.\n\nAnd that may be part of the issue your 6/7 fixes.\n\nI spent some time playing with this the other day, though, and couldn't\ncome up with a specific example that fails the condition above.\n\nIt just seems like the whole thing would conceptually easier if we\npre-parsed the versions into a sequence of elements, then the comparison\nbetween any two elements would just walk that sequence. The benefit\nthere is that you can implement whatever rules you like for the parsing\n(like \"prefer longer suffixes to shorter\"), but you know the comparison\nwill always be consistent.\n\nIt would also be more efficient, I think (it seems like the sort is\nO(nr_tags * lg(nr_tags) * nr_suffixes) due to parsing suffixes in the\ncomparator). Though that probably doesn't matter much in practice.\n\nI dunno. I think maybe your 6/7 has converged on an equivalent behavior.\nAnd I am certainly not volunteering to re-write it, so if what you have\nworks, I'm not opposed to it.\n\n-Peff\n"},{"id":"307789","messageId":"xmqqzijyxviz.fsf@gitster.mtv.corp.google.com","threadId":"44016","inReplyTo":"20161214170852.bzh5pyl4bov6rwbt@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/7] Fix and generalize version sort reordering","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-14T17:36:36Z","receivedAt":"2016-12-14T17:37:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Dec 08, 2016 at 03:23:54PM +0100, SZEDER Gábor wrote:\n>\n>> > With my patches it looks like this:\n>> > \n>> > $ git -c versionsort.prereleasesuffix=-pre \\\n>> > -c versionsort.prereleasesuffix=-prerelease \\\n>> > tag -l --sort=version:refname\n>> > v1.0.0-prerelease1\n>> > v1.0.0-pre1\n>> > v1.0.0-pre2\n>> > v1.0.0\n>> \n>> And when there happen to be more than one matching suffixes starting\n>> at the same earliest position, then we should pick the longest of\n>> them. The new patch 6/7 implements this behavior.\n>\n> The whole approach taken by the suffix code (before your patches) leaves\n> me with the nagging feeling that the comparison is not always going to\n> be transitive (i.e., that \"a < b && b < c\" does not always imply \"a <\n> c\"), which is going to cause nonsensical sorting results.\n>\n> And that may be part of the issue your 6/7 fixes.\n>\n> I spent some time playing with this the other day, though, and couldn't\n> come up with a specific example that fails the condition above.\n>\n> It just seems like the whole thing would conceptually easier if we\n> pre-parsed the versions into a sequence of elements, then the comparison\n> between any two elements would just walk that sequence. The benefit\n> there is that you can implement whatever rules you like for the parsing\n> (like \"prefer longer suffixes to shorter\"), but you know the comparison\n> will always be consistent.\n>\n> It would also be more efficient, I think (it seems like the sort is\n> O(nr_tags * lg(nr_tags) * nr_suffixes) due to parsing suffixes in the\n> comparator). Though that probably doesn't matter much in practice.\n>\n> I dunno. I think maybe your 6/7 has converged on an equivalent behavior.\n> And I am certainly not volunteering to re-write it, so if what you have\n> works, I'm not opposed to it.\n\nI also had worries about transitiveness but couldn't break it after\ntrying for some time. I find your pre-parsing suggestion a great\none, not from the point of view of performance, but because I would\nimagine that the resulting logic would become a lot easier to\nunderstand.\n\n\n"},{"id":"308133","messageId":"CAM0VKjmDDKgYCvtbwpx=GcwRENzvSDLW_Xhia3btdeMjtAjAvg@mail.gmail.com","threadId":"44016","inReplyTo":"20161214170852.bzh5pyl4bov6rwbt@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/7] Fix and generalize version sort reordering","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2016-12-20T08:50:42Z","receivedAt":"2016-12-20T08:50:49Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Dec 14, 2016 at 6:08 PM, Jeff King wrote:\n> On Thu, Dec 08, 2016 at 03:23:54PM +0100, SZEDER Gábor wrote:\n>\n>> > With my patches it looks like this:\n>> >\n>> > $ git -c versionsort.prereleasesuffix=-pre \\\n>> > -c versionsort.prereleasesuffix=-prerelease \\\n>> > tag -l --sort=version:refname\n>> > v1.0.0-prerelease1\n>> > v1.0.0-pre1\n>> > v1.0.0-pre2\n>> > v1.0.0\n>>\n>> And when there happen to be more than one matching suffixes starting\n>> at the same earliest position, then we should pick the longest of\n>> them. The new patch 6/7 implements this behavior.\n>\n> The whole approach taken by the suffix code (before your patches) leaves\n> me with the nagging feeling that the comparison is not always going to\n> be transitive (i.e., that \"a < b && b < c\" does not always imply \"a <\n> c\"), which is going to cause nonsensical sorting results.\n>\n> And that may be part of the issue your 6/7 fixes.\n>\n> I spent some time playing with this the other day, though, and couldn't\n> come up with a specific example that fails the condition above.\n>\n> It just seems like the whole thing would conceptually easier if we\n> pre-parsed the versions into a sequence of elements, then the comparison\n> between any two elements would just walk that sequence. The benefit\n> there is that you can implement whatever rules you like for the parsing\n> (like \"prefer longer suffixes to shorter\"), but you know the comparison\n> will always be consistent.\n\nI considered parsing tagnames into prefix, version number and suffix,\nand then work from that, but decided against it.\n\nversioncmp() is taken from glibc, so I assume that it's thoroughly\ntested, even in corner cases (e.g. multiple leading zeros).\nFurthermore, I think it's a good thing that by default (i.e. without\nsuffix reordering) our version sort orders the same way as glibc's\nversion sort does. Introducing a different algorithm would risk bugs\nin the more subtle cases.\n\nThen there are all the weird release suffixes out there, and I didn't\nwant to decide on a policy for splitting them sanely; don't know\nwhether there exist any universal rules for this splitting at\nall. E.g. one of the packages here has the following version (let's\nignore the fact that because of the '~' this is an invalid refname in\ngit):\n\n 1.1.0~rc1-2ubuntu7-1linuxmint1\n\nNow, it's clear that the version number is \"1.1.0\", and the user\nshould configure the suffix \"~rc\" for prerelease reordering. But what\nabout the rest? How should we split it \"into a sequence of elements\",\nis it { \"1.1.0\", \"~rc1\", \"-2ubuntu7\", \"-1linuxmint1\" } or { \"1.1.0\",\n\"~rc1-2\", \"ubuntu7-1\", \"linuxmint1\" }?\nWhat if there is a hard-working developer who is involved in a lot of\nDebian derivatives (and derivatives of derivatives...), and, for\nwhatever reason, wants to put derivative-specific versions in a\nparticular order? With my series, or conceptually even with master if\nit weren't buggy, it's possible to specify the order of suffixes of\nsuffixes, and that dev could do this:\n\n $ git -c versionsort.suffix=-rc\n -c versionsort.suffix=linuxmint\n -c versionsort.suffix=YADoaDD\n tag -l --sort=version:refname\n '1.1.0*'\n 1.1.0-rc1-2ubuntu7-1linuxmint1\n 1.1.0-rc1-2ubuntu7-1YADoaDD2\n 1.1.0\n 1.1.0-2ubuntu7-1linuxmint1\n 1.1.0-2ubuntu7-1YADoaDD2\n\nand would get Linux Mint-specific tags before \"Yet Another Derivative\nof a Debian Derivative\"-specific ones. Not sure whether this is\nrelevant in practice, but I think it's a nice property nonetheless.\n\n(Btw, just for fun, I also found a package version\n2.0.0~beta2+isreally1.8.6-0ubuntu1. \"isreally\". Oh yeah :)\n\n> It would also be more efficient, I think (it seems like the sort is\n> O(nr_tags * lg(nr_tags) * nr_suffixes) due to parsing suffixes in the\n> comparator). Though that probably doesn't matter much in practice.\n\nI don't think there will be more than only a few configured suffixes\nin any repository. However, if you consider O as \"number of\nstarts_with() invocations\", then there is an additional suffix_length\nfactor. But then again, these suffixes tend to be short.\n\n> I dunno. I think maybe your 6/7 has converged on an equivalent behavior.\n> And I am certainly not volunteering to re-write it, so if what you have\n> works, I'm not opposed to it.\n>\n> -Peff\n"},{"id":"308147","messageId":"20161220164910.deanwq5hcve3vggt@sigill.intra.peff.net","threadId":"44016","inReplyTo":"CAM0VKjmDDKgYCvtbwpx=GcwRENzvSDLW_Xhia3btdeMjtAjAvg@mail.gmail.com","subject":"Re: [PATCHv2 0/7] Fix and generalize version sort reordering","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-20T16:49:11Z","receivedAt":"2016-12-20T16:49:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 20, 2016 at 09:50:42AM +0100, SZEDER Gábor wrote:\n\n> > It just seems like the whole thing would conceptually easier if we\n> > pre-parsed the versions into a sequence of elements, then the comparison\n> > between any two elements would just walk that sequence. The benefit\n> > there is that you can implement whatever rules you like for the parsing\n> > (like \"prefer longer suffixes to shorter\"), but you know the comparison\n> > will always be consistent.\n> \n> I considered parsing tagnames into prefix, version number and suffix,\n> and then work from that, but decided against it.\n> \n> versioncmp() is taken from glibc, so I assume that it's thoroughly\n> tested, even in corner cases (e.g. multiple leading zeros).\n> Furthermore, I think it's a good thing that by default (i.e. without\n> suffix reordering) our version sort orders the same way as glibc's\n> version sort does. Introducing a different algorithm would risk bugs\n> in the more subtle cases.\n\nFair enough. If it's working, I agree there is risk in changing things.\nAnd if you're willing to deal with the bugs, then I'm happy to stand\nback. :)\n\n> Then there are all the weird release suffixes out there, and I didn't\n> want to decide on a policy for splitting them sanely; don't know\n> whether there exist any universal rules for this splitting at\n> all. E.g. one of the packages here has the following version (let's\n> ignore the fact that because of the '~' this is an invalid refname in\n> git):\n\nI have a hunch that any policy you'd have to set for splitting is going\nto end up becoming a policy you'll have to use when comparing (or you\nrisk violating the transitivity of your comparison function).\n\nBut that's just a hunch, not a proof. Again, I'm happy to defer to you\nif you're the one working on it.\n\n-Peff\n"}]}