Re: [PATCH 2/3] git-svn: Make it scream by minimizing temp files

[Eric Wong <normalperson@yhbt.net>]

Marcus Griep <marcus@griep.us> wrote:
> ---
>  git-svn.perl |   53 +++++++++++++++++++++++++++++++++++------------------
>  1 files changed, 35 insertions(+), 18 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index fe78461..0937918 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1285,6 +1285,8 @@ use Carp qw/croak/;
>  use File::Path qw/mkpath/;
>  use File::Copy qw/copy/;
>  use IPC::Open3;
> +use File::Temp ();
> +use File::Spec;
>  
>  my ($_gc_nr, $_gc_period);
>  
> @@ -1323,10 +1325,11 @@ BEGIN {
>  	}
>  }
>  
> -my (%LOCKFILES, %INDEX_FILES);
> +my (%LOCKFILES, %INDEX_FILES, %TEMP_FILES);
>  END {
>  	unlink keys %LOCKFILES if %LOCKFILES;
>  	unlink keys %INDEX_FILES if %INDEX_FILES;
> +	unlink values %TEMP_FILES if %TEMP_FILES;
>  }
  
>  sub resolve_local_globs {
> @@ -2935,6 +2938,21 @@ sub remove_username {
>  	$_[0] =~ s{^([^:]*://)[^@]+@}{$1};
>  }
>  
> +sub _temp_file {
> +	my ($self, $fd) = @_;
> +	if (defined $TEMP_FILES{$fd}) {
> +		truncate $TEMP_FILES{$fd}, 0 or croak $!;
> +		sysseek $TEMP_FILES{$fd}, 0, 0 or croak $!;
> +	} else {
> +		$TEMP_FILES{$fd} = File::Temp->new(
> +			TEMPLATE => 'GitSvn_XXXXXX',
> +			DIR => File::Spec->tmpdir
> +			) or croak $!;
> +		$TEMP_FILES{$fd}->autoflush(1);
> +	}
> +	$TEMP_FILES{$fd};
> +}
> +

The above is dead code now that we're using the versions in Git::,
right?

> @@ -3246,9 +3262,10 @@ sub apply_textdelta {
> -	[ SVN::TxDelta::apply($base, $fh, undef, $fb->{path}, $fb->{pool}) ];
> +	my $return = [ SVN::TxDelta::apply($base, $dup, undef, $fb->{path}, $fb->{pool}) ];
> +	$return;

Why create a temporary variable? (and break the sacred 80-column limit).

> @@ -3277,22 +3294,23 @@ sub close_file {
> -				or croak("write $tmp_filename: $!\n");
> +				or croak("write ", $tmp_fh->filename, ": $!\n");

I missed this before, but $tmp_fh->filename doesn't interpolate correctly.

("write ${\$tmp_fh->filename}: $!\n")
or
("write ", $tmp_fh->filename, ": $!\n") works.

I believe the latter form is more idiomatic so we should probably use
that...

-- 
Eric Wong