Merge branch 'sg/fix-versioncmp-with-common-suffix'
authorJunio C Hamano <gitster@pobox.com>
Mon, 23 Jan 2017 23:59:21 +0000 (15:59 -0800)
committerJunio C Hamano <gitster@pobox.com>
Mon, 23 Jan 2017 23:59:21 +0000 (15:59 -0800)
The prereleaseSuffix feature of version comparison that is used in
"git tag -l" did not correctly when two or more prereleases for the
same release were present (e.g. when 2.0, 2.0-beta1, and 2.0-beta2
are there and the code needs to compare 2.0-beta1 and 2.0-beta2).

* sg/fix-versioncmp-with-common-suffix:
versioncmp: generalize version sort suffix reordering
versioncmp: factor out helper for suffix matching
versioncmp: use earliest-longest contained suffix to determine sorting order
versioncmp: cope with common part overlapping with prerelease suffix
versioncmp: pass full tagnames to swap_prereleases()
t7004-tag: add version sort tests to show prerelease reordering issues
t7004-tag: use test_config helper
t7004-tag: delete unnecessary tags with test_when_finished

Documentation/config.txt
Documentation/git-tag.txt
t/t7004-tag.sh
versioncmp.c
index 506431267ed0ef15bce2570d3f1bf69e7268e8f0..af2ae4cc02af75c5cf9395e228ff8bbc6e390181 100644 (file)
@@ -3113,17 +3113,39 @@ user.signingKey::
        This option is passed unchanged to gpg's --local-user parameter,
        so you may specify a key using any method that gpg supports.
 
-versionsort.prereleaseSuffix::
-       When version sort is used in linkgit:git-tag[1], prerelease
-       tags (e.g. "1.0-rc1") may appear after the main release
-       "1.0". By specifying the suffix "-rc" in this variable,
-       "1.0-rc1" will appear before "1.0".
-+
-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.
+versionsort.prereleaseSuffix (deprecated)::
+       Deprecated alias for `versionsort.suffix`.  Ignored if
+       `versionsort.suffix` is set.
+
+versionsort.suffix::
+       Even when version sort is used in linkgit:git-tag[1], tagnames
+       with the same base version but different suffixes are still sorted
+       lexicographically, resulting e.g. in prerelease tags appearing
+       after the main release (e.g. "1.0-rc1" after "1.0").  This
+       variable can be specified to determine the sorting order of tags
+       with different suffixes.
++
+By specifying a single suffix in this variable, any tagname containing
+that suffix will appear before the corresponding main release.  E.g. if
+the variable is set to "-rc", then all "1.0-rcX" tags will appear before
+"1.0".  If specified multiple times, once per suffix, then the order of
+suffixes in the configuration will determine the sorting order of tagnames
+with those suffixes.  E.g. if "-pre" appears before "-rc" in the
+configuration, then all "1.0-preX" tags will be listed before any
+"1.0-rcX" tags.  The placement of the main release tag relative to tags
+with various suffixes can be determined by specifying the empty suffix
+among those other suffixes.  E.g. if the suffixes "-rc", "", "-ck" and
+"-bfs" appear in the configuration in this order, then all "v4.8-rcX" tags
+are listed first, followed by "v4.8", then "v4.8-ckX" and finally
+"v4.8-bfsX".
++
+If more than one suffixes match the same tagname, then that tagname will
+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.
 
 web.browser::
        Specify a web browser that may be used by some commands.
index 76cfe40d969bc5f37dce674f43857ec89f25ae67..5055a9682393409c1ed07e1e417015f46e46a5bc 100644 (file)
@@ -101,8 +101,8 @@ OPTIONS
        multiple times, in which case the last key becomes the primary
        key. Also supports "version:refname" or "v:refname" (tag
        names are treated as versions). The "version:refname" sort
-       order can also be affected by the
-       "versionsort.prereleaseSuffix" configuration variable.
+       order can also be affected by the "versionsort.suffix"
+       configuration variable.
        The keys supported are the same as those in `git for-each-ref`.
        Sort order defaults to the value configured for the `tag.sort`
        variable if it exists, or lexicographic order otherwise. See
index 07869b0c09da1313dd5564c2318dc5d44818f3a9..1cfa8a21d23173e51e95906c00342fa13b85d955 100755 (executable)
@@ -149,11 +149,11 @@ test_expect_success '--force can create a tag with the name of one existing' '
        tag_exists mytag'
 
 test_expect_success '--force is moot with a non-existing tag name' '
+       test_when_finished git tag -d newtag forcetag &&
        git tag newtag >expect &&
        git tag --force forcetag >actual &&
        test_cmp expect actual
 '
-git tag -d newtag forcetag
 
 # deleting tags:
 
@@ -324,11 +324,9 @@ EOF
 '
 
 test_expect_success 'listing tags in column with column.*' '
-       git config column.tag row &&
-       git config column.ui dense &&
+       test_config column.tag row &&
+       test_config column.ui dense &&
        COLUMNS=40 git tag -l >actual &&
