Bug #77790 for Memoize: Memoize::Storable: 'nstore' option not respected

Jonathan Nieder wrote: Show quoted text
Self-contained testcase below. When suggesting the fix, I was briefly worried that it could hurt compatibility: might not some older program be broken by the silent switch to nstore format, even when that is obviously what the coder intended? Luckily "Storable::retrieve" doesn't care which format is used, and my worries were unfounded. I've been testing with Memoize 1.02 bundled with perl 5.14.2, as distributed in Debian's perl 5.14.2-11 package. The patch I sent in the last message is against perlblead. Thanks for reading. Jonathan -- >8 -- #!/usr/bin/perl use strict; use warnings; use Memoize qw(memoize unmemoize); use Storable; use Memoize::Storable; sub function { return 1; } tie my %cache => 'Memoize::Storable', 'test.storable', 'nstore'; memoize 'function', SCALAR_CACHE => [HASH => \%cache]; unmemoize 'function'; untie %cache; print(Storable::file_magic('test.storable')->{'netorder'}, "\n");

Memoize(3perl) says: tie my %cache => 'Memoize::Storable', $filename, 'nstore'; memoize 'function', SCALAR_CACHE => [HASH => \%cache]; Include the ‘nstore’ option to have the "Storable" database written in ‘network order’. (See Storable for more details about this.) In fact the "nstore" option does no such thing. Option parsing looks like this: @options{@_} = (); $self->{OPTIONS}{'nstore'} is accordingly set to undef. Later Memoize::Storable checks if the option is true, and since undef is not true, the "else" branch is always taken. if ($self->{OPTIONS}{'nstore'}) { Storable::nstore($self->{H}, $self->{FILENAME}); } else { Storable::store($self->{H}, $self->{FILENAME}); } Correcting the condition to (exists $self->{OPTIONS}{'nstore'}) fixes it. Noticed because git-svn, which uses the 'nstore' option for its on-disk caches, was producing Byte order is not compatible at ../../lib/Storable.pm when run using a perl with a different integer size (and hence byteorder). Reported by Tim Retout (RT#77790) --- (just resending to CPAN RT so this gets archived) Hi, git-svn has been using the Memoize::Storable 'nstore' option for a couple of years now, but it never actually worked. :) See [1] and [2] for context. Patch is against Memoize 1.03 from CPAN. Hints of all kinds welcome. If this is the wrong address to send patches to, please feel free to pass it along. Thoughts? Jonathan [1] http://bugs.debian.org/587650 [2] https://rt.cpan.org/Public/Bug/Display.html?id=77790 Memoize/Storable.pm | 2 +- t/tie_storable.t | 24 ++++++++++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/Memoize/Storable.pm b/Memoize/Storable.pm index 13147972..87876f22 100644 --- a/Memoize/Storable.pm +++ b/Memoize/Storable.pm @@ -55,7 +55,7 @@ sub DESTROY { require Carp if $Verbose; my $self= shift; print STDERR "Memoize::Storable::DESTROY(@_)\n" if $Verbose; - if ($self->{OPTIONS}{'nstore'}) { + if (exists $self->{OPTIONS}{'nstore'}) { Storable::nstore($self->{H}, $self->{FILENAME}); } else { Storable::store($self->{H}, $self->{FILENAME}); diff --git a/t/tie_storable.t b/t/tie_storable.t index de3b8dc2..a6242385 100644 --- a/t/tie_storable.t +++ b/t/tie_storable.t @@ -31,18 +31,34 @@ if ($@) { exit 0; } -print "1..4\n"; +print "1..9\n"; $file = "storable$$"; 1 while unlink $file; tryout('Memoize::Storable', $file, 1); # Test 1..4 1 while unlink $file; +tryout('Memoize::Storable', $file, 5, 'nstore'); # Test 5..8 +assert_netorder($file, 9); # Test 9 +1 while unlink $file; + + +sub assert_netorder { + my ($file, $testno) = @_; + + my $netorder = Storable::file_magic($file)->{'netorder'}; + print ($netorder ? "ok $testno\n" : "not ok $testno\n"); +} sub tryout { - my ($tiepack, $file, $testno) = @_; + my ($tiepack, $file, $testno, $option) = @_; - tie my %cache => $tiepack, $file - or die $!; + if (defined $option) { + tie my %cache => $tiepack, $file, $option + or die $!; + } else { + tie my %cache => $tiepack, $file + or die $!; + } memoize 'c5', SCALAR_CACHE => [HASH => \%cache], -- 1.7.10.4

Just for the record, this patch has been applied in Debian for more than three years now, so it's probably a good candidate for applying upstream, which would help other users. Cheers, Dominic.