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@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html