Revert "Dpkg::Conf: Switch implementation to be hash based"

This reverts commit 94e241761c06ab112ec3e899dd9449784928c6c5.

This change broke backwards compatibility in multiple ways. The
format_argv option was set by default, the order was not preserved,
which was important for dpkg.cfg files, and duplicate option names
stopped being supported.

Add regression tests to avoid similar changes in the future.

Closes: #824938

debian/changelog

@@ -13,6 +13,12 @@ dpkg (1.18.8) UNRELEASED; urgency=medium
       dpkg-parsechangelog's output.
     - Validate source version in set_version_substvars()'s Dpkg::Substvars
       method.
+    - Revert "Dpkg::Conf: Switch implementation to be hash based", as this
+      change broke backwards compatibility in multiple ways. The format_argv
+      option was set by default, the order was not preserved, which was
+      important for dpkg.cfg files, and duplicate option names stopped being
+      supported. Add regression tests to avoid similar changes in the future.
+      Closes: #824938
   * Test suite:
     - Bump perlcritic ValuesAndExpressions::RequireNumberSeparators minimum
       to 99999.

scripts/Dpkg/Conf.pm

@@ -18,7 +18,9 @@ package Dpkg::Conf;
 use strict;
 use warnings;
 
-our $VERSION = '1.02';
+our $VERSION = '1.03';
+
+use Carp;
 
 use Dpkg::Gettext;
 use Dpkg::ErrorHandling;
