[BUG] Another bad garbage collection bug

[Martin Atkins]

I think this dots-to-colons mapping should happen in the 
Brackup::Target->chunks method. The renaming of colons to dots is an 
implementation detail of the target, but the public Target interface is 
to use colons so the target should apply the opposite mapping when it 
returns a list of chunks for symmetry/consistency.

I don't have a diff tool handy right now, but something like this:

  sub chunks {
      my $self = shift;


      my @chunks = ();
      my $found_chunk = sub {
          m/\.chunk$/ or return;
          my $chunk_name = basename($_);
          $chunk_name =~ s/\.chunk$//;
+        $chunk_name =~ s/\./:/;
          push @chunks, $chunk_name;
      };
      File::Find::find({ wanted => $found_chunk, no_chdir => 1}, 
$self->{path});
      return @chunks;
  }

(in Brackup::Target::Filesystem, line 312 or so)

Gavin Carr wrote:
> I've found another bad bug with garbage collection: any target
> that has the 'nocolons' setting turned on (and it's on by default 
> for both the Ftp and Sftp targets) have their chunks incorrectly 
> compared during garbage collection, with the result that ALL 
> chunks get deleted when you do a gc.
> 
> The problem is that chunks in the brackup file are recorded with
> colons, while the chunks on disk are escaped, and then the 
> comparison during garbage collection is just done directly, when
> one or other of the operands should be converted.
> 
> The simplest fix is probably to unescape to colons again e.g.
> 
> -----------------------------------------------------------------------
> === modified file 'lib/Brackup/Target.pm'
> --- lib/Brackup/Target.pm       2008-11-17 06:05:36 +0000
> +++ lib/Brackup/Target.pm       2008-11-17 10:30:05 +0000
> @@ -136,7 +136,7 @@
>  
>      # get all chunks and then loop through metafiles to detect
>      # referenced ones
> -    my %chunks = map {$_ => 1} $self->chunks;
> +    my %chunks = map {s/\./:/ if $self->nocolons; $_ => 1} $self->chunks;
>      my $tempfile = +(tempfile())[1];
>      my @backups = $self->backups;
>      BACKUP: foreach my $i (0 .. $#backups) {
> -----------------------------------------------------------------------
> 
> 
> A stylistic wrinkle is that the whole 'nocolons' thing is target-
> specific at the moment, so because garbage collection is 
> implemented in the parent Brackup::Target class we probably need
> to either implement a default nocolons() method there, or leave it
> target-optional and make the test more complex e.g.
> 
>   my %chunks = map {s/\./:/ if $self->can('nocolons') && $self->nocolons; $_ => 1} $self->chunks;
> 
> Thoughts on that? I'm probably inclined towards the former at the 
> moment, but not too strongly. If we do the latter, then we should
> maybe redo the map as a for, as it's getting pretty long?
> 
> Although the real fix is probably to try and get rid of this 
> whole colons thing altogether and convert to the dotted version
> everywhere - if we can figure out how do it in a way that's 
> backward compatible.
> 
> Cheers,
> Gavin