Re: [PATCH] git-svn: Destroy the cache when we fail to read it

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 23, 2011, 02:27 UTC
Message-ID: <20110823022717.GA4623@elie.gateway.2wire.net>
In-Reply-To: <1313979422-21286-1-git-send-email-jgross@mit.edu>

(+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).

[...]

Show 18 quoted lines

> --- 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;

[...]

Show 13 quoted lines

> +			;
> +		};
> +
> +		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