versioncmp: use earliest-longest contained suffix to determine sorting order

When comparing tagnames, it is possible that a tagname contains more
than one of the configured prerelease suffixes around the first
different character.  After fixing a bug in the previous commit such a
tagname is sorted according to the contained suffix which comes first
in the configuration.  This is, however, not quite the right thing to
do in the following corner cases:

1.   $ git -c versionsort.suffix=-bar
-c versionsort.suffix=-foo-baz
-c versionsort.suffix=-foo-bar
tag -l --sort=version:refname 'v1*'
v1.0-foo-bar
v1.0-foo-baz

The suffix of the tagname 'v1.0-foo-bar' is clearly '-foo-bar',
so it should be listed last.  However, as it also contains '-bar'
around the first different character, it is listed first instead,
because that '-bar' suffix comes first the configuration.

2. One of the configured suffixes starts with the other:

$ git -c versionsort.prereleasesuffix=-pre \
-c versionsort.prereleasesuffix=-prerelease \
tag -l --sort=version:refname 'v2*'
v2.0-prerelease1
v2.0-pre1
v2.0-pre2

Here the tagname 'v2.0-prerelease1' should be the last.  When
comparing 'v2.0-pre1' and 'v2.0-prerelease1' the first different
characters are '1' and 'r', respectively.  Since this first
different character must be part of the configured suffix, the
'-pre' suffix is not recognized in the first tagname.  OTOH, the
'-prerelease' suffix is properly recognized in
'v2.0-prerelease1', thus it is listed first.

Improve version sort in these corner cases, and

- look for a configured prerelease suffix containing the first
different character or ending right before it, so the '-pre'
suffixes are recognized in case (2).  This also means that
when comparing tagnames 'v2.0-pre1' and 'v2.0-pre2',
swap_prereleases() would find the '-pre' suffix in both, but then
it will return "undecided" and the caller will do the right thing
by sorting based in '1' and '2'.

- If the tagname contains more than one suffix, then give precedence
to the contained suffix that starts at the earliest offset in the
tagname to address (1).

- If there are more than one suffixes starting at that earliest
position, then give precedence to the longest of those suffixes,
thus ensuring that in (2) the tagname 'v2.0-prerelease1' won't be
sorted based on the '-pre' suffix.

Add tests for these corner cases and adjust the documentation
accordingly.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e053f9048876cfcd3a6110d38ec8aeea9baefc1..9078c8c4f4db140955c651c6e16f2f956042dbb8 100644 (file)
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3049,8 +3049,10 @@ order of suffixes in the config file determines the sorting order
 (e.g. if "-pre" appears before "-rc" in the config file then 1.0-preXX
 is sorted before 1.0-rcXX).
 If more than one suffixes match the same tagname, then that tagname will
 (e.g. if "-pre" appears before "-rc" in the config file then 1.0-preXX
 is sorted before 1.0-rcXX).
 If more than one suffixes match the same tagname, then that tagname will

-be sorted according to the matching suffix which comes first in the
-configuration.
+be sorted according to the suffix which starts at the earliest position in
+the tagname.  If more than one different matching suffixes start at
+that earliest position, then that tagname will be sorted according to the
+longest of those suffixes.

 The sorting order between different suffixes is undefined if they are
 in multiple config files.
 
 The sorting order between different suffixes is undefined if they are
 in multiple config files.
 

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index c7aaace8cde5474d0eed43a6ac283ad8dbd8bbb8..e2efe312dcf0ac985817417dd78d5fe128bd8349 100755 (executable)
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1564,6 +1564,37 @@ test_expect_success 'version sort with prerelease reordering, multiple suffixes
        test_cmp expect actual
 '
 
        test_cmp expect actual
 '
 

+test_expect_success 'version sort with prerelease reordering, multiple suffixes match the same tag' '
+       test_config versionsort.prereleaseSuffix -bar &&
+       git config --add versionsort.prereleaseSuffix -foo-baz &&
+       git config --add versionsort.prereleaseSuffix -foo-bar &&
+       git tag foo1.8-foo-bar &&
+       git tag foo1.8-foo-baz &&
+       git tag foo1.8 &&
+       git tag -l --sort=version:refname "foo1.8*" >actual &&
+       cat >expect <<-\EOF &&
+       foo1.8-foo-baz
+       foo1.8-foo-bar
+       foo1.8
+       EOF
+       test_cmp expect actual
+'
+
+test_expect_success 'version sort with prerelease reordering, multiple suffixes match starting at the same position' '
+       test_config versionsort.prereleaseSuffix -pre &&
+       git config --add versionsort.prereleaseSuffix -prerelease &&
+       git tag foo1.9-pre1 &&
+       git tag foo1.9-pre2 &&
+       git tag foo1.9-prerelease1 &&
+       git tag -l --sort=version:refname "foo1.9*" >actual &&
+       cat >expect <<-\EOF &&
+       foo1.9-pre1
+       foo1.9-pre2
+       foo1.9-prerelease1
+       EOF
+       test_cmp expect actual
+'
+

 test_expect_success 'version sort with very long prerelease suffix' '
        test_config versionsort.prereleaseSuffix -very-looooooooooooooooooooooooong-prerelease-suffix &&
        git tag -l --sort=version:refname
 test_expect_success 'version sort with very long prerelease suffix' '
        test_config versionsort.prereleaseSuffix -very-looooooooooooooooooooooooong-prerelease-suffix &&
        git tag -l --sort=version:refname

