Fix vercmp and add additional tests

This vercmp issue has been a sticking point but this should resolve many of
the issues that have come up. Only a few minor code changes were necessary
to get the behavior we desired, and this version appears to beat any other
vercmp rendition on a few more cases added in this commit.

This commit passes all 58 vercmp tests currently out there. Other 'fixes'
still fail on a few tests, namely these ones:

test: ver1: 1.5.a ver2: 1.5 ret: -1 expected: 1
  ==> FAILURE
test: ver1: 1.5 ver2: 1.5.a ret: 1 expected: -1
  ==> FAILURE
test: ver1: 1.5-1 ver2: 1.5.b ret: 1 expected: -1
  ==> FAILURE
test: ver1: 1.5.b ver2: 1.5-1 ret: -1 expected: 1
  ==> FAILURE
4 of 58 tests failed

Signed-off-by: Dan McGee <dan@archlinux.org>

doc/pacman.8.txt

@@ -67,7 +67,7 @@ You can also use `pacman -Su` to upgrade all packages that are out of date. See
 to determine which packages need upgrading. This behavior operates as follows:
 
   Alphanumeric:
-    1.0 < 1.0a < 1.0alpha < 1.0b < 1.0beta < 1.0p < 1.0pre < 1.0rc
+    1.0a < 1.0alpha < 1.0b < 1.0beta < 1.0p < 1.0pre < 1.0rc < 1.0
   Numeric:
     1 < 1.0 < 1.1 < 1.1.1 < 1.2 < 2.0 < 3.0.0
 

lib/libalpm/package.c

@@ -691,17 +691,13 @@ int SYMEXPORT alpm_pkg_vercmp(const char *a, const char *b)
 		if(*ptr1 == '-' && *ptr2 == '-') {
 			/* no-op, continue comparing since we are equivalent throughout */
 		} else if(*ptr1 == '-') {
-			/* ptr1 has hit the pkgrel and ptr2 has not.
+			/* ptr1 has hit the pkgrel and ptr2 has not. continue version
-			 * version 2 is newer iff we are not at the end of ptr2;
+			 * comparison after stripping the pkgrel from ptr1. */
-			 * if we are at end then one version had pkgrel and one did not */
+			*ptr1 = '\0';
-			ret = *ptr2 ? -1 : 0;
-			goto cleanup;
 		} else if(*ptr2 == '-') {
-			/* ptr2 has hit the pkgrel and ptr1 has not.
+			/* ptr2 has hit the pkgrel and ptr1 has not. continue version
-			 * version 1 is newer iff we are not at the end of ptr1;
+			 * comparison after stripping the pkgrel from ptr2. */
-			 * if we are at end then one version had pkgrel and one did not */
+			*ptr2 = '\0';
-			ret = *ptr1 ? 1 : 0;
-			goto cleanup;
 		}
 	}
 
@@ -713,8 +709,14 @@ int SYMEXPORT alpm_pkg_vercmp(const char *a, const char *b)
 		goto cleanup;
 	}
 
-	/* whichever version still has characters left over wins */
+	/* the final showdown. we never want a remaining alpha string to
-	if (!*one) {
+	 * beat an empty string. the logic is a bit weird, but:
+	 * - if one is empty and two is not an alpha, two is newer.
+	 * - if one is an alpha, two is newer.
+	 * - otherwise one is newer.
+	 * */
+	if ( ( !*one && !isalpha((int)*two) )
+			|| isalpha((int)*one) ) {
 		ret = -1;
 	} else {
 		ret = 1;

pactest/vercmptest.sh

@@ -26,8 +26,9 @@ failure=0
 # args:
 # pass ver1 ver2 ret expected
 pass() {
-	echo "test: ver1: $1 ver2: $2 ret: $3 expected: $4"
+	#echo "test: ver1: $1 ver2: $2 ret: $3 expected: $4"
-	echo "  --> pass"
+	#echo "  --> pass"
+	echo -n
 }
 
 # args:
@@ -54,13 +55,20 @@ runtest() {
 	ret=$($bin $2 $1)
 	func='pass'
 	[ $ret -eq $reverse ] || func='fail'
-	$func $1 $2 $ret $reverse
+	$func $2 $1 $ret $reverse
 	total=$(expr $total + 1)
 }
 
 # use first arg as our binary if specified
 [ -n "$1" ] && bin="$1"
 
+if [ ! -x "$bin" ]; then
+	echo "vercmp binary ($bin) could not be located"
+	exit 1
+fi
+
+echo "Beginning vercmp tests"
+
 # BEGIN TESTS
 
 # all similar length, no pkgrel
@@ -88,6 +96,28 @@ runtest 1.1-1 1.1   0
 runtest 1.0-1 1.1  -1
 runtest 1.1-1 1.0   1
 
+# alphanumeric versions
+runtest 1.5b-1  1.5-1  -1
+runtest 1.5b    1.5    -1
+runtest 1.5b-1  1.5    -1
+runtest 1.5b    1.5.1  -1
+
+# from the manpage
+runtest 1.0a     1.0alpha -1
+runtest 1.0alpha 1.0b     -1
+runtest 1.0b     1.0beta  -1
+runtest 1.0beta  1.0rc    -1
+runtest 1.0rc    1.0      -1
+
+# going crazy? alpha-dotted versions
+runtest 1.5.a    1.5     1
+runtest 1.5.b    1.5.a   1
+runtest 1.5.1    1.5.b   1
+
+# alpha dots and dashes
+runtest 1.5.b-1  1.5.b   0
+runtest 1.5-1    1.5.b  -1
+
 #END TESTS
 
 echo