versioncmp: cope with common part overlapping with prerelease suffix

Version sort with prerelease reordering sometimes puts tagnames in the
wrong order, when the common part of two compared tagnames overlaps
with the leading character(s) of one or more configured prerelease
suffixes.  Note the position of "v2.1.0-beta-1":

$ git -c versionsort.prereleaseSuffix=-beta \
tag -l --sort=version:refname v2.1.*
v2.1.0-beta-2
v2.1.0-beta-3
v2.1.0
v2.1.0-RC1
v2.1.0-RC2
v2.1.0-beta-1
v2.1.1
v2.1.2

The reason is that when comparing a pair of tagnames, first
versioncmp() looks for the first different character in a pair of
tagnames, and then the swap_prereleases() helper function looks for a
configured prerelease suffix _starting at_ that character.  Thus, when
in the above example the sorting algorithm happens to compare the
tagnames "v2.1.0-beta-1" and "v2.1.0-RC2", swap_prereleases() tries to
match the suffix "-beta" against "beta-1" to no avail, and the two
tagnames erroneously end up being ordered lexicographically.

To fix this issue change swap_prereleases() to look for configured
prerelease suffixes _containing_ the position of that first different
character.

Care must be taken, when a configured suffix is longer than the
tagnames' common part up to the first different character, to avoid
reading memory before the beginning of the tagnames.  Add a test that
uses an exceptionally long prerelease suffix to check for this, in the
hope that in case of a regression the illegal memory access causes a
segfault in 'git tag' on one of the commonly used platforms (the test
happens to pass successfully on my Linux system with the safety check
removed), or at least makes valgrind complain.

Under some circumstances it's possible that more than one prerelease
suffixes can be found in the same tagname around that first different
character.  With this simple bugfix patch such a tagname is sorted
according to the contained suffix that comes first in the
configuration for now.  This is less than ideal in some cases, and the
following patch will take care of those.

Reported-by: Leho Kraav <leho@conversionready.com>
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 a0ab66aae70db90bd18e14ec5bc5c95007b118f7..2e053f9048876cfcd3a6110d38ec8aeea9baefc1 100644 (file)
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3047,8 +3047,12 @@ versionsort.prereleaseSuffix::
 This variable can be specified multiple times, once per suffix. The
 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
 This variable can be specified multiple times, once per suffix. The
 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). The sorting order between different
-suffixes is undefined if they are in multiple config files.
+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.
+The sorting order between different suffixes is undefined if they are
+in multiple config files.

 
 web.browser::
        Specify a web browser that may be used by some commands.
 
 web.browser::
        Specify a web browser that may be used by some commands.

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 6445aae29ba77cfad14a74a97d87852cd9c80379..c7aaace8cde5474d0eed43a6ac283ad8dbd8bbb8 100755 (executable)
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1538,7 +1538,7 @@ test_expect_success 'reverse version sort with prerelease reordering' '
        test_cmp expect actual
 '
 
        test_cmp expect actual
 '
 

-test_expect_failure 'version sort with prerelease reordering and common leading character' '
+test_expect_success 'version sort with prerelease reordering and common leading character' '

        test_config versionsort.prereleaseSuffix -before &&
        git tag foo1.7-before1 &&
        git tag foo1.7 &&
        test_config versionsort.prereleaseSuffix -before &&
        git tag foo1.7-before1 &&
        git tag foo1.7 &&


@@ -1552,7 +1552,7 @@ test_expect_failure 'version sort with prerelease reordering and common leading
        test_cmp expect actual
 '
 
        test_cmp expect actual
 '
 

-test_expect_failure 'version sort with prerelease reordering, multiple suffixes and common leading character' '
+test_expect_success 'version sort with prerelease reordering, multiple suffixes and common leading character' '

        test_config versionsort.prereleaseSuffix -before &&
        git config --add versionsort.prereleaseSuffix -after &&
        git tag -l --sort=version:refname "foo1.7*" >actual &&
        test_config versionsort.prereleaseSuffix -before &&
        git config --add versionsort.prereleaseSuffix -after &&
        git tag -l --sort=version:refname "foo1.7*" >actual &&


@@ -1564,6 +1564,11 @@ test_expect_failure 'version sort with prerelease reordering, multiple suffixes
        test_cmp expect actual
 '
 
        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
+'
+

 run_with_limited_stack () {
        (ulimit -s 128 && "$@")
 }
 run_with_limited_stack () {
        (ulimit -s 128 && "$@")
 }

diff --git a/versioncmp.c b/versioncmp.c
index a55c23ad57041a6f01b206e940706afb29cf21af..f86ac562e294c59c829ff6da60eb0f99c712c0b4 100644 (file)
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -26,12 +26,15 @@ static int initialized;
 
 /*
  * off is the offset of the first different character in the two strings
 
 /*
  * 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 starting
- * at that offset, it will be forced to be on top.
+ * 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.

  *
  *

- * If both s1 and s2 contain a (different) suffix at that position,
+ * 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.
  * 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.

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


@@ -44,10 +47,21 @@ static int swap_prereleases(const char *s1,
 
        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;

-               if (i1 == -1 && starts_with(s1 + off, suffix))
-                       i1 = i;
-               if (i2 == -1 && starts_with(s2 + off, suffix))
-                       i2 = i;
+               int j, start, suffix_len = strlen(suffix);
+               if (suffix_len < off)
+                       start = off - suffix_len + 1;
+               else
+                       start = 0;
+               for (j = start; j <= off; j++)
+                       if (i1 == -1 && starts_with(s1 + j, suffix)) {
+                               i1 = i;
+                               break;
+                       }
+               for (j = start; j <= off; j++)
+                       if (i2 == -1 && starts_with(s2 + j, suffix)) {
+                               i2 = i;
+                               break;
+                       }

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