From: Jason Gross <jgross@mit.edu>
Date: Tue, 23 Aug 2011 05:51:50 GMT
Subject: Re: [PATCH] git-svn: Destroy the cache when we fail to read it
Message-ID: <CAKObCaoj+jwkCW3DOA=N-EtOmn-Ckwevi3fHPqans+_cd8ZC8A@mail.gmail.com>
In-Reply-To: <20110823022717.GA4623@elie.gateway.2wire.net>

1) Sure.  I'm not sure whether I should make $cache_path toplevel, or
a parameter of do_memoization.  I'm thinking the latter, just in case
$ENV{GIT_DIR} changes between calls to
memoize_svn_mergeinfo_functions?

2) I'm not sure if there's a clean/robust way to detect cache
corruption; if it's completely corrupted, Storable complains about not
finding the right magic number, but I could imagine things being
corrupted in more subtle ways that give rise to other Storable errors.
As for your worry: I don't know the code well enough to have git-svn
recover from some other error in memoization; if there's an error,
that means that it can't read the cache correctly (either in tieing
the hashes, or in memoizing the functions).  The other alternative
that I see is to die.  Is this preferable to repeatedly discarding the
cache?

3) Will fix.

4) Will do.

I'll send a new patch in once I hear back from you about 1 and 2.

-Jason

On Mon, Aug 22, 2011 at 10:27 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> (+cc: Eric Wong)
> Hi Jason,
>
> Jason Gross wrote:
>
> [This patch teaches "git svn" to invalidate caches when they
>  fail to load, for example because the endianness or size of
>  some type changed, which is common in the perl 5.6 -> 5.8
>  upgrade.]
>
> > http://lists.debian.org/debian-perl/2011/05/msg00023.html and
> > http://lists.debian.org/debian-perl/2011/05/msg00026.html).
> [...]
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -1680,7 +1680,7 @@ use vars qw/$default_repo_id $default_ref_id $_no_metadata $_follow_parent
> >              $_use_svnsync_props $no_reuse_existing $_minimize_url
> >           $_use_log_author $_add_author_from $_localtime/;
> >  use Carp qw/croak/;
> > -use File::Path qw/mkpath/;
> > +use File::Path qw/mkpath rmtree/;
> >  use File::Copy qw/copy/;
> >  use IPC::Open3;
> >  use Memoize;  # core since 5.8.0, Jul 2002
> > @@ -3198,28 +3198,41 @@ sub has_no_changes {
> >               $memoized = 1;
> >
> >               my $cache_path = "$ENV{GIT_DIR}/svn/.caches/";
> > -             mkpath([$cache_path]) unless -d $cache_path;
> > -
> > -             tie my %lookup_svn_merge_cache => 'Memoize::Storable',
> [...]
> > -             ;
> > +             my $do_memoization = sub {
> > +                     mkpath([$cache_path]) unless -d $cache_path;
> [...]
> > +                     ;
> > +             };
> > +
> > +             if (not eval {
> > +                     $do_memoization->();
> > +                     1;
> > +             }) {
> > +                     my $err = $@ || "Zombie error"; # "Zombie error" to catch clobbered $@ in buggy destructors
> > +                     die $err unless -d $cache_path;
> > +                     print STDERR "Discarding cache and trying again ($@)\n";
> > +                     rmtree([$cache_path]);
> > +                     $do_memoization->();
> > +             }
>
> Thank you thank you thank you.
>
> Okay, time for nitpicks:
>
>  1) Would it be possible to lift this do_memoization() to a toplevel sub?
>    I suspect that could make the code a little easier to read.
>
>  2) Is it important to discard the cache for _all_ errors, instead
>    of just corruption and "is not compatible" errors?  Rebuilding the
>    cache is not cheap, and I am afraid of effects like repeatedly
>    discarding the cache only to rebuild it again due to a typo in
>    git-svn.perl or an out-of-memory condition.
>
>  3) The line with "Zombie error" is very long --- I guess putting the
>    comment on the line before would help.
>
>  4) The series would be clearer imho as a single patch that includes
>    both the fix and tests.
>
> Eric, what do you think?
>
> Thanks again, :)
> Jonathan