Re: [PATCH 9/9] git-svn: Make fetch ~1.7x faster

[Adam Roben <aroben@apple.com>]

Eric Wong wrote:
> Adam Roben <aroben@apple.com> wrote:
>   
>> +package Git::Commands;
>>     
>
> Can this be a separate file, or a part of Git.pm?  I'm sure other
> scripts can eventually use this and I've been meaning to split
> git-svn.perl into separate files so it's easier to follow.
>   

I had considered doing one of the above, but decided that splitting it 
out could be done if/when it was deemed useful for another script. But 
I'll split it out since you think it's a good idea.

>> +use vars qw/$_cat_blob_pid $_cat_blob_in $_cat_blob_out $_cat_blob_ctx $_cat_blob_separator
>> +	    $_hash_object_pid $_hash_object_in $_hash_object_out $_hash_object_ctx/;
>>     
>
> I have trouble following long lines, and most of the git code also wraps
> at 80-columns.  Dead-tree publishers got this concept right a long
> time ago :)
>   

Will fix.

>> +use strict;
>> +use warnings;
>> +use File::Temp qw/tempfile/;
>> +use Git qw/command_bidi_pipe command_close_bidi_pipe/;
>> +
>> +sub _open_cat_blob_if_needed {
>> +	return if defined($_cat_blob_pid);
>> +	$_cat_blob_separator = "--------------GITCATFILESEPARATOR-----------";
>>     
>
> Brian brought this up already, but yes, having pre-defined separators
> instead of explicitly-specified sizes makes it all too easy for a
> malicious user to commit code that will break things for git-svn users.
>   

Yup, will fix this. :-)

>> +sub hash_object {
>> +	my (undef, $fh) = @_;
>> +
>> +	my ($tmp_fh, $tmp_filename) = tempfile(UNLINK => 1);
>> +	while (my $line = <$fh>) {
>> +		print $tmp_fh $line;
>> +	}
>> +	close($tmp_fh);
>>     
>
> Related to the above.  It's better to sysread()/syswrite() or
> read()/print() in a loop with a predefined buffer size rather than to
> use a readline() since you could be dealing with files with very long
> lines or binaries with no newline characters in them at all.
>   

Hm, OK. I'll look for similar code in git-svn and follow that.

>> +	_open_hash_object_if_needed();
>> +	print $_hash_object_out $tmp_filename . "\n";
>>     
>
> Minor, but
>
> 	print $_hash_object_out $tmp_filename, "\n";
>
> avoids creating a new string.
>   

Good idea.

Thanks for the feedback! I'll send out some new patches sometime soon.

-Adam