Re: Bug#574956: dpkg drops zero-epoch in status file

[Raphael Hertzog <hertzog@debian.org>]

On Fri, 30 Apr 2010, Guillem Jover wrote:
> > It is relatively easy for apt to ignore the epoch for the hash as it is
> > a simple hash and we don't need to care about false positive removes
> > so apt still doesn't need to do expensive parsing here, but i want
> > to ask dpkg maintainers (cc'ed and titled to grep their attension)
> > for their opinion as this is maybe something they want to change
> > instead in their logic for consistence…
> > (through no zero epoch could be a change to be consistence…).
> 
> I don't think dpkg should stop removing the zero-epoch, it would be
> redundant and confusing information. But it might be a good idea to
> make dpkg-gencontrol for example drop it on output. And it seems that
> apt might need to consider the other equivalent versions too.

I don't agree with this. The perl API preserve all the information
required to be able to output exactly the same string that has been parsed
and I don't see why the C part should not be able to do the same.

Please find attached a patch that implements this. I added non-regression
tests and verified that the epoch is preserved with the package that
generated this request. I'd like to commit this to the master branch so a
review is welcome.

That said maybe dpkg-gencontrol should indeed simplify the output but
that's a different matter. I recorded that wish in a somewhat related
bug report.

Cheers,
-- 
Raphaël Hertzog

Like what I do? Sponsor me: http://ouaza.com/wp/2010/01/05/5-years-of-freexian/
My Debian goals: http://ouaza.com/wp/2010/01/09/debian-related-goals-for-2010/

>From d61adb3e7f7154ade8efd260d6ab4b64bde85d2f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rapha=C3=ABl=20Hertzog?= <hertzog@debian.org>
Date: Sun, 2 May 2010 09:59:29 +0200
Subject: [PATCH 1/2] libdpkg: remember whether a version string had the epoch/revision when parsed

For the epoch, we add a new boolean attribute "epoch_present" to the
versionrevision struct. For the revision, we ensure that a NULL value
means "no revision" and that an empty revision really represents an empty
revision ("1.0-").

Add non-regression tests to check that the output function preserves
the original string when it has been parsed by parseversion().
---
 lib/dpkg/database.c       |    1 +
 lib/dpkg/dpkg-db.h        |    8 +++-
 lib/dpkg/parsehelp.c      |   17 +++++--
 lib/dpkg/test/t-version.c |  118 +++++++++++++++++++++++++++++++++++++--------
 src/enquiry.c             |    8 ++--
 5 files changed, 121 insertions(+), 31 deletions(-)

diff --git a/lib/dpkg/database.c b/lib/dpkg/database.c
index e982372..3f9d017 100644
--- a/lib/dpkg/database.c
+++ b/lib/dpkg/database.c
@@ -59,6 +59,7 @@ static unsigned int hash(const char *name) {
 void blankversion(struct versionrevision *version) {
   version->epoch= 0;
   version->version= version->revision= NULL;
+  version->epoch_present = false;
 }
 
 void blankpackage(struct pkginfo *pigp) {
diff --git a/lib/dpkg/dpkg-db.h b/lib/dpkg/dpkg-db.h
index 9ad461c..6dd01af 100644
--- a/lib/dpkg/dpkg-db.h
+++ b/lib/dpkg/dpkg-db.h
@@ -36,6 +36,7 @@ struct versionrevision {
   unsigned long epoch;
   const char *version;
   const char *revision;
+  bool epoch_present;
 };  
 
 enum deptype {
@@ -260,7 +261,12 @@ extern const struct namevalue wantinfos[];
 
 int informativeversion(const struct versionrevision *version);
 
-enum versiondisplayepochwhen { vdew_never, vdew_nonambig, vdew_always };
+enum versiondisplayepochwhen {
+  vdew_never,    /* Always hide the epoch */
+  vdew_nonambig, /* Display it only when required (!= 0, other : in version) */
+  vdew_always,   /* Always display it */
+  vdew_parsed    /* Keep same output as what has been parsed */
+};
 void varbufversion(struct varbuf*, const struct versionrevision*,
                    enum versiondisplayepochwhen);
 const char *parseversion(struct versionrevision *rversion, const char*);
diff --git a/lib/dpkg/parsehelp.c b/lib/dpkg/parsehelp.c
index a37df93..4c98e21 100644
--- a/lib/dpkg/parsehelp.c
+++ b/lib/dpkg/parsehelp.c
@@ -173,21 +173,26 @@ void varbufversion
  const struct versionrevision *version,
  enum versiondisplayepochwhen vdew) 
 {
+  bool epoch_required = version->epoch ||
+    (version->version && strchr(version->version, ':')) ||
+    (version->revision && strchr(version->revision, ':'));
   switch (vdew) {
   case vdew_never:
     break;
   case vdew_nonambig:
-    if (!version->epoch &&
-        (!version->version || !strchr(version->version,':')) &&
-        (!version->revision || !strchr(version->revision,':'))) break;
+    if (!epoch_required) break;
   case vdew_always: /* FALL THROUGH */
     varbufprintf(vb,"%lu:",version->epoch);
     break;
+  case vdew_parsed:
+    if (version->epoch_present || epoch_required)
+      varbufprintf(vb, "%lu:", version->epoch);
+    break;
   default:
     internerr("unknown versiondisplayepochwhen '%d'", vdew);
   }
   if (version->version) varbufaddstr(vb,version->version);
-  if (version->revision && *version->revision) {
+  if (version->revision) {
     varbufaddc(vb,'-');
     varbufaddstr(vb,version->revision);
   }
@@ -240,14 +245,16 @@ const char *parseversion(struct versionrevision *rversion, const char *string) {
     if (!*++colon) return _("nothing after colon in version number");
     string= colon;
     rversion->epoch= epoch;
+    rversion->epoch_present = true;
   } else {
     rversion->epoch= 0;
+    rversion->epoch_present = false;
   }
   rversion->version= nfstrnsave(string,end-string);
   hyphen= strrchr(rversion->version,'-');
   if (hyphen)
     *hyphen++ = '\0';
-  rversion->revision= hyphen ? hyphen : "";
+  rversion->revision = hyphen; /* NULL means no revision */
   
   return NULL;
 }
diff --git a/lib/dpkg/test/t-version.c b/lib/dpkg/test/t-version.c
index 8355c6d..950c93d 100644
--- a/lib/dpkg/test/t-version.c
+++ b/lib/dpkg/test/t-version.c
@@ -24,8 +24,8 @@
 #include <dpkg/test.h>
 #include <dpkg/dpkg-db.h>
 
-#define version(epoch, version, revision) \
-	(struct versionrevision) { (epoch), (version), (revision) }
+#define version(epoch, version, revision, epoch_present) \
+	(struct versionrevision) { (epoch), (version), (revision), (epoch_present) }
 
 static void
 test_version_compare(void)
@@ -41,19 +41,19 @@ test_version_compare(void)
 	test_pass(epochsdiffer(&a, &b));
 
 	/* Test for version equality. */
-	a = b = version(0, "0", "0");
+	a = b = version(0, "0", "0", false);
 	test_pass(versioncompare(&a, &b) == 0);
 
-	a = version(0, "0", "00");
-	b = version(0, "00", "0");
+	a = version(0, "0", "00", false);
+	b = version(0, "00", "0", true);
 	test_pass(versioncompare(&a, &b) == 0);
 
-	a = b = version(1, "2", "3");
+	a = b = version(1, "2", "3", true);
 	test_pass(versioncompare(&a, &b) == 0);
 
 	/* Test for epoch difference. */
-	a = version(0, "0", "0");
-	b = version(1, "0", "0");
+	a = version(0, "0", "0", false);
+	b = version(1, "0", "0", true);
 	test_pass(versioncompare(&a, &b) < 0);
 	test_pass(versioncompare(&b, &a) > 0);
 
@@ -67,68 +67,72 @@ test_version_parse(void)
 
 	/* Test 0 versions. */
 	blankversion(&a);
-	b = version(0, "0", "");
+	b = version(0, "0", "", false);
 
 	test_pass(parseversion(&a, "0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
+	test_pass(a.epoch_present == false);
 
 	test_pass(parseversion(&a, "0:0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
+	test_pass(a.epoch_present == true);
+	test_pass(a.revision == NULL);
 
 	test_pass(parseversion(&a, "0:0-") == NULL);
+	test_pass(a.revision && *a.revision == '\0');
 	test_pass(versioncompare(&a, &b) == 0);
 
-	b = version(0, "0", "0");
+	b = version(0, "0", "0", false);
 	test_pass(parseversion(&a, "0:0-0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
-	b = version(0, "0.0", "0.0");
+	b = version(0, "0.0", "0.0", false);
 	test_pass(parseversion(&a, "0:0.0-0.0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
 	/* Test epoched versions. */
-	b = version(1, "0", "");
+	b = version(1, "0", "", true);
 	test_pass(parseversion(&a, "1:0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
-	b = version(5, "1", "");
+	b = version(5, "1", "", true);
 	test_pass(parseversion(&a, "5:1") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
 	/* Test multiple dashes. */
-	b = version(0, "0-0", "0");
+	b = version(0, "0-0", "0", false);
 	test_pass(parseversion(&a, "0:0-0-0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
-	b = version(0, "0-0-0", "0");
+	b = version(0, "0-0-0", "0", false);
 	test_pass(parseversion(&a, "0:0-0-0-0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
 	/* Test multiple colons. */
-	b = version(0, "0:0", "0");
+	b = version(0, "0:0", "0", false);
 	test_pass(parseversion(&a, "0:0:0-0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
-	b = version(0, "0:0:0", "0");
+	b = version(0, "0:0:0", "0", false);
 	test_pass(parseversion(&a, "0:0:0:0-0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
 	/* Test multiple dashes and colons. */
-	b = version(0, "0:0-0", "0");
+	b = version(0, "0:0-0", "0", false);
 	test_pass(parseversion(&a, "0:0:0-0-0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
-	b = version(0, "0-0:0", "0");
+	b = version(0, "0-0:0", "0", false);
 	test_pass(parseversion(&a, "0:0-0:0-0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
 	/* Test valid characters in upstream version. */
-	b = version(0, "azAZ09.-+~:", "0");
+	b = version(0, "azAZ09.-+~:", "0", false);
 	test_pass(parseversion(&a, "0:azAZ09.-+~:-0") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
 	/* Test valid characters in revision. */
-	b = version(0, "0", "azAZ09.+~");
+	b = version(0, "0", "azAZ09.+~", false);
 	test_pass(parseversion(&a, "0:0-azAZ09.+~") == NULL);
 	test_pass(versioncompare(&a, &b) == 0);
 
@@ -149,9 +153,81 @@ test_version_parse(void)
 }
 
 static void
+test_version_output(void)
+{
+	struct versionrevision a;
+	struct varbuf output = VARBUF_INIT;
+	const char * string;
+
+	/* Test how epoch is handled on output */
+	string = "0:2-3";
+	parseversion(&a, string);
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_parsed);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp(string, output.buf) == 0);
+
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_always);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp(string, output.buf) == 0);
+
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_never);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp("2-3", output.buf) == 0);
+
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_nonambig);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp("2-3", output.buf) == 0);
+
+	string = "1:2-3";
+	parseversion(&a, string);
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_nonambig);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp("1:2-3", output.buf) == 0);
+
+	a = version(0, "1:a", "2", false); /* Can't happen with parseversion */
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_parsed);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp("0:1:a-2", output.buf) == 0);
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_nonambig);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp("0:1:a-2", output.buf) == 0);
+
+	a = version(0, "1", "2:3", false); /* Can't happen with parseversion */
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_parsed);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp("0:1-2:3", output.buf) == 0);
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_nonambig);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp("0:1-2:3", output.buf) == 0);
+
+	/* Test how revision is output */
+	a = version(0, "1", "", false);
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_parsed);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp("1-", output.buf) == 0);
+
+	a = version(0, "1", NULL, false);
+	varbufreset(&output);
+	varbufversion(&output, &a, vdew_parsed);
+	varbufaddc(&output, '\0');
+	test_pass(strcmp("1", output.buf) == 0);
+}
+
+static void
 test(void)
 {
 	test_version_compare();
 	test_version_parse();
+	test_version_output();
 }
 
diff --git a/src/enquiry.c b/src/enquiry.c
index 6441082..a43c562 100644
--- a/src/enquiry.c
+++ b/src/enquiry.c
@@ -289,22 +289,22 @@ assert_version_support(const char *const *argv,
 }
 
 void assertpredep(const char *const *argv) {
-  struct versionrevision version = { 0, "1.1.0", NULL };
+  struct versionrevision version = { 0, "1.1.0", NULL, false };
   assert_version_support(argv, &version, _("Pre-Depends field"));
 }
 
 void assertepoch(const char *const *argv) {
-  struct versionrevision version = { 0, "1.4.0.7", NULL };
+  struct versionrevision version = { 0, "1.4.0.7", NULL, false };
   assert_version_support(argv, &version, _("epoch"));
 }
 
 void assertlongfilenames(const char *const *argv) {
-  struct versionrevision version = { 0, "1.4.1.17", NULL };
+  struct versionrevision version = { 0, "1.4.1.17", NULL, false };
   assert_version_support(argv, &version, _("long filenames"));
 }
 
 void assertmulticonrep(const char *const *argv) {
-  struct versionrevision version = { 0, "1.4.1.19", NULL };
+  struct versionrevision version = { 0, "1.4.1.19", NULL, false };
   assert_version_support(argv, &version, _("multiple Conflicts and Replaces"));
 }
 
-- 
1.7.0.5

>From a8851813ae5e165ad11429630712d9764a80b843 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rapha=C3=ABl=20Hertzog?= <hertzog@debian.org>
Date: Sun, 2 May 2010 10:07:43 +0200
Subject: [PATCH 2/2] libdpkg: keep same version string in dependency fields in the status db

---
 lib/dpkg/dump.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/dpkg/dump.c b/lib/dpkg/dump.c
index 6c63354..18a21cf 100644
--- a/lib/dpkg/dump.c
+++ b/lib/dpkg/dump.c
@@ -230,7 +230,7 @@ void varbufdependency(struct varbuf *vb, struct dependency *dep) {
         internerr("unknown verrel '%d'", dop->verrel);
       }
       varbufaddc(vb,' ');
-      varbufversion(vb,&dop->version,vdew_nonambig);
+      varbufversion(vb, &dop->version, vdew_parsed);
       varbufaddc(vb,')');
     }
   }
-- 
1.7.0.5