Merge lp:~maxb/ubuntu/lucid/devscripts/445432 into lp:ubuntu/lucid/devscripts

[Max Bowsher]

Please see bug 445432

[James Westby (james-w)]

Hi Max,

Thanks for the fix.

I like the idea to make the ubuntu code more targetted, but I'm a little
concerned that doing it in this fashion may break someone's use of it.

Can you convince me that doing this is better than just checking for -l in
the long conditional that is already there?

Thanks,

James

[Max Bowsher (maxb)]

OK... here goes then: I begin by making a list of all of the "mode selection" options of dch:
 -a Append
 -e Edit
 -r Release
 -i Increment
 -v Explicit version
 -d From dir name
 -n NMU
 --bin-nmu BinNMU
 -R Rebuild (-lbuild)
 -q QA
 -s Security
 --bpo BPO
 -l Local (suffix)

then I discounted those ones for which dch actually overrides the distributor back to Debian if they are used:
 -n NMU
 --bin-nmu BinNMU
 -q QA
 --bpo BPO

and discounted the ones which don't add a new version stanza to the changelog:
 -a Append
 -e Edit
 -r Release

and discounted the ones which explicitly specify an exact version:
 -v Explicit version
 -d From dir name

and discounted the one already explicitly excluded from the "ubuntu1" block:
 -R Rebuild (-lbuild)

which leaves:
 -i Increment
 -s Security
 -l Local (suffix)

of which -i and -s can and should exhibit "ubuntu1" behaviour and -l never should.

... at which point I prove to myself that you are right and my changes as-is do in fact break the ubuntu-desired behaviour for -s. :-/

So I'll be fixing that, then.

Once I have fixed that, the logical justification proceeds as follows: Since the "ubuntu1" behaviour is desired only in the case of -i and -s, provided every preceding conditional in the long conditional EITHER refers to a mutually exclusive mode option OR explicitly tests for $distributor ne 'Ubuntu', it will be OK to move the "ubuntu1" logic into the !$opt_news block.

[lp:~maxb/ubuntu/lucid/devscripts/445432]

85. By Max Bowsher on 2010-03-30

Fix mistaken removal of Ubuntu-specific versioning from the 'dch -s' case.

86. By Max Bowsher on 2010-03-30

Remove logically-redundant check.

87. By Max Bowsher on 2010-03-30

Re-add avoidance of automatic NMU version removal in the distributor=Ubuntu case.

[Max Bowsher (maxb)]

OK, changes pushed - including a further fix to skip the "Drop NMU revision" block in the distributor=Ubuntu case.

I hope/believe I've now caught all the edge-cases, but I certainly agree the fact that I had to patch two up after initial review isn't entirely comforting.

For comparison, the one-liner most direct patch for just the immediate bug would be:

- if ($distributor eq 'Ubuntu' and $start !~ /(build|ubuntu|prop(osed)?)(\d+\.)*$/ and not $opt_U and not $opt_R) {
+ if ($distributor eq 'Ubuntu' and $start !~ /(build|ubuntu|prop(osed)?)(\d+\.)*$/ and not $opt_U and ($opt_i or $opt_s)) {

What I've tried to do instead with this branch is to properly localize the Ubuntu-specific changes to the specific increment operation they seek to override, rather than have them stuck on the front of a long cascading if-elsif where they can potentially override more than they are supposed to. An additional (though admittedly very minor) bug that this branch fixes that the one-liner does not is the processing of 'dch --news -i', which is supposed to add a new NEWS entry matching the current changelog version - the current code can increment the version added to NEWS beyond the current top changelog entry.

[James Westby (james-w)]

Thanks Max, you logic looks sounds to me.

James

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2010-01-19 02:16:10 +0000
+++ debian/changelog	2010-03-30 22:59:25 +0000
@@ -1,3 +1,11 @@
+devscripts (2.10.61ubuntu3) lucid; urgency=low
+
+  * Fix "dch -l fails due to ubuntu patching" (LP: #445432), by narrowing the
+    applicability of the 'Append "ubuntu1"' code to just the 'dch -i' case
+    (including when -i is heuristically selected when no mode is given).
+
+ -- Max Bowsher <[e-mail]>  Fri, 26 Mar 2010 07:14:10 +0000
+
 devscripts (2.10.61ubuntu2) lucid; urgency=low
 
   * Restore changes to scripts/debchange.pl erroneously dropped in the

=== modified file 'scripts/debchange.pl'
--- scripts/debchange.pl	2010-01-19 02:16:10 +0000
+++ scripts/debchange.pl	2010-03-30 22:59:25 +0000
@@ -1024,13 +1024,7 @@
 	    # If it's not already an NMU make it so
 	    # otherwise we can be safe if we behave like dch -i
 
-	    if ($distributor eq 'Ubuntu' and $start !~ /(build|ubuntu|prop(osed)?)(\d+\.)*$/ and not $opt_U and not $opt_R) {
-		if ($start =~ /~ppa([0-9]*)$/) {
-		    $end++;
-		} else {
-		    $end .= "ubuntu1";
-		}
-	    } elsif (($opt_n or $opt_s) and (
+	    if (($opt_n or $opt_s) and $distributor ne 'Ubuntu' and (
 		($VERSION eq $UVERSION and not $start =~ /\+nmu/)
 		or ($VERSION ne $UVERSION and not $start =~ /\.$/))) {
 
@@ -1042,7 +1036,7 @@
 		}
 	    } elsif ($opt_bn and not $start =~ /\+b/) {
 		$end .= "+b1";
-	} elsif ($opt_R and not $start =~ /build/ and not $start =~ /ubuntu/) {
+	    } elsif ($opt_R and not $start =~ /build/ and not $start =~ /ubuntu/) {
 		$end .= "build1";
 	    } elsif ($opt_qa and $start =~/(.*?)-(\d+)\.$/) {
 		# Drop NMU revision when doing a QA upload
@@ -1062,7 +1056,14 @@
 	    } elsif (!$opt_news) {
 		# Don't bump the version of a NEWS file in this case as we're
 		# using the version from the changelog
-		$end++;
+
+		if (($opt_i or $opt_s) and $distributor eq 'Ubuntu' and
+		    $start !~ /(build|ubuntu|prop(osed)?|~ppa)(\d+\.)*$/ and
+		    not $opt_U) {
+		    $end .= "ubuntu1";
+		} else {
+		    $end++;
+		}
 
 		# Attempt to set the distribution for a backport correctly
 		# based on the version of the previous backport
@@ -1078,7 +1079,7 @@
 		    }
 		}
 
-		if(! ($opt_s or $opt_n)) {
+		if(! ($opt_s or $opt_n or $distributor eq 'Ubuntu')) {
 		    if ($start =~/(.*?)-(\d+)\.$/) {
 			# Drop NMU revision
 			my $upstream_version = $1;