[PATCH 1/3] Compare package versions, not setup.ini timestamps

[Jon TURNEY <jon dot turney at dronecode dot org dot uk>]

Currently, when two mirrors offer different versions of a package for a stability
level, the one from the setup.ini with the latest timestamp is chosen. This works
adequately when the mirrors are exactly that, so the one which has been updated most
recently has the most up-to-date version for all packages.  But this makes it difficult
to have an overlay, a site which offers updated versions of a subset of packages,
as there's no way to reliably ensure setup selects those updated versions (without
massaging the timestamps in the setup.ini files after downloading them)

So, instead of comparing the setup.ini timestamps, compare the package version numbers
to determine which package is most recent.

This shouldn't change the current behaviour in the simple case where only one mirror
is selected (provided no packages have negative version numbers).

2010-11-06  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* package_version.h (packageversion): Add compareVersion() utility
	function.
	* package_version.cc (Vendor_version, Package_version)
	(compareVersions): Implement the Vendor_version() and Package_version()
	accessor functions.  Add compareVersions() utility function.
	* cygpackage.cc (setCanonicalVersion): Fix to extract vendor version
	correctly.
	* IniDBBuilderPackage.cc (add_correct_version): When multiple setup.ini's
	offer different packages for the same stability level, use the package with
	the highest version number rather than the package coming from the setup.ini
	with the latest timestamp.
---
 IniDBBuilderPackage.cc |   54 ++++++++++++++++++++++++++++++++---------------
 cygpackage.cc          |   17 ++++++++-------
 package_version.cc     |   37 ++++++++++++++++++++++++++++++++
 package_version.h      |    3 ++
 4 files changed, 86 insertions(+), 25 deletions(-)

diff --git a/IniDBBuilderPackage.cc b/IniDBBuilderPackage.cc
index 70de988..50094fe 100644
--- a/IniDBBuilderPackage.cc
+++ b/IniDBBuilderPackage.cc
@@ -505,6 +505,10 @@ IniDBBuilderPackage::add_correct_version()
            categories and requires are consistent for the same version across
            all mirrors
            */
+        /*
+          XXX: if the versions are equal but the size/md5sum are different,
+          we should alert the user, as they may not be getting what they expect...
+        */
         /* Copy the binary mirror across if this site claims to have an install */
         if (cbpv.source()->sites.size() )
           ver.source()->sites.push_back(site (cbpv.source()->sites.begin()->key));
@@ -522,34 +526,50 @@ IniDBBuilderPackage::add_correct_version()
 	currentSpec = NULL;
         cbpv = *n;
         merged = 1;
+#if DEBUG
+        log (LOG_BABBLE) << cp->name << " merged with an existing version " << cbpv.Canonical_version() << endLog;
+#endif
       }
+
   if (!merged)
-    cp->add_version (cbpv);
-  /* trust setting */
+    {
+      cp->add_version (cbpv);
+#if DEBUG
+      log (LOG_BABBLE) << cp->name << " version " << cbpv.Canonical_version() << " added" << endLog;
+#endif
+    }
+
+  /*
+    Should this version be the one selected for this package at a given
+    stability/trust setting?  After merging potentially multiple package
+    databases, we should pick the one with the highest version number.
+  */
+  packageversion *v = NULL;
   switch (trust)
   {
     case TRUST_CURR:
-      if (cp->currtimestamp < timestamp)
-      {
-        cp->currtimestamp = timestamp;
-        cp->curr = cbpv;
-      }
+      v = &(cp->curr);
     break;
     case TRUST_PREV:
-    if (cp->prevtimestamp < timestamp)
-    {
-        cp->prevtimestamp = timestamp;
-        cp->prev = cbpv;
-    }
+      v = &(cp->prev);
     break;
     case TRUST_TEST:
-    if (cp->exptimestamp < timestamp)
-    {
-        cp->exptimestamp = timestamp;
-        cp->exp = cbpv;
-    }
+      v = &(cp->exp);
     break;
   }
+
+  if (v)
+    {
+      int comparison = packageversion::compareVersions(cbpv, *v);
+
+      if ((bool)(*v))
+        log (LOG_BABBLE) << "package " << cp->name << " comparing versions " << cbpv.Canonical_version() << " and " << v->Canonical_version() << ", result was " << comparison << endLog;
+
+      if (comparison > 0)
+        {
+          *v = cbpv;
+        }
+    }
 }
 
 void
diff --git a/cygpackage.cc b/cygpackage.cc
index ec79f7c..0189271 100644
--- a/cygpackage.cc
+++ b/cygpackage.cc
@@ -85,19 +85,20 @@ void
 cygpackage::setCanonicalVersion (const std::string& version)
 {
   canonical = version;
-  char *start = strchr (canonical.c_str(), '-');
-  char*curr=start;
+
+  const char *start = canonical.c_str();
+  const char *curr = strchr(start, '-');
+
   if (curr)
     {
-      char *next;
+      const char *next;
       while ((next = strchr (curr + 1, '-')))
 	curr = next;
-      /* curr = last - in the version string */
+
+      /* package version appears after the last '-' in the version string */
       packagev = curr + 1;
-      char tvendor [version.size() +1];
-      strcpy (tvendor, version.c_str());
-      tvendor[curr - start] = '\0';
-      vendor=tvendor;
+      /* vendor version is everything up to that last '-' */
+      vendor.assign(canonical.c_str(), (size_t)(curr - start));
     }
   else
     {
diff --git a/package_version.cc b/package_version.cc
index 26dbd82..e5a6369 100644
--- a/package_version.cc
+++ b/package_version.cc
@@ -30,6 +30,7 @@ static const char *cvsid =
 #include <algorithm>
 #include "download.h"
 #include "Exception.h"
+#include "csu_util/version_compare.h"
 
 using namespace std;
 
@@ -151,6 +152,18 @@ packageversion::Name () const
 }
 
 const std::string
+packageversion::Vendor_version() const
+{
+  return data->Vendor_version();
+}
+
+const std::string
+packageversion::Package_version() const
+{
+  return data->Package_version();
+}
+
+const std::string
 packageversion::Canonical_version() const
 {
   return data->Canonical_version();
@@ -495,6 +508,30 @@ packageversion::scripts()
   return data->scripts();
 }
 
+int
+packageversion::compareVersions(packageversion a, packageversion b)
+{
+  /* Compare Vendor_version */
+  int comparison = version_compare(a.Vendor_version(), b.Vendor_version());
+ 
+#if DEBUG
+  log (LOG_BABBLE) << "vendor version comparison " << a.Vendor_version() << " and " << b.Vendor_version() << ", result was " << comparison << endLog;
+#endif
+
+  if (comparison != 0)
+    {
+      return comparison;
+    }
+
+  /* Vendor_version are tied, compare Package_version */
+#if DEBUG
+  log (LOG_BABBLE) <<  "package version comparison " << a.Package_version() << " and " << b.Package_version() << ", result was " << comparison << endLog;
+#endif
+
+  comparison = version_compare(a.Package_version(), b.Package_version());
+  return comparison;
+}
+
 /* the parent data class */
   
 _packageversion::_packageversion ():picked (false), references (0)
diff --git a/package_version.h b/package_version.h
index 26a286e..7c6590c 100644
--- a/package_version.h
+++ b/package_version.h
@@ -145,6 +145,9 @@ public:
   void addScript(Script const &);
   std::vector <Script> &scripts();
 
+  /* utility function to compare package versions */
+  static int compareVersions(packageversion a, packageversion b);
+
 private:
   _packageversion *data; /* Invariant: * data is always valid */
 };
-- 
1.7.2.3