-       git config --unset column.ui &&
-       git config --unset column.tag &&
        cat >expected <<\EOF &&
 a1      aa1   cba     t210    t211
 v0.2.1  v1.0  v1.0.1  v1.1.3
@@ -341,9 +339,8 @@ test_expect_success 'listing tag with -n --column should fail' '
 '
 
 test_expect_success 'listing tags -n in column with column.ui ignored' '
-       git config column.ui "row dense" &&
+       test_config column.ui "row dense" &&
        COLUMNS=40 git tag -l -n >actual &&
-       git config --unset column.ui &&
        cat >expected <<\EOF &&
 a1              Foo
 aa1             Foo
@@ -1227,11 +1224,10 @@ test_expect_success GPG,RFC1991 \
 '
 
 # try to sign with bad user.signingkey
-git config user.signingkey BobTheMouse
 test_expect_success GPG \
        'git tag -s fails if gpg is misconfigured (bad key)' \
-       'test_must_fail git tag -s -m tail tag-gpg-failure'
-git config --unset user.signingkey
+       'test_config user.signingkey BobTheMouse &&
+       test_must_fail git tag -s -m tail tag-gpg-failure'
 
 # try to produce invalid signature
 test_expect_success GPG \
@@ -1511,7 +1507,7 @@ test_expect_success 'reverse lexical sort' '
 '
 
 test_expect_success 'configured lexical sort' '
-       git config tag.sort "v:refname" &&
+       test_config tag.sort "v:refname" &&
        git tag -l "foo*" >actual &&
        cat >expect <<-\EOF &&
        foo1.3
@@ -1522,6 +1518,7 @@ test_expect_success 'configured lexical sort' '
 '
 
 test_expect_success 'option override configured sort' '
+       test_config tag.sort "v:refname" &&
        git tag -l --sort=-refname "foo*" >actual &&
        cat >expect <<-\EOF &&
        foo1.6
@@ -1536,13 +1533,12 @@ test_expect_success 'invalid sort parameter on command line' '
 '
 
 test_expect_success 'invalid sort parameter in configuratoin' '
-       git config tag.sort "v:notvalid" &&
+       test_config tag.sort "v:notvalid" &&
        test_must_fail git tag -l "foo*"
 '
 
 test_expect_success 'version sort with prerelease reordering' '
-       git config --unset tag.sort &&
-       git config versionsort.prereleaseSuffix -rc &&
+       test_config versionsort.prereleaseSuffix -rc &&
        git tag foo1.6-rc1 &&
        git tag foo1.6-rc2 &&
        git tag -l --sort=version:refname "foo*" >actual &&
@@ -1557,6 +1553,7 @@ test_expect_success 'version sort with prerelease reordering' '
 '
 
 test_expect_success 'reverse version sort with prerelease reordering' '
+       test_config versionsort.prereleaseSuffix -rc &&
        git tag -l --sort=-version:refname "foo*" >actual &&
        cat >expect <<-\EOF &&
        foo1.10
@@ -1568,6 +1565,103 @@ test_expect_success 'reverse version sort with prerelease reordering' '
        test_cmp expect actual
 '
 
+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 &&
+       git tag foo1.7-after1 &&
+       git tag -l --sort=version:refname "foo1.7*" >actual &&
+       cat >expect <<-\EOF &&
+       foo1.7-before1
+       foo1.7
+       foo1.7-after1
+       EOF
+       test_cmp expect actual
+'
+
+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 &&
+       cat >expect <<-\EOF &&
+       foo1.7-before1
+       foo1.7-after1
+       foo1.7
+       EOF
+       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 general suffix reordering' '
+       test_config versionsort.suffix -alpha &&
+       git config --add versionsort.suffix -beta &&
+       git config --add versionsort.suffix ""  &&
+       git config --add versionsort.suffix -gamma &&
+       git config --add versionsort.suffix -delta &&
+       git tag foo1.10-alpha &&
+       git tag foo1.10-beta &&
+       git tag foo1.10-gamma &&
+       git tag foo1.10-delta &&
+       git tag foo1.10-unlisted-suffix &&
+       git tag -l --sort=version:refname "foo1.10*" >actual &&
+       cat >expect <<-\EOF &&
+       foo1.10-alpha
+       foo1.10-beta
+       foo1.10
+       foo1.10-unlisted-suffix
+       foo1.10-gamma
+       foo1.10-delta
+       EOF
+       test_cmp expect actual
+'
+
+test_expect_success 'versionsort.suffix overrides versionsort.prereleaseSuffix' '
+       test_config versionsort.suffix -before &&
+       test_config versionsort.prereleaseSuffix -after &&
+       git tag -l --sort=version:refname "foo1.7*" >actual &&
+       cat >expect <<-\EOF &&
+       foo1.7-before1
+       foo1.7
+       foo1.7-after1
+       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
+'
+
 run_with_limited_stack () {
        (ulimit -s 128 && "$@")
 }