diff --git a/versioncmp.c b/versioncmp.c
index f86ac562e294c59c829ff6da60eb0f99c712c0b4..ae0a9199bd3b6dfb0d6d7c6e7399161efe8e0aa7 100644 (file)
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -27,14 +27,18 @@ static int initialized;
 /*
  * off is the offset of the first different character in the two strings
  * s1 and s2. If either s1 or s2 contains a prerelease suffix containing
 /*
  * off is the offset of the first different character in the two strings
  * s1 and s2. If either s1 or s2 contains a prerelease suffix containing

- * that offset, then that string will be forced to be on top.
+ * that offset or a suffix ends right before that offset, then that
+ * string will be forced to be on top.

  *
  * If both s1 and s2 contain a (different) suffix around that position,
  * their order is determined by the order of those two suffixes in the
  * configuration.
  * If any of the strings contains more than one different suffixes around
  * that position, then that string is sorted according to the contained
  *
  * If both s1 and s2 contain a (different) suffix around that position,
  * their order is determined by the order of those two suffixes in the
  * configuration.
  * If any of the strings contains more than one different suffixes around
  * that position, then that string is sorted according to the contained

- * suffix which comes first in the configuration.
+ * suffix which starts at the earliest offset in that string.
+ * If more than one different contained suffixes start at that earliest
+ * offset, then that string is sorted according to the longest of those
+ * suffixes.

  *
  * Return non-zero if *diff contains the return value for versioncmp()
  */
  *
  * Return non-zero if *diff contains the return value for versioncmp()
  */


@@ -44,27 +48,40 @@ static int swap_prereleases(const char *s1,
                            int *diff)
 {
        int i, i1 = -1, i2 = -1;
                            int *diff)
 {
        int i, i1 = -1, i2 = -1;

+       int start_at1 = off, start_at2 = off, match_len1 = -1, match_len2 = -1;

 
        for (i = 0; i < prereleases->nr; i++) {
                const char *suffix = prereleases->items[i].string;
 
        for (i = 0; i < prereleases->nr; i++) {
                const char *suffix = prereleases->items[i].string;

-               int j, start, suffix_len = strlen(suffix);
+               int j, start, end, suffix_len = strlen(suffix);

                if (suffix_len < off)
                if (suffix_len < off)

-                       start = off - suffix_len + 1;
+                       start = off - suffix_len;

                else
                        start = 0;
                else
                        start = 0;

-               for (j = start; j <= off; j++)
-                       if (i1 == -1 && starts_with(s1 + j, suffix)) {
+               end = match_len1 < suffix_len ? start_at1 : start_at1-1;
+               for (j = start; j <= end; j++)
+                       if (starts_with(s1 + j, suffix)) {

                                i1 = i;
                                i1 = i;

+                               start_at1 = j;
+                               match_len1 = suffix_len;

                                break;
                        }
                                break;
                        }

-               for (j = start; j <= off; j++)
-                       if (i2 == -1 && starts_with(s2 + j, suffix)) {
+               end = match_len2 < suffix_len ? start_at2 : start_at2-1;
+               for (j = start; j <= end; j++)
+                       if (starts_with(s2 + j, suffix)) {

                                i2 = i;
                                i2 = i;

+                               start_at2 = j;
+                               match_len2 = suffix_len;

                                break;
                        }
        }
        if (i1 == -1 && i2 == -1)
                return 0;
                                break;
                        }
        }
        if (i1 == -1 && i2 == -1)
                return 0;

+       if (i1 == i2)
+               /* Found the same suffix in both, e.g. "-rc" in "v1.0-rcX"
+                * and "v1.0-rcY": the caller should decide based on "X"
+                * and "Y". */
+               return 0;
+

        if (i1 >= 0 && i2 >= 0)
                *diff = i1 - i2;
        else if (i1 >= 0)
        if (i1 >= 0 && i2 >= 0)
                *diff = i1 - i2;
        else if (i1 >= 0)