Re: [PATCH] git svn: reset invalidates the memoized mergeinfo caches

[Peter Baumann <waste.manager@gmx.de>]

On Tue, Aug 07, 2012 at 08:45:10PM +0000, Eric Wong wrote:
> Peter Baumann <waste.manager@gmx.de> wrote:
> > Therefore the easiest solution to clear the cache is to delete the files
> > on disk in 'git svn reset'. Normally, deleting the files behind the back
> > of the memoization module would be problematic, because the in-memory
> > representation would still exist and contain wrong data. Fortunately, the
> > memoization is active in memory only for a small portion of the code.
> > Invalidating the cache by deleting the files on disk if it isn't active
> > should be safe.
> 
> Thanks for the patch and explanation.  A few comments below:
> 
> > +	sub clear_memoized_mergeinfo_caches {
> > +		die "Only call this method in non-memoized context" if ($memoized);
> > +
> > +		my $cache_path = "$ENV{GIT_DIR}/svn/.caches/";
> > +		return unless -d $cache_path;
> > +
> > +		for my $cache_file (("$cache_path/lookup_svn_merge",
> > +				     "$cache_path/check_cherry_pick",
> > +				     "$cache_path/has_no_changes")) {
> > +			for my $suffix (qw(yaml db)) {
> > +				unlink("$cache_file.$suffix");
> 
> Need to check for unlink() errors (and ignore ENOENT).


I'm not sure what you mean here: Aren't we screwed either way if unlinking
the file failed? There is nothhing we can do about it if e.g. the user doesn't
have the permissions to delete the file, besides terminating, e.g.

	for my $cache_file (("$cache_path/lookup_svn_merge",
			     "$cache_path/check_cherry_pick",
			     "$cache_path/has_no_changes")) {
		for my $suffix (qw(yaml db)) {
			next unless (-e "$cache_file.$suffix");
			unlink("$cache_file.$suffix") or 
				die "Failed to delete $cache_file.$suffix";
		}
	}

> 
> > @@ -2126,8 +2142,13 @@ sub rev_map_set {
> >  
> >  	sysopen(my $fh, $db_lock, O_RDWR | O_CREAT)
> >  	     or croak "Couldn't open $db_lock: $!\n";
> > -	$update_ref eq 'reset' ? _rev_map_reset($fh, $rev, $commit) :
> > -				 _rev_map_set($fh, $rev, $commit);
> > +	if ($update_ref eq 'reset') {
> > +		_rev_map_reset($fh, $rev, $commit);
> > +		clear_memoized_mergeinfo_caches();
> 
> Better to clear_memoized_mergeinfo_caches() before _rev_map_reset()
> in case unlink() (or anything else) fails when clearing the cache.

Will do.

> 
> > +test_expect_success 'initialize source svn repo' '
> > +	svn_cmd mkdir -m "create trunk" "$svnrepo"/trunk &&
> > +	svn_cmd mkdir -m "create branches" "$svnrepo/branches" &&
> > +	svn_cmd co "$svnrepo"/trunk "$SVN_TREE" &&
> > +	(
> > +		cd "$SVN_TREE" &&
> > +		touch foo &&
> > +		svn add foo &&
> 
> svn_cmd here, too.

Will do.
> 
> > +		svn commit -m "a" &&
> > +		svn cp -m branch "$svnrepo"/trunk "$svnrepo"/branches/branch1 &&
> > +		svn switch "$svnrepo"/branches/branch1 &&
> > +		touch bar &&
> > +		svn add bar &&
> > +		svn commit -m b &&
> > +		svn switch "$svnrepo"/trunk &&
> > +		touch baz &&
> > +		svn add baz &&
> > +		svn commit -m c &&
> > +		svn up &&
> > +		svn merge "$svnrepo"/branches/branch1 &&
> > +		svn commit -m "m"
> > +	) &&
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>