@@ -1596,13 +1690,11 @@ EOF"
 
 test_expect_success '--format should list tags as per format given' '
        cat >expect <<-\EOF &&
-       refname : refs/tags/foo1.10
-       refname : refs/tags/foo1.3
-       refname : refs/tags/foo1.6
-       refname : refs/tags/foo1.6-rc1
-       refname : refs/tags/foo1.6-rc2
+       refname : refs/tags/v1.0
+       refname : refs/tags/v1.0.1
+       refname : refs/tags/v1.1.3
        EOF
-       git tag -l --format="refname : %(refname)" "foo*" >actual &&
+       git tag -l --format="refname : %(refname)" "v1*" >actual &&
        test_cmp expect actual
 '
 
index 80bfd109fa1285a24dbfa8e9aabdfc3b3bd59801..9f81dc1062bc1f7f243afa8c0404ff5100fc0ef8 100644 (file)
 static const struct string_list *prereleases;
 static int initialized;
 
+struct suffix_match {
+       int conf_pos;
+       int start;
+       int len;
+};
+
+static void find_better_matching_suffix(const char *tagname, const char *suffix,
+                                       int suffix_len, int start, int conf_pos,
+                                       struct suffix_match *match)
+{
+       /*
+        * A better match either starts earlier or starts at the same offset
+        * but is longer.
+        */
+       int end = match->len < suffix_len ? match->start : match->start-1;
+       int i;
+       for (i = start; i <= end; i++)
+               if (starts_with(tagname + i, suffix)) {
+                       match->conf_pos = conf_pos;
+                       match->start = i;
+                       match->len = suffix_len;
+                       break;
+               }
+}
+
 /*
- * p1 and p2 point to the first different character in two strings. If
- * either p1 or p2 starts with a prerelease suffix, it will be forced
- * to be on top.
- *
- * If both p1 and p2 start with (different) suffix, the order is
- * determined by config file.
+ * 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 or a suffix ends right before that offset, then that
+ * string will be forced to be on top.
  *
- * Note that we don't have to deal with the situation when both p1 and
- * p2 start with the same suffix because the common part is already
- * consumed by the caller.
+ * 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 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()
  */
-static int swap_prereleases(const void *p1_,
-                           const void *p2_,
+static int swap_prereleases(const char *s1,
+                           const char *s2,
+                           int off,
                            int *diff)
 {
-       const char *p1 = p1_;
-       const char *p2 = p2_;
-       int i, i1 = -1, i2 = -1;
+       int i;
+       struct suffix_match match1 = { -1, off, -1 };
+       struct suffix_match match2 = { -1, off, -1 };
 
        for (i = 0; i < prereleases->nr; i++) {
                const char *suffix = prereleases->items[i].string;
-               if (i1 == -1 && starts_with(p1, suffix))
-                       i1 = i;
-               if (i2 == -1 && starts_with(p2, suffix))
-                       i2 = i;
+               int start, suffix_len = strlen(suffix);
+               if (suffix_len < off)
+                       start = off - suffix_len;
+               else
+                       start = 0;
+               find_better_matching_suffix(s1, suffix, suffix_len, start,
+                                           i, &match1);
+               find_better_matching_suffix(s2, suffix, suffix_len, start,
+                                           i, &match2);
        }
-       if (i1 == -1 && i2 == -1)
+       if (match1.conf_pos == -1 && match2.conf_pos == -1)
                return 0;
-       if (i1 >= 0 && i2 >= 0)
-               *diff = i1 - i2;
-       else if (i1 >= 0)
+       if (match1.conf_pos == match2.conf_pos)
+               /* 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 (match1.conf_pos >= 0 && match2.conf_pos >= 0)
+               *diff = match1.conf_pos - match2.conf_pos;
+       else if (match1.conf_pos >= 0)
                *diff = -1;
-       else /* if (i2 >= 0) */
+       else /* if (match2.conf_pos >= 0) */
                *diff = 1;
        return 1;
 }
@@ -118,10 +159,18 @@ int versioncmp(const char *s1, const char *s2)
        }
 
        if (!initialized) {
+               const struct string_list *deprecated_prereleases;
                initialized = 1;
-               prereleases = git_config_get_value_multi("versionsort.prereleasesuffix");
+               prereleases = git_config_get_value_multi("versionsort.suffix");
+               deprecated_prereleases = git_config_get_value_multi("versionsort.prereleasesuffix");
+               if (prereleases) {
+                       if (deprecated_prereleases)
+                               warning("ignoring versionsort.prereleasesuffix because versionsort.suffix is set");
+               } else
+                       prereleases = deprecated_prereleases;
        }
-       if (prereleases && swap_prereleases(p1 - 1, p2 - 1, &diff))
+       if (prereleases && swap_prereleases(s1, s2, (const char *) p1 - s1 - 1,
+                                           &diff))
                return diff;
 
        state = result_type[state * 3 + (((c2 == '0') + (isdigit (c2) != 0)))];