@@ -58,7 +60,7 @@ sub new {
     my $class = ref($this) || $this;
 
     my $self = {
-	options => {},
+	options => [],
 	allow_short => 0,
     };
     foreach my $opt (keys %opts) {
@@ -79,46 +81,19 @@ Returns the list of options that can be parsed like @ARGV.
 
 sub get_options {
     my $self = shift;
-    my @options;
-
-    foreach my $name (sort keys %{$self->{options}}) {
-        my $value = $self->{options}->{$name};
 
-        $name = "--$name" unless $name =~ /^-/;
-        if (defined $value) {
-            push @options, "$name=$value";
-        } else {
-            push @options, $name;
-        }
-    }
-
-    return @options;
+    return @{$self->{options}};
 }
 
-=item $value = $conf->get($name)
-
-Returns the value of option $name, where the option name should not have "--"
-prefixed. If the option is not present the function will return undef.
-
-=cut
+# These functions existed for a brief time, but do not mesh well with
+# repeated options.
 
 sub get {
-    my ($self, $name) = @_;
-
-    return $self->{options}->{$name};
+    croak 'obsolete function, use get_options instead';
 }
 
-=item $conf->set($name, $value)
-
-Set the $value of option $name, where the option name should not have "--"
-prefixed.
-
-=cut
-
 sub set {
-    my ($self, $name, $value) = @_;
-
-    $self->{options}->{$name} = $value;
+    croak 'obsolete function, use get_options instead';
 }
 
 =item $conf->load($file)
@@ -150,11 +125,13 @@ sub parse {
 	}
 	if (/^([^=]+)(?:=(.*))?$/) {
 	    my ($name, $value) = ($1, $2);
+	    $name = "--$name" unless $name =~ /^-/;
 	    if (defined $value) {
 		$value =~ s/^"(.*)"$/$1/ or $value =~ s/^'(.*)'$/$1/;
+		push @{$self->{options}}, "$name=$value";
+	    } else {
+		push @{$self->{options}}, $name;
 	    }
-	    $name =~ s/^--//;
-	    $self->{options}->{$name} = $value;
 	    $count++;
 	} else {
 	    warning(g_('invalid syntax for option in %s, line %d'), $desc, $.);
@@ -167,8 +144,6 @@ sub parse {
 
 Filter the list of options, either removing or keeping all those that
 return true when &$opts{remove}($option) or &opts{keep}($option) is called.
-If $opts{format_argv} is true the long options passed to the filter
-functions will have "--" prefixed.
 
 =cut
 
@@ -177,16 +152,10 @@ sub filter {
     my $remove = $opts{remove} // sub { 0 };
     my $keep = $opts{keep} // sub { 1 };
 
-    foreach my $name (keys %{$self->{options}}) {
-        my $option = $name;
+    croak 'obsolete option format_argv' if exists $opts{format_argv};
 
-        if ($opts{format_argv}) {
-            $option = "--$name" unless $name =~ /^-/;
-        }
-        if (&$remove($option) or not &$keep($option)) {
-            delete $self->{options}->{$name};
-        }
-    }
+    @{$self->{options}} = grep { not &$remove($_) and &$keep($_) }
+                               @{$self->{options}};
 }
 
 =item $string = $conf->output($fh)
@@ -207,12 +176,9 @@ Save the options in a file.
 sub output {
     my ($self, $fh) = @_;
     my $ret = '';
-
-    foreach my $name (sort keys %{$self->{options}}) {
-	my $value = $self->{options}->{$name};
-
-	my $opt = $name;
-	$opt .= " = \"$value\"" if defined $value;
+    foreach my $opt ($self->get_options()) {
+	$opt =~ s/^--//;
+	$opt =~ s/^([^=]+)=(.*)$/$1 = "$2"/;
 	$opt .= "\n";
 	print { $fh } $opt if defined $fh;
 	$ret .= $opt;
@@ -224,6 +190,12 @@ sub output {
 
 =head1 CHANGES
 
+=head2 Version 1.03 (dpkg 1.18.8)
+
+Obsolete option: 'format_argv' in $conf->filter().
+
+Obsolete methods: $conf->get(), $conf->set().
+
 =head2 Version 1.02 (dpkg 1.18.5)
 
 New option: Accept new option 'format_argv' in $conf->filter().

scripts/t/Dpkg_Conf.t

@@ -16,7 +16,7 @@
 use strict;
 use warnings;
 
-use Test::More tests => 14;
+use Test::More tests => 9;
 
 BEGIN {
     use_ok('Dpkg::Conf');
@@ -28,17 +28,21 @@ my $datadir = $srcdir . '/t/Dpkg_Conf';
 my ($conf, $count, @opts);
 
 my @expected_long_opts = (
-'--l=v',
-'--option-dash=value-dash',
 '--option-double-quotes=value double quotes',
-'--option-equal=value-equal=subvalue-equal',
-'--option-indent=value-indent',
-'--option-name=value-name',
-'--option-noequal=value-noequal',
-'--option-simple',
 '--option-single-quotes=value single quotes',
 '--option-space=value words space',
-);
+qw(
+--option-dupe=value1
+--option-name=value-name
+--option-indent=value-indent
+--option-equal=value-equal=subvalue-equal
+--option-noequal=value-noequal
+--option-dupe=value2
+--option-simple
+--option-dash=value-dash
+--option-dupe=value3
+--l=v
+));
 my @expected_short_opts = qw(
 -o=vd
 -s
@@ -48,55 +52,50 @@ $conf = Dpkg::Conf->new();
 local $SIG{__WARN__} = sub { };
 $count = $conf->load("$datadir/config-mixed");
 delete $SIG{__WARN__};
-is($count, 10, 'Load a config file, only long options');
+is($count, 13, 'Load a config file, only long options');
 
 @opts = $conf->get_options();
 is_deeply(\@opts, \@expected_long_opts, 'Parse long options');
 
 $conf = Dpkg::Conf->new(allow_short => 1);
 $count = $conf->load("$datadir/config-mixed");
-is($count, 12, 'Load a config file, mixed options');
+is($count, 15, 'Load a config file, mixed options');
 
 @opts = $conf->get_options();
-my @expected_mixed_opts = ( @expected_short_opts, @expected_long_opts );
+my @expected_mixed_opts = ( @expected_long_opts, @expected_short_opts );
 is_deeply(\@opts, \@expected_mixed_opts, 'Parse mixed options');
 
 my $expected_mixed_output = <<'MIXED';
--o = "vd"
--s
-l = "v"
-option-dash = "value-dash"
 option-double-quotes = "value double quotes"
-option-equal = "value-equal=subvalue-equal"
-option-indent = "value-indent"
+option-single-quotes = "value single quotes"
+option-space = "value words space"
+option-dupe = "value1"
 option-name = "value-name"
+option-indent = "value-indent"
+option-equal = "value-equal=subvalue-equal"
 option-noequal = "value-noequal"
+option-dupe = "value2"
 option-simple
-option-single-quotes = "value single quotes"
-option-space = "value words space"
+option-dash = "value-dash"
+option-dupe = "value3"
+l = "v"
+-o = "vd"
+-s
 MIXED
 
 is($conf->output, $expected_mixed_output, 'Output mixed options');
 
-is($conf->get('-o'), 'vd', 'Get option -o');
-is($conf->get('option-dash'), 'value-dash', 'Get option-dash');
-is($conf->get('option-space'), 'value words space', 'Get option-space');
-
-is($conf->get('manual-option'), undef, 'Get non-existent option');
-$conf->set('manual-option', 'manual value');
-is($conf->get('manual-option'), 'manual value', 'Get manual option');
-
 my $expected_filter;
 
 $expected_filter = <<'FILTER';
+l = "v"
 -o = "vd"
 -s
-l = "v"
 FILTER
 
 $conf = Dpkg::Conf->new(allow_short => 1);
 $conf->load("$datadir/config-mixed");
-$conf->filter(format_argv => 1, remove => sub { $_[0] =~ m/^--option/ });
+$conf->filter(remove => sub { $_[0] =~ m/^--option/ });
 is($conf->output, $expected_filter, 'Filter remove');
 
 $expected_filter = <<'FILTER';
@@ -106,7 +105,7 @@ FILTER
 
 $conf = Dpkg::Conf->new(allow_short => 1);
 $conf->load("$datadir/config-mixed");
-$conf->filter(keep => sub { $_[0] =~ m/^option-[a-z]+-quotes/ });
+$conf->filter(keep => sub { $_[0] =~ m/^--option-[a-z]+-quotes/ });
 is($conf->output, $expected_filter, 'Filter keep');
 
 $expected_filter = <<'FILTER';
@@ -115,8 +114,7 @@ FILTER
 
 $conf = Dpkg::Conf->new(allow_short => 1);
 $conf->load("$datadir/config-mixed");
-$conf->filter(format_argv => 1,
-              remove => sub { $_[0] =~ m/^--option/ },
+$conf->filter(remove => sub { $_[0] =~ m/^--option/ },
               keep => sub { $_[0] =~ m/^--/ });
 is($conf->output, $expected_filter, 'Filter keep and remove');
 

scripts/t/Dpkg_Conf/config-mixed

@@ -4,6 +4,7 @@ option-double-quotes = "value double quotes"
 option-single-quotes = 'value single quotes'
 option-space  = 	 value words space	  
 
+option-dupe=value1
 option-name=value-name		  
 	  
 	  # Indented comment.
@@ -11,10 +12,12 @@ option-name=value-name
 
 option-equal=value-equal=subvalue-equal
 option-noequal value-noequal
+option-dupe=value2
 option-simple
 
-# Fully spelled out option.
+# Fully spelled out options.
 --option-dash=value-dash
+--option-dupe=value3
 
 # Long option with one character name.
 l=v