{"thread":{"id":"14900","subject":"[PATCH] git-svn: Make it scream by minimizing temp files","startedAt":"2008-08-08T22:41:53Z","lastAt":"2008-08-15T19:53:59Z","messageCount":43,"participants":["Marcus Griep","Junio C Hamano","Eric Wong","Lea Wiemann","Miklos Vajna","Bryan Donlan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"86554","messageId":"1218235313-19480-1-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":null,"subject":"[PATCH] git-svn: Make it scream by minimizing temp files","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-08T22:41:53Z","receivedAt":"2008-08-08T22:41:53Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Currently, git-svn would create a temp file on four occasions:\n1. Reading a blob out of the object db\n2. Creating a delta from svn\n3. Hashing and writing a blob into the object db\n4. Reading a blob out of the object db (in another place in code)\n\nAny time git-svn did the above, it would dutifully create and then\ndelete said temp file. Unfortunately, this means that between 2-4\ntemporary files are created/deleted per file 'add/modify'-ed in\nsvn (O(n)). This causes significant overhead and helps the inode\ncounter to spin beautifully.\n\nBy its nature, git-svn is a serial beast. Thus, reusing a temp file\ndoes not pose significant problems. \"truncate and seek\" takes much\nless time than \"unlink and create\". This patch centralizes the\ntempfile creation and holds onto the tempfile until they are deleted\non exit. This significantly reduces file overhead, now requiring\nat most three (3) temp files per run (O(1)).\n\nSigned-off-by: Marcus Griep \n---\n git-svn.perl | 48 ++++++++++++++++++++++++++++++------------------\n 1 files changed, 30 insertions(+), 18 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 5099c1f..02ae207 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1257,7 +1257,7 @@ sub md5sum {\n \tmy $arg = shift;\n \tmy $ref = ref $arg;\n \tmy $md5 = Digest::MD5->new();\n- if ($ref eq 'GLOB' || $ref eq 'IO::File') {\n+ if ($ref eq 'GLOB' || $ref eq 'IO::File' || $ref eq 'File::Temp') {\n \t\t$md5->addfile($arg) or croak $!;\n \t} elsif ($ref eq 'SCALAR') {\n \t\t$md5->add($$arg) or croak $!;\n@@ -1282,6 +1282,8 @@ use Carp qw/croak/;\n use File::Path qw/mkpath/;\n use File::Copy qw/copy/;\n use IPC::Open3;\n+use File::Temp qw/ :seekable /;\n+use File::Spec;\n \n my ($_gc_nr, $_gc_period);\n \n@@ -1320,10 +1322,11 @@ BEGIN {\n \t}\n }\n \n-my (%LOCKFILES, %INDEX_FILES);\n+my (%LOCKFILES, %INDEX_FILES, %TEMP_FILES);\n END {\n \tunlink keys %LOCKFILES if %LOCKFILES;\n \tunlink keys %INDEX_FILES if %INDEX_FILES;\n+\tunlink values %TEMP_FILES if %TEMP_FILES;\n }\n \n sub resolve_local_globs {\n@@ -2932,6 +2935,23 @@ sub remove_username {\n \t$_[0] =~ s{^([^:]*://)[^@]+@}{$1};\n }\n \n+sub _temp_file {\n+\tmy ($self, $fd, $autoflush) = @_;\n+\tif (defined $TEMP_FILES{$fd}) {\n+\t\ttruncate $TEMP_FILES{$fd}, 0 or croak $!;\n+\t\tseek $TEMP_FILES{$fd}, 0, 0 or croak $!;\n+\t} else {\n+\t\t$TEMP_FILES{$fd} = File::Temp->new(\n+\t\t\t\t\t\t\t\t\tTEMPLATE => 'GitSvn_XXXXXX',\n+\t\t\t\t\t\t\t\t\tDIR => File::Spec->tmpdir\n+\t\t\t\t\t\t\t\t\t) or croak $!;\n+\t\tif (defined $autoflush) {\n+\t\t\t$TEMP_FILES{$fd}->autoflush($autoflush);\n+\t\t}\n+\t}\n+\t$TEMP_FILES{$fd};\n+}\n+\n package Git::SVN::Prompt;\n use strict;\n use warnings;\n@@ -3222,13 +3242,11 @@ sub change_file_prop {\n \n sub apply_textdelta {\n \tmy ($self, $fb, $exp) = @_;\n-\tmy $fh = IO::File->new_tmpfile;\n-\t$fh->autoflush(1);\n+\tmy $fh = Git::SVN->_temp_file('delta_temp', 1);\n \t# $fh gets auto-closed() by SVN::TxDelta::apply(),\n \t# (but $base does not,) so dup() it for reading in close_file\n \topen my $dup, '<&', $fh or croak $!;\n-\tmy $base = IO::File->new_tmpfile;\n-\t$base->autoflush(1);\n+\tmy $base = Git::SVN->_temp_file('git_blob_temp', 1);\n \tif ($fb->{blob}) {\n \t\tprint $base 'link ' if ($fb->{mode_a} == 120000);\n \t\tmy $size = $::_repository->cat_blob($fb->{blob}, $base);\n@@ -3243,9 +3261,9 @@ sub apply_textdelta {\n \t\t}\n \t}\n \tseek $base, 0, 0 or croak $!;\n-\t$fb->{fh} = $dup;\n+\t$fb->{fh} = $fh;\n \t$fb->{base} = $base;\n-\t[ SVN::TxDelta::apply($base, $fh, undef, $fb->{path}, $fb->{pool}) ];\n+\t[ SVN::TxDelta::apply($base, $dup, undef, $fb->{path}, $fb->{pool}) ];\n }\n \n sub close_file {\n@@ -3274,22 +3292,18 @@ sub close_file {\n \t\t\t}\n \t\t}\n \n-\t\tmy ($tmp_fh, $tmp_filename) = File::Temp::tempfile(UNLINK => 1);\n+\t\tmy $tmp_fh = Git::SVN->_temp_file('hash_temp');\n \t\tmy $result;\n \t\twhile ($result = sysread($fh, my $string, 1024)) {\n \t\t\tmy $wrote = syswrite($tmp_fh, $string, $result);\n \t\t\tdefined($wrote) && $wrote == $result\n-\t\t\t\tor croak(\"write $tmp_filename: $!\\n\");\n+\t\t\t\tor croak(\"write $tmp_fh->filename: $!\\n\");\n \t\t}\n \t\tdefined $result or croak $!;\n-\t\tclose $tmp_fh or croak $!;\n \n-\t\tclose $fh or croak $!;\n \n-\t\t$hash = $::_repository->hash_and_insert_object($tmp_filename);\n-\t\tunlink($tmp_filename);\n+\t\t$hash = $::_repository->hash_and_insert_object($tmp_fh->filename);\n \t\t$hash =~ /^[a-f\\d]{40}$/ or die \"not a sha1: $hash\\n\";\n-\t\tclose $fb->{base} or croak $!;\n \t} else {\n \t\t$hash = $fb->{blob} or die \"no blob information\\n\";\n \t}\n@@ -3659,7 +3673,7 @@ sub chg_file {\n \t} elsif ($m->{mode_b} !~ /755$/ && $m->{mode_a} =~ /755$/) {\n \t\t$self->change_file_prop($fbat,'svn:executable',undef);\n \t}\n-\tmy $fh = IO::File->new_tmpfile or croak $!;\n+\tmy $fh = Git::SVN->_temp_file('git_blob_temp');\n \tif ($m->{mode_b} =~ /^120/) {\n \t\tprint $fh 'link ' or croak $!;\n \t\t$self->change_file_prop($fbat,'svn:special','*');\n@@ -3679,8 +3693,6 @@ sub chg_file {\n \tmy $got = SVN::TxDelta::send_stream($fh, @$atd, $pool);\n \tdie \"Checksum mismatch\\nexpected: $exp\\ngot: $got\\n\" if ($got ne $exp);\n \t$pool->clear;\n-\n-\tclose $fh or croak $!;\n }\n \n sub D {\n-- \n1.6.0.rc2.4.g39f8\n"},{"id":"86555","messageId":"7vd4kjazaz.fsf@gitster.siamese.dyndns.org","threadId":"14900","inReplyTo":"1218235313-19480-1-git-send-email-marcus@griep.us","subject":"Re: [PATCH] git-svn: Make it scream by minimizing temp files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-08T22:59:16Z","receivedAt":"2008-08-08T22:59:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marcus Griep writes:\n\n> Currently, git-svn would create a temp file on four occasions:\n> 1. Reading a blob out of the object db\n> 2. Creating a delta from svn\n> 3. Hashing and writing a blob into the object db\n> 4. Reading a blob out of the object db (in another place in code)\n>\n> Any time git-svn did the above, it would dutifully create and then\n> delete said temp file. Unfortunately, this means that between 2-4\n> temporary files are created/deleted per file 'add/modify'-ed in\n> svn (O(n)). This causes significant overhead and helps the inode\n> counter to spin beautifully.\n>\n> By its nature, git-svn is a serial beast. Thus, reusing a temp file\n> does not pose significant problems. \"truncate and seek\" takes much\n> less time than \"unlink and create\". This patch centralizes the\n> tempfile creation and holds onto the tempfile until they are deleted\n> on exit. This significantly reduces file overhead, now requiring\n> at most three (3) temp files per run (O(1)).\n\nBeautifully written analysis of the issue being tackled.\n\nBut optimization patch should be backed by numbers --- do you have a\nbenchmark result of some sort that you would want to include here?\n"},{"id":"86569","messageId":"489CEF06.7050204@griep.us","threadId":"14900","inReplyTo":"7vd4kjazaz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-svn: Make it scream by minimizing temp files","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-09T01:12:38Z","receivedAt":"2008-08-09T01:12:38Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"I am working on that right now; however, against master I am getting\nchecksum mismatches with my svn repository, so generating benchmarks\nagainst that requires committing a revert of ffe256f9, which makes\nthings even slower. My work comp is running cygwin, and that could be \nwhy ffe256f9 is a problem.\n\nI am, however using a smaller repository, namely that of the Boo\nProgramming Language, to run some benchmarks. I'm running it on a \nLinux box, and I'll publish the results as soon as they are ready. \n\nI'll include:\n\nffe256f9 and my patch\nffe256f9 and no patch\nrevert ffe256f9 and my patch\nrevert ffe256f9 and no patch\n\nMarcus\n\nJunio C Hamano wrote:\n> Marcus Griep writes:\n> \n>> Currently, git-svn would create a temp file on four occasions:\n>> 1. Reading a blob out of the object db\n>> 2. Creating a delta from svn\n>> 3. Hashing and writing a blob into the object db\n>> 4. Reading a blob out of the object db (in another place in code)\n>>\n>> Any time git-svn did the above, it would dutifully create and then\n>> delete said temp file. Unfortunately, this means that between 2-4\n>> temporary files are created/deleted per file 'add/modify'-ed in\n>> svn (O(n)). This causes significant overhead and helps the inode\n>> counter to spin beautifully.\n>>\n>> By its nature, git-svn is a serial beast. Thus, reusing a temp file\n>> does not pose significant problems. \"truncate and seek\" takes much\n>> less time than \"unlink and create\". This patch centralizes the\n>> tempfile creation and holds onto the tempfile until they are deleted\n>> on exit. This significantly reduces file overhead, now requiring\n>> at most three (3) temp files per run (O(1)).\n> \n> Beautifully written analysis of the issue being tackled.\n> \n> But optimization patch should be backed by numbers --- do you have a\n> benchmark result of some sort that you would want to include here?\n> \n> \n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"86575","messageId":"20080809062521.GA10480@untitled","threadId":"14900","inReplyTo":"1218235313-19480-1-git-send-email-marcus@griep.us","subject":"Re: [PATCH] git-svn: Make it scream by minimizing temp files","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-09T06:25:21Z","receivedAt":"2008-08-09T06:25:21Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Marcus Griep wrote:\n> Currently, git-svn would create a temp file on four occasions:\n> 1. Reading a blob out of the object db\n> 2. Creating a delta from svn\n> 3. Hashing and writing a blob into the object db\n> 4. Reading a blob out of the object db (in another place in code)\n> \n> Any time git-svn did the above, it would dutifully create and then\n> delete said temp file. Unfortunately, this means that between 2-4\n> temporary files are created/deleted per file 'add/modify'-ed in\n> svn (O(n)). This causes significant overhead and helps the inode\n> counter to spin beautifully.\n> \n> By its nature, git-svn is a serial beast. Thus, reusing a temp file\n> does not pose significant problems. \"truncate and seek\" takes much\n> less time than \"unlink and create\". This patch centralizes the\n> tempfile creation and holds onto the tempfile until they are deleted\n> on exit. This significantly reduces file overhead, now requiring\n> at most three (3) temp files per run (O(1)).\n\nWow. I've considered this in the past didn't think there would be a\nsignificant difference (of course I'm always network I/O bound). Which\nplatform and filesystem are you using are you using for tests?\n\nI don't notice any difference running the test suite on Linux + ext3\nhere, but the test suite is not a good benchmark :)\n\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -1282,6 +1282,8 @@ use Carp qw/croak/;\n> use File::Path qw/mkpath/;\n> use File::Copy qw/copy/;\n> use IPC::Open3;\n> +use File::Temp qw/ :seekable /;\n\nqw/ :seekable / does not appear in my version of Perl (5.8.8-7etch3 from\nDebian stable) Just having \"use File::Temp;\" there works for me.\n\n> sub resolve_local_globs {\n> @@ -2932,6 +2935,23 @@ sub remove_username {\n> \t$_[0] =~ s{^([^:]*://)[^@]+@}{$1};\n> }\n> \n> +sub _temp_file {\n> +\tmy ($self, $fd, $autoflush) = @_;\n> +\tif (defined $TEMP_FILES{$fd}) {\n> +\t\ttruncate $TEMP_FILES{$fd}, 0 or croak $!;\n> +\t\tseek $TEMP_FILES{$fd}, 0, 0 or croak $!;\n\nPerhaps a sysseek in addition to the seek above would help\nwith the problems you mentioned in the other email.\n\n\t\tsysseek $TEMP_FILES{$fd}, 0, 0 or croak $!;\n\n(It doesn't seem to affect me when running the test suite, though).\n\n> +\t} else {\n> +\t\t$TEMP_FILES{$fd} = File::Temp->new(\n> +\t\t\t\t\t\t\t\t\tTEMPLATE => 'GitSvn_XXXXXX',\n> +\t\t\t\t\t\t\t\t\tDIR => File::Spec->tmpdir\n> +\t\t\t\t\t\t\t\t\t) or croak $!;\n\nWay too much indentation :x\n\n> +\t\tif (defined $autoflush) {\n> +\t\t\t$TEMP_FILES{$fd}->autoflush($autoflush);\n> +\t\t}\n\nGiven how much we interact with external programs, I'd rather force\nevery autoflush on every file handle to avoid subtle bugs on\ndifferent platforms. It's faster in some (most?) cases, too.\n\n\nAlso, this seems generic enough that other programs (git-cvsimport\nperhaps) can probably use it, too. So maybe it could go into Git.pm or\na new module, Git/Tempfile.pm?\n\n-- \nEric Wong\n"},{"id":"86601","messageId":"489DBB8A.2060207@griep.us","threadId":"14900","inReplyTo":"20080809062521.GA10480@untitled","subject":"Re: [PATCH] git-svn: Make it scream by minimizing temp files","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-09T15:45:14Z","receivedAt":"2008-08-09T15:45:14Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Eric Wong wrote:\n> Wow. I've considered this in the past didn't think there would be a\n> significant difference (of course I'm always network I/O bound). Which\n> platform and filesystem are you using are you using for tests?\n> \n> I don't notice any difference running the test suite on Linux + ext3\n> here, but the test suite is not a good benchmark :)\n\nYeah, much of the test suite uses small repositories without much history.\nWhere you see the benefit is with large repositories with many files.\nIn such cases, even a small speedup can reduce the total import time \nsignificantly.\n\nMy benchmark against a large repository uses the svn we have at work, but\nthere is currently a planned power outage, so I'll have to wait until \ntonight to run my benchmarks there (and they'll take significant time).\n\nNonetheless, my tests against the smaller Boo repository showed almost no\nchange in user time, but a 10% reduction in system time used. There was\nalso a small (1%) drop in minor page faults. I'm confident in these\nresults, but won't certify them until I'm able to run the tests on a much\nlarger repository.\n\n>> +use File::Temp qw/ :seekable /;\n> \n> qw/ :seekable / does not appear in my version of Perl (5.8.8-7etch3 from\n> Debian stable) Just having \"use File::Temp;\" there works for me.\n\nMy newbishness in perl shows. I'll change it to a simple 'use'.\n\n>> +\t\tseek $TEMP_FILES{$fd}, 0, 0 or croak $!;\n> \n> Perhaps a sysseek in addition to the seek above would help\n> with the problems you mentioned in the other email.\n> \n> \t\tsysseek $TEMP_FILES{$fd}, 0, 0 or croak $!;\n> \n> (It doesn't seem to affect me when running the test suite, though).\n\nSounds like a good idea, but I found the source of my cygwin issue,\nnamely that /tmp (which perl uses for its temp files) was mounted\nin textmode. I fixed that by remounting that folder in binmode.\n\nNonetheless, if consumers may use sysread, after getting the file handle\nthen we'll want to use sysseek.\n\n>> +\t} else {\n>> +\t\t$TEMP_FILES{$fd} = File::Temp->new(\n>> +\t\t\t\t\t\t\t\t\tTEMPLATE => 'GitSvn_XXXXXX',\n>> +\t\t\t\t\t\t\t\t\tDIR => File::Spec->tmpdir\n>> +\t\t\t\t\t\t\t\t\t) or croak $!;\n> \n> Way too much indentation :x\n\nThat's what I get for assuming a tab width of 4. I'll redo it with\nabout half as many tabs.\n\n>> +\t\tif (defined $autoflush) {\n>> +\t\t\t$TEMP_FILES{$fd}->autoflush($autoflush);\n>> +\t\t}\n> \n> Given how much we interact with external programs, I'd rather force\n> every autoflush on every file handle to avoid subtle bugs on\n> different platforms. It's faster in some (most?) cases, too.\n\nThat sounds good to me.\n\n> Also, this seems generic enough that other programs (git-cvsimport\n> perhaps) can probably use it, too. So maybe it could go into Git.pm or\n> a new module, Git/Tempfile.pm?\n\nI'd advocate the latter since it's not really Git functionality, but\nrather a support, so a submodule would perhaps be the better placement.\n\nAlso, I came up with one more optimization inside 'sub close_file', so\nI'll roll that in too. Tell me where you/the community would prefer \nthe tempfile functionality, and I'll submit a new patch series with \none patch for the module and one patch for git-svn.\n\nBy then, I should have some better benchmark results.\n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"86630","messageId":"20080810014625.GA31438@hand.yhbt.net","threadId":"14900","inReplyTo":"489DBB8A.2060207@griep.us","subject":"Re: [PATCH] git-svn: Make it scream by minimizing temp files","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-10T01:46:25Z","receivedAt":"2008-08-10T01:46:25Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Marcus Griep wrote:\n> Eric Wong wrote:\n> > Perhaps a sysseek in addition to the seek above would help\n> > with the problems you mentioned in the other email.\n> > \n> > \t\tsysseek $TEMP_FILES{$fd}, 0, 0 or croak $!;\n> > \n> > (It doesn't seem to affect me when running the test suite, though).\n> \n> Sounds like a good idea, but I found the source of my cygwin issue,\n> namely that /tmp (which perl uses for its temp files) was mounted\n> in textmode. I fixed that by remounting that folder in binmode.\n\nHmm.. Instead of relying on users on weird platforms to change their\nmount options, git-svn should also set binmode on all filehandles\nregardless. Will that get around the problem you had with cygwin?\n\ngit-svn already sets binmode for all the rev_map files.\n\n> Nonetheless, if consumers may use sysread, after getting the file handle\n> then we'll want to use sysseek.\n> \n> >> +\t} else {\n> >> +\t\t$TEMP_FILES{$fd} = File::Temp->new(\n> >> +\t\t\t\t\t\t\t\t\tTEMPLATE => 'GitSvn_XXXXXX',\n> >> +\t\t\t\t\t\t\t\t\tDIR => File::Spec->tmpdir\n> >> +\t\t\t\t\t\t\t\t\t) or croak $!;\n> > \n> > Way too much indentation :x\n> \n> That's what I get for assuming a tab width of 4. I'll redo it with\n> about half as many tabs.\n\nTabwidth is 8 characters by default, and that is what git uses.\n\n> > Also, this seems generic enough that other programs (git-cvsimport\n> > perhaps) can probably use it, too. So maybe it could go into Git.pm or\n> > a new module, Git/Tempfile.pm?\n> \n> I'd advocate the latter since it's not really Git functionality, but\n> rather a support, so a submodule would perhaps be the better placement.\n> \n> Also, I came up with one more optimization inside 'sub close_file', so\n> I'll roll that in too. Tell me where you/the community would prefer \n> the tempfile functionality, and I'll submit a new patch series with \n> one patch for the module and one patch for git-svn.\n> \n> By then, I should have some better benchmark results.\n\nJunio (or anybody else), any thoughts on what the submodule should be\nnamed? I'm not good at naming things :x\n\n-- \nEric Wong\n"},{"id":"86635","messageId":"7v8wv54jar.fsf@gitster.siamese.dyndns.org","threadId":"14900","inReplyTo":"20080810014625.GA31438@hand.yhbt.net","subject":"Re: [PATCH] git-svn: Make it scream by minimizing temp files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-10T03:53:48Z","receivedAt":"2008-08-10T03:53:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Junio (or anybody else), any thoughts on what the submodule should be\n> named? I'm not good at naming things :x\n\nI'd say putting it in Git.pm itself is fine. Git.pm is to give common\nservices to Porcelains, and we already have things like command_*_pipe()\nfamily of functions that do not have to be git specific.\n\nI'd be a bit surprised if there isn't any existing CPAN module for things\nlike this, though...\n"},{"id":"86638","messageId":"20080810074742.GB31438@hand.yhbt.net","threadId":"14900","inReplyTo":"7v8wv54jar.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-svn: Make it scream by minimizing temp files","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-10T07:47:42Z","receivedAt":"2008-08-10T07:47:42Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Eric Wong writes:\n> \n> > Junio (or anybody else), any thoughts on what the submodule should be\n> > named? I'm not good at naming things :x\n> \n> I'd say putting it in Git.pm itself is fine. Git.pm is to give common\n> services to Porcelains, and we already have things like command_*_pipe()\n> family of functions that do not have to be git specific.\n\nOK.\n\n> I'd be a bit surprised if there isn't any existing CPAN module for things\n> like this, though...\n\nWow, I am surprised. I couldn't find anything in a few minutes of\nsearching...\n\n-- \nEric Wong\n"},{"id":"86642","messageId":"20080810080956.GB21575@untitled","threadId":"14900","inReplyTo":"489DBB8A.2060207@griep.us","subject":"Re: [PATCH] git-svn: Make it scream by minimizing temp files","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-10T08:09:56Z","receivedAt":"2008-08-10T08:09:56Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Marcus Griep wrote:\n> Also, I came up with one more optimization inside 'sub close_file',\n\nWould that be truncating the file immediately after we're done using it?\n\nPreviously IO->new_tmpfile would unlink the file immediately after it\ngot created; so closing the file descriptor immediately after use would\nallow the OS it to bypass the actual writeout to disk on an asynchronous\nfilesystem.\n\n-- \nEric Wong\n"},{"id":"86643","messageId":"7vr68x2s3h.fsf@gitster.siamese.dyndns.org","threadId":"14900","inReplyTo":"20080810074742.GB31438@hand.yhbt.net","subject":"Re: [PATCH] git-svn: Make it scream by minimizing temp files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-10T08:26:42Z","receivedAt":"2008-08-10T08:26:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Junio C Hamano wrote:\n> ...\n>> I'd be a bit surprised if there isn't any existing CPAN module for things\n>> like this, though...\n>\n> Wow, I am surprised. I couldn't find anything in a few minutes of\n> searching...\n\nThat's Ok. Even CPAN has something, if it is not widely used and/or if it\ncomes with a lot of unnecessary baggage, we would be better off having the\nsingle function in Git.pm.\n"},{"id":"86783","messageId":"1218470035-13864-1-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":"489DBB8A.2060207@griep.us","subject":"[PATCH 0/3] git-svn and temporary file improvements","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-11T15:53:52Z","receivedAt":"2008-08-11T15:53:52Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"\nThis series of patches relates to temp file usage within git-svn and possible\nextensions applicable to other perl auxiliary functions.\n\nThe first patch allows for a central \"registry\" of temp files to be maintained.\nIt offers both locking and non-locking constructs depending upon the user's\ncomplexity concern. The functions provided are also documented for perldoc.\n\nThe second patch changes git-svn to utilize the central registry in the first\npatch to help reduce the amount of temp files created and destroyed during a\nnormal run of git-svn. The asymptotic limit on the number of temp files needed\nis decreased from O(n+m) to O(1) where n is the number of files imported and\nm is the number of file deltas. In real terms, this change does not\nsignificantly reduce the time required for an import as other concerns, such as\nnetwork and disk i/o dominate over inode/MFT changes, however an incremental\nreduction of ~10% system time was found on large change sets, though in a large\nrepository of small changesets, this incremental reduction reduced to \napproximately 3%.\n\nThe third patch modifies the way git-svn handles symlinks versus normal files\nimported from svn. Currently, git-svn is very inefficient in this respect,\nduplicating entire files solely for the sake of eliminating the first five\nbytes of the file if it is a symlink. This causes a large amount of unnecessary\ndisk i/o, even when considering most of it takes place in in-memory buffers.\nBy eliminating the unnecessary duplication for normal files, a significant 48%\nreduction in system time and a 33% reduction in user time was realized on\nlarge changesets. Over many commits with small changesets, other operations\ndominate, but an incremental 6% reduction was still noted. In addition, in both\ncases a 15-25% reduction in maximum resident set size was found.\n\nLogs and results of the benchmarks along with the procedure used are available\nat http://blog.xpdm.us/2008/08/git-svn-and-temporary-files.html.\n\nMarcus Griep (3):\n Git.pm: Add faculties to allow temp files to be cached\n git-svn: Make it scream by minimizing temp files\n git-svn: Reduce temp file usage when dealing with non-links\n\n git-svn.perl | 84 ++++++++++++++++++++--------------\n perl/Git.pm | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 192 insertions(+), 37 deletions(-)\n"},{"id":"86784","messageId":"1218470035-13864-2-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":"1218470035-13864-1-git-send-email-marcus@griep.us","subject":"[PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-11T15:53:53Z","receivedAt":"2008-08-11T15:53:53Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"This patch offers a generic interface to allow temp files to be\ncached while using an instance of the 'Git' package. If many\ntemp files are created and destroyed during the execution of a\nprogram, this caching mechanism can help reduce the amount of\nfiles created and destroyed by the filesystem.\n\nThere are two methods offered for creating a new file: a no-lock and\na acquire-lock version. The no-lock version provides no\nguarantee that a file is not in use or that the temp file may be\nstolen by a subsequent request. The acquire-lock version provides a\nweak guarantee that a temp file will not be stolen by subsequent\nrequests even from a no-lock request. If a file is locked when\nanother acquire request is made, a simple error is thrown.\n\nSigned-off-by: Marcus Griep \n---\n perl/Git.pm | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 143 insertions(+), 2 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex e1ca5b4..fc24f55 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -57,7 +57,8 @@ require Exporter;\n command_output_pipe command_input_pipe command_close_pipe\n command_bidi_pipe command_close_bidi_pipe\n version exec_path hash_object git_cmd_try\n- remote_refs);\n+ remote_refs\n+ temp_acquire temp_release temp_unsafe temp_reset);\n \n \n =head1 DESCRIPTION\n@@ -99,7 +100,9 @@ use Carp qw(carp croak); # but croak is bad - throw instead\n use Error qw(:try);\n use Cwd qw(abs_path);\n use IPC::Open2 qw(open2);\n-\n+use File::Temp ();\n+require File::Spec;\n+use Fcntl qw(SEEK_SET);\n }\n \n \n@@ -933,6 +936,143 @@ sub _close_cat_blob {\n \tdelete @$self{@vars};\n }\n \n+\n+{ # %TEMP_* Lexical Context\n+\n+my (%TEMP_LOCKS, %TEMP_FILES);\n+\n+=item temp_acquire ( NAME )\n+\n+Attempts to retreive the temporary file mapped to the string C. If an\n+associated temp file has not been created this session or was closed, it is\n+created, cached, and set for autoflush and binmode.\n+\n+Internally locks the file mapped to C. This lock must be released with\n+C when the temp file is no longer needed. Subsequent attempts\n+to retrieve temporary files mapped to the same C while still locked will\n+cause an error. This locking mechanism provides a weak guarantee and is not\n+threadsafe. It does provide some error checking to help prevent temp file refs\n+writing over one another.\n+\n+The L returned is truncated and seeked to position 0.\n+\n+In general, the L returned should not be closed by consumers as\n+it defeats the purpose of this caching mechanism. If you need to close the temp\n+file handle, then you should use L or another temp file faculty\n+directly. If a handle is closed and then requested again, then a warning will\n+issue.\n+\n+=cut\n+\n+sub temp_acquire {\n+\tmy ($self, $name) = _maybe_self(@_);\n+\n+\tmy $temp_fd = _temp_cache($name);\n+\n+\t$TEMP_LOCKS{$temp_fd} = 1;\n+\t$temp_fd;\n+}\n+\n+=item temp_release ( NAME [, BOOL] )\n+\n+=item temp_release ( FILEHANDLE [, BOOL] )\n+\n+Releases a lock acquired through C. Can be called either with\n+the C mapping used when acquiring the temp file or with the C\n+referencing a locked temp file.\n+\n+Warns if an attempt is made to release a file that is not locked.\n+\n+If called with C true, then the temp file will be truncated before being\n+released. This can help to reduce disk I/O where the system is smart enough to\n+detect the truncation while data is in the output buffers.\n+\n+=cut\n+\n+sub temp_release {\n+\tmy ($self, $temp_fd, $trunc) = _maybe_self(@_);\n+\n+\tif (ref($temp_fd) ne 'File::Temp') {\n+\t\t$temp_fd = $TEMP_FILES{$temp_fd};\n+\t}\n+\tunless ($TEMP_LOCKS{$temp_fd}) {\n+\t\tcarp \"Attempt to release temp file '$temp_fd' that has not been locked\";\n+\t}\n+\ttemp_reset($temp_fd) if $trunc and $temp_fd->opened;\n+\n+\t$TEMP_LOCKS{$temp_fd} = 0;\n+\tundef;\n+}\n+\n+=item temp_unsafe ( NAME )\n+\n+Attempts to retreive the temporary file mapped to the string C. If an\n+associated temp file has not been created this session or was closed, it is\n+created, cached, and set for autoflush and binmode.\n+\n+If the file mapped to C has been locked using C, then\n+this method will throw an L.\n+\n+The L returned is truncated and seeked to position 0.\n+\n+In general, the L returned should not be closed by consumers as\n+it defeats the purpose of this caching mechanism. If you need to close the temp\n+file handle, then you should use L or another temp file faculty\n+directly. If a handle is closed and then requested again, then a warning will\n+issue.\n+\n+=cut\n+\n+sub temp_unsafe {\n+\tmy ($self, $name) = _maybe_self(@_);\n+\n+\t_temp_cache($name);\n+}\n+\n+sub _temp_cache {\n+\tmy ($name) = @_;\n+\n+\tmy $temp_fd = \\$TEMP_FILES{$name};\n+\tif (defined $$temp_fd and $$temp_fd->opened) {\n+\t\tif ($TEMP_LOCKS{$$temp_fd}) {\n+\t\t\tthrow Error::Simple(\"Temp file with moniker '$name' already in use\");\n+\t\t}\n+\t\ttemp_reset($$temp_fd);\n+\t} else {\n+\t\tif (defined $$temp_fd) { # then we're here because of a closed handle.\n+\t\t\tcarp \"Temp file '$name' was closed. Opening replacement.\";\n+\t\t}\n+\t\t$$temp_fd = File::Temp->new(\n+\t\t\tTEMPLATE => 'Git_XXXXXX',\n+\t\t\tDIR => File::Spec->tmpdir\n+\t\t\t) or throw Error::Simple(\"couldn't open new temp file\");\n+\t\t$$temp_fd->autoflush;\n+\t\tbinmode $$temp_fd;\n+\t}\n+\t$$temp_fd;\n+}\n+\n+=item temp_reset ( FILEHANDLE )\n+\n+Truncates and resets the position of the C. Uses C.\n+\n+=cut\n+\n+sub temp_reset {\n+\tmy ($self, $temp_fd) = _maybe_self(@_);\n+\n+\ttruncate $temp_fd, 0\n+\t\tor throw Error::Simple(\"couldn't truncate file\");\n+\tsysseek $temp_fd, 0, SEEK_SET\n+\t\tor throw Error::Simple(\"couldn't seek to beginning of file\");\n+}\n+\n+sub END {\n+\tunlink values %TEMP_FILES if %TEMP_FILES;\n+}\n+\n+} # %TEMP_* Lexical Context\n+\n =back\n \n =head1 ERROR HANDLING\n@@ -1153,6 +1293,7 @@ sub DESTROY {\n \tmy ($self) = @_;\n \t$self->_close_hash_and_insert_object();\n \t$self->_close_cat_blob();\n+\tunlink values %{$self->{temp_files}} if $self->{temp_files};\n }\n \n \n-- \n1.6.0.rc2.6.g8eda3\n"},{"id":"86785","messageId":"1218470035-13864-3-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":"1218470035-13864-2-git-send-email-marcus@griep.us","subject":"[PATCH 2/3] git-svn: Make it scream by minimizing temp files","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-11T15:53:54Z","receivedAt":"2008-08-11T15:53:54Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Currently, git-svn would create a temp file on four occasions:\n1. Reading a blob out of the object db\n2. Creating a delta from svn\n3. Hashing and writing a blob into the object db\n4. Reading a blob out of the object db (in another place in code)\n\nAny time git-svn did the above, it would dutifully create and then\ndelete said temp file. Unfortunately, this means that between 2-4\ntemporary files are created/deleted per file 'add/modify'-ed in\nsvn (O(n)). This causes significant overhead and helps the inode\ncounter to spin beautifully.\n\nBy its nature, git-svn is a serial beast. Thus, reusing a temp file\ndoes not pose significant problems. \"truncate and seek\" takes much\nless time than \"unlink and create\". This patch centralizes the\ntempfile creation and holds onto the tempfile until they are deleted\non exit. This significantly reduces file overhead, now requiring\nat most three (3) temp files per run (O(1)).\n\nSigned-off-by: Marcus Griep \n---\n git-svn.perl | 53 +++++++++++++++++++++++++++++++++++------------------\n 1 files changed, 35 insertions(+), 18 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex fe78461..0937918 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1260,7 +1260,7 @@ sub md5sum {\n \tmy $arg = shift;\n \tmy $ref = ref $arg;\n \tmy $md5 = Digest::MD5->new();\n- if ($ref eq 'GLOB' || $ref eq 'IO::File') {\n+ if ($ref eq 'GLOB' || $ref eq 'IO::File' || $ref eq 'File::Temp') {\n \t\t$md5->addfile($arg) or croak $!;\n \t} elsif ($ref eq 'SCALAR') {\n \t\t$md5->add($$arg) or croak $!;\n@@ -1285,6 +1285,8 @@ use Carp qw/croak/;\n use File::Path qw/mkpath/;\n use File::Copy qw/copy/;\n use IPC::Open3;\n+use File::Temp ();\n+use File::Spec;\n \n my ($_gc_nr, $_gc_period);\n \n@@ -1323,10 +1325,11 @@ BEGIN {\n \t}\n }\n \n-my (%LOCKFILES, %INDEX_FILES);\n+my (%LOCKFILES, %INDEX_FILES, %TEMP_FILES);\n END {\n \tunlink keys %LOCKFILES if %LOCKFILES;\n \tunlink keys %INDEX_FILES if %INDEX_FILES;\n+\tunlink values %TEMP_FILES if %TEMP_FILES;\n }\n \n sub resolve_local_globs {\n@@ -2935,6 +2938,21 @@ sub remove_username {\n \t$_[0] =~ s{^([^:]*://)[^@]+@}{$1};\n }\n \n+sub _temp_file {\n+\tmy ($self, $fd) = @_;\n+\tif (defined $TEMP_FILES{$fd}) {\n+\t\ttruncate $TEMP_FILES{$fd}, 0 or croak $!;\n+\t\tsysseek $TEMP_FILES{$fd}, 0, 0 or croak $!;\n+\t} else {\n+\t\t$TEMP_FILES{$fd} = File::Temp->new(\n+\t\t\tTEMPLATE => 'GitSvn_XXXXXX',\n+\t\t\tDIR => File::Spec->tmpdir\n+\t\t\t) or croak $!;\n+\t\t$TEMP_FILES{$fd}->autoflush(1);\n+\t}\n+\t$TEMP_FILES{$fd};\n+}\n+\n package Git::SVN::Prompt;\n use strict;\n use warnings;\n@@ -3225,13 +3243,11 @@ sub change_file_prop {\n \n sub apply_textdelta {\n \tmy ($self, $fb, $exp) = @_;\n-\tmy $fh = IO::File->new_tmpfile;\n-\t$fh->autoflush(1);\n+\tmy $fh = Git::temp_acquire('svn_delta');\n \t# $fh gets auto-closed() by SVN::TxDelta::apply(),\n \t# (but $base does not,) so dup() it for reading in close_file\n \topen my $dup, '<&', $fh or croak $!;\n-\tmy $base = IO::File->new_tmpfile;\n-\t$base->autoflush(1);\n+\tmy $base = Git::temp_acquire('git_blob');\n \tif ($fb->{blob}) {\n \t\tprint $base 'link ' if ($fb->{mode_a} == 120000);\n \t\tmy $size = $::_repository->cat_blob($fb->{blob}, $base);\n@@ -3246,9 +3262,10 @@ sub apply_textdelta {\n \t\t}\n \t}\n \tseek $base, 0, 0 or croak $!;\n-\t$fb->{fh} = $dup;\n+\t$fb->{fh} = $fh;\n \t$fb->{base} = $base;\n-\t[ SVN::TxDelta::apply($base, $fh, undef, $fb->{path}, $fb->{pool}) ];\n+\tmy $return = [ SVN::TxDelta::apply($base, $dup, undef, $fb->{path}, $fb->{pool}) ];\n+\t$return;\n }\n \n sub close_file {\n@@ -3277,22 +3294,23 @@ sub close_file {\n \t\t\t}\n \t\t}\n \n-\t\tmy ($tmp_fh, $tmp_filename) = File::Temp::tempfile(UNLINK => 1);\n+\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n \t\tmy $result;\n \t\twhile ($result = sysread($fh, my $string, 1024)) {\n \t\t\tmy $wrote = syswrite($tmp_fh, $string, $result);\n \t\t\tdefined($wrote) && $wrote == $result\n-\t\t\t\tor croak(\"write $tmp_filename: $!\\n\");\n+\t\t\t\tor croak(\"write $tmp_fh->filename: $!\\n\");\n \t\t}\n \t\tdefined $result or croak $!;\n-\t\tclose $tmp_fh or croak $!;\n \n-\t\tclose $fh or croak $!;\n \n-\t\t$hash = $::_repository->hash_and_insert_object($tmp_filename);\n-\t\tunlink($tmp_filename);\n+\t\tGit::temp_release($fh, 1);\n+\n+\t\t$hash = $::_repository->hash_and_insert_object($tmp_fh->filename);\n \t\t$hash =~ /^[a-f\\d]{40}$/ or die \"not a sha1: $hash\\n\";\n-\t\tclose $fb->{base} or croak $!;\n+\n+\t\tGit::temp_release($fb->{base}, 1);\n+\t\tGit::temp_release($tmp_fh, 1);\n \t} else {\n \t\t$hash = $fb->{blob} or die \"no blob information\\n\";\n \t}\n@@ -3662,7 +3680,7 @@ sub chg_file {\n \t} elsif ($m->{mode_b} !~ /755$/ && $m->{mode_a} =~ /755$/) {\n \t\t$self->change_file_prop($fbat,'svn:executable',undef);\n \t}\n-\tmy $fh = IO::File->new_tmpfile or croak $!;\n+\tmy $fh = Git::temp_acquire('git_blob');\n \tif ($m->{mode_b} =~ /^120/) {\n \t\tprint $fh 'link ' or croak $!;\n \t\t$self->change_file_prop($fbat,'svn:special','*');\n@@ -3681,9 +3699,8 @@ sub chg_file {\n \tmy $atd = $self->apply_textdelta($fbat, undef, $pool);\n \tmy $got = SVN::TxDelta::send_stream($fh, @$atd, $pool);\n \tdie \"Checksum mismatch\\nexpected: $exp\\ngot: $got\\n\" if ($got ne $exp);\n+\tGit::temp_release($fh, 1);\n \t$pool->clear;\n-\n-\tclose $fh or croak $!;\n }\n \n sub D {\n-- \n1.6.0.rc2.6.g8eda3\n"},{"id":"86782","messageId":"1218470035-13864-4-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":"1218470035-13864-3-git-send-email-marcus@griep.us","subject":"[PATCH 3/3] git-svn: Reduce temp file usage when dealing with non-links","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-11T15:53:55Z","receivedAt":"2008-08-11T15:53:55Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Currently, in sub 'close_file', git-svn creates a temporary file and\ncopies the contents of the blob to be written into it. This is useful\nfor symlinks because svn stores symlinks in the form:\n\nlink $FILE_PATH\n\nGit creates a blob only out of '$FILE_PATH' and uses file mode to\nindicate that the blob should be interpreted as a symlink.\n\nAs git-hash-object is invoked with --stdin-paths, a duplicate of the\nlink from svn must be created that leaves off the first five bytes,\ni.e. 'link '. However, this is wholly unnecessary for normal blobs,\nthough, as we already have a temp file with their contents. Copying\nthe entire file gains nothing, and effectively requires a file to be\nwritten twice before making it into the object db.\n\nThis patch corrects that issue, holding onto the substr-like\nduplication for symlinks, but skipping it altogether for normal blobs\nby reusing the existing temp file.\n\nSigned-off-by: Marcus Griep \n---\n git-svn.perl | 43 ++++++++++++++++++++-----------------------\n 1 files changed, 20 insertions(+), 23 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 0937918..f53afaa 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -3281,36 +3281,33 @@ sub close_file {\n \t\t\t\t \"expected: $exp\\n got: $got\\n\";\n \t\t\t}\n \t\t}\n-\t\tsysseek($fh, 0, 0) or croak $!;\n \t\tif ($fb->{mode_b} == 120000) {\n-\t\t\teval {\n-\t\t\t\tsysread($fh, my $buf, 5) == 5 or croak $!;\n-\t\t\t\t$buf eq 'link ' or die \"$path has mode 120000\",\n-\t\t\t\t\t\t \" but is not a link\";\n-\t\t\t};\n-\t\t\tif ($@) {\n-\t\t\t\twarn \"$@\\n\";\n-\t\t\t\tsysseek($fh, 0, 0) or croak $!;\n-\t\t\t}\n-\t\t}\n-\n-\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n-\t\tmy $result;\n-\t\twhile ($result = sysread($fh, my $string, 1024)) {\n-\t\t\tmy $wrote = syswrite($tmp_fh, $string, $result);\n-\t\t\tdefined($wrote) && $wrote == $result\n-\t\t\t\tor croak(\"write $tmp_fh->filename: $!\\n\");\n-\t\t}\n-\t\tdefined $result or croak $!;\n+\t\t\tsysseek($fh, 0, 0) or croak $!;\n+\t\t\tsysread($fh, my $buf, 5) == 5 or croak $!;\n \n+\t\t\tunless ($buf eq 'link ') {\n+\t\t\t\twarn \"$path has mode 120000\",\n+\t\t\t\t\t\t\" but is not a link\\n\";\n+\t\t\t} else {\n+\t\t\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n+\t\t\t\tmy $result;\n+\t\t\t\twhile ($result = sysread($fh, my $string, 1024)) {\n+\t\t\t\t\tmy $wrote = syswrite($tmp_fh, $string, $result);\n+\t\t\t\t\tdefined($wrote) && $wrote == $result\n+\t\t\t\t\t\tor croak(\"write $tmp_fh->filename: $!\\n\");\n+\t\t\t\t}\n+\t\t\t\tdefined $result or croak $!;\n \n-\t\tGit::temp_release($fh, 1);\n+\t\t\t\t($fh, $tmp_fh) = ($tmp_fh, $fh);\n+\t\t\t\tGit::temp_release($tmp_fh, 1);\n+\t\t\t}\n+\t\t}\n \n-\t\t$hash = $::_repository->hash_and_insert_object($tmp_fh->filename);\n+\t\t$hash = $::_repository->hash_and_insert_object($fh->filename);\n \t\t$hash =~ /^[a-f\\d]{40}$/ or die \"not a sha1: $hash\\n\";\n \n \t\tGit::temp_release($fb->{base}, 1);\n-\t\tGit::temp_release($tmp_fh, 1);\n+\t\tGit::temp_release($fh, 1);\n \t} else {\n \t\t$hash = $fb->{blob} or die \"no blob information\\n\";\n \t}\n-- \n1.6.0.rc2.6.g8eda3\n"},{"id":"86895","messageId":"20080812030809.GA14051@untitled","threadId":"14900","inReplyTo":"1218470035-13864-2-git-send-email-marcus@griep.us","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-12T03:08:09Z","receivedAt":"2008-08-12T03:08:09Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Marcus Griep wrote:\n> This patch offers a generic interface to allow temp files to be\n> cached while using an instance of the 'Git' package. If many\n> temp files are created and destroyed during the execution of a\n> program, this caching mechanism can help reduce the amount of\n> files created and destroyed by the filesystem.\n> \n> There are two methods offered for creating a new file: a no-lock and\n> a acquire-lock version. The no-lock version provides no\n> guarantee that a file is not in use or that the temp file may be\n> stolen by a subsequent request. The acquire-lock version provides a\n> weak guarantee that a temp file will not be stolen by subsequent\n> requests even from a no-lock request. If a file is locked when\n> another acquire request is made, a simple error is thrown.\n\nI'm not sure if the no-lock version is worth the potential for\nbuggy or dangerous code. I like this new idea of locking the\nfiles to prevent bugs.\n\n> +=item temp_release ( NAME [, BOOL] )\n> +\n> +=item temp_release ( FILEHANDLE [, BOOL] )\n> +\n> +Releases a lock acquired through C. Can be called either with\n> +the C mapping used when acquiring the temp file or with the C\n> +referencing a locked temp file.\n> +\n> +Warns if an attempt is made to release a file that is not locked.\n> +\n> +If called with C true, then the temp file will be truncated before being\n> +released. This can help to reduce disk I/O where the system is smart enough to\n> +detect the truncation while data is in the output buffers.\n\nAlways truncating on release makes the interface simpler. With locking,\nwe can probably *only* truncate on release if you're that worried about\nthe extra overhead :)\n\n> +=item temp_reset ( FILEHANDLE )\n> +\n> +Truncates and resets the position of the C. Uses C.\n> +\n> +=cut\n> +\n> +sub temp_reset {\n> +\tmy ($self, $temp_fd) = _maybe_self(@_);\n> +\n> +\ttruncate $temp_fd, 0\n> +\t\tor throw Error::Simple(\"couldn't truncate file\");\n\nI would do a regular seek() here in addition to the sysseek() below. I\nam not certain one of the many userspace buffering layers Perl can\npotentially use doesn't do anything funky with its offset accounting.\n\n> +\tsysseek $temp_fd, 0, SEEK_SET\n> +\t\tor throw Error::Simple(\"couldn't seek to beginning of file\");\n\nI would also put a tell() here after the sysseek and throw an error if\nit returns a non-zero value just in case. Yes, I'm really paranoid\nabout this stuff and have a huge distrust of userspace I/O layers :)\n\n-- \nEric Wong\n"},{"id":"86896","messageId":"20080812031442.GB14051@untitled","threadId":"14900","inReplyTo":"1218470035-13864-3-git-send-email-marcus@griep.us","subject":"Re: [PATCH 2/3] git-svn: Make it scream by minimizing temp files","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-12T03:14:42Z","receivedAt":"2008-08-12T03:14:42Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Marcus Griep wrote:\n> ---\n> git-svn.perl | 53 +++++++++++++++++++++++++++++++++++------------------\n> 1 files changed, 35 insertions(+), 18 deletions(-)\n> \n> diff --git a/git-svn.perl b/git-svn.perl\n> index fe78461..0937918 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -1285,6 +1285,8 @@ use Carp qw/croak/;\n> use File::Path qw/mkpath/;\n> use File::Copy qw/copy/;\n> use IPC::Open3;\n> +use File::Temp ();\n> +use File::Spec;\n> \n> my ($_gc_nr, $_gc_period);\n> \n> @@ -1323,10 +1325,11 @@ BEGIN {\n> \t}\n> }\n> \n> -my (%LOCKFILES, %INDEX_FILES);\n> +my (%LOCKFILES, %INDEX_FILES, %TEMP_FILES);\n> END {\n> \tunlink keys %LOCKFILES if %LOCKFILES;\n> \tunlink keys %INDEX_FILES if %INDEX_FILES;\n> +\tunlink values %TEMP_FILES if %TEMP_FILES;\n> }\n \n> sub resolve_local_globs {\n> @@ -2935,6 +2938,21 @@ sub remove_username {\n> \t$_[0] =~ s{^([^:]*://)[^@]+@}{$1};\n> }\n> \n> +sub _temp_file {\n> +\tmy ($self, $fd) = @_;\n> +\tif (defined $TEMP_FILES{$fd}) {\n> +\t\ttruncate $TEMP_FILES{$fd}, 0 or croak $!;\n> +\t\tsysseek $TEMP_FILES{$fd}, 0, 0 or croak $!;\n> +\t} else {\n> +\t\t$TEMP_FILES{$fd} = File::Temp->new(\n> +\t\t\tTEMPLATE => 'GitSvn_XXXXXX',\n> +\t\t\tDIR => File::Spec->tmpdir\n> +\t\t\t) or croak $!;\n> +\t\t$TEMP_FILES{$fd}->autoflush(1);\n> +\t}\n> +\t$TEMP_FILES{$fd};\n> +}\n> +\n\nThe above is dead code now that we're using the versions in Git::,\nright?\n\n> @@ -3246,9 +3262,10 @@ sub apply_textdelta {\n> -\t[ SVN::TxDelta::apply($base, $fh, undef, $fb->{path}, $fb->{pool}) ];\n> +\tmy $return = [ SVN::TxDelta::apply($base, $dup, undef, $fb->{path}, $fb->{pool}) ];\n> +\t$return;\n\nWhy create a temporary variable? (and break the sacred 80-column limit).\n\n> @@ -3277,22 +3294,23 @@ sub close_file {\n> -\t\t\t\tor croak(\"write $tmp_filename: $!\\n\");\n> +\t\t\t\tor croak(\"write \", $tmp_fh->filename, \": $!\\n\");\n\nI missed this before, but $tmp_fh->filename doesn't interpolate correctly.\n\n(\"write ${\\$tmp_fh->filename}: $!\\n\")\nor\n(\"write \", $tmp_fh->filename, \": $!\\n\") works.\n\nI believe the latter form is more idiomatic so we should probably use\nthat...\n\n-- \nEric Wong\n"},{"id":"86897","messageId":"20080812033700.GC14051@untitled","threadId":"14900","inReplyTo":"1218470035-13864-4-git-send-email-marcus@griep.us","subject":"Re: [PATCH 3/3] git-svn: Reduce temp file usage when dealing with non-links","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-12T03:37:00Z","receivedAt":"2008-08-12T03:37:00Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Marcus Griep wrote:\n> Currently, in sub 'close_file', git-svn creates a temporary file and\n> copies the contents of the blob to be written into it. This is useful\n> for symlinks because svn stores symlinks in the form:\n> \n> link $FILE_PATH\n> \n> Git creates a blob only out of '$FILE_PATH' and uses file mode to\n> indicate that the blob should be interpreted as a symlink.\n> \n> As git-hash-object is invoked with --stdin-paths, a duplicate of the\n> link from svn must be created that leaves off the first five bytes,\n> i.e. 'link '. However, this is wholly unnecessary for normal blobs,\n> though, as we already have a temp file with their contents. Copying\n> the entire file gains nothing, and effectively requires a file to be\n> written twice before making it into the object db.\n> \n> This patch corrects that issue, holding onto the substr-like\n> duplication for symlinks, but skipping it altogether for normal blobs\n> by reusing the existing temp file.\n\nSweet optimization! Thanks!\n\n\nOne thing, again, can you please make sure things don't exceed\n80-columns when using 8 character-wide tabs?\n\nI'm not sure how much it matters to the guys maintaining Git.pm, but\nthat's the standard for here and the Linux kernel (although it\nunfortunately seems to have gotten more lax in recent years...).\n\nLarger monitors can't help me because I use big fonts that would require\nme to move my neck or eyes to see across the screen, leading to more eye\nand neck strain (I have a bad neck). I very much wish ANSI had\nstandardized on something even smaller, perhaps 64-char wide terminals\n:)\n\n-- \nEric Wong\n"},{"id":"86935","messageId":"48A1AF28.5000400@griep.us","threadId":"14900","inReplyTo":"20080812030809.GA14051@untitled","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-12T15:41:28Z","receivedAt":"2008-08-12T15:41:28Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Eric Wong wrote:\n> I'm not sure if the no-lock version is worth the potential for\n> buggy or dangerous code. I like this new idea of locking the\n> files to prevent bugs.\n\nI can agree with that, and the \"unsafe\" version was just a front to the\ncommon function. I've removed the unsafe version from @EXPORT_OK and\nremoved temp_unsafe, but _temp_cached is still available for those\nthat _really_ want the unsafe version.\n\n> Always truncating on release makes the interface simpler. With locking,\n> we can probably *only* truncate on release if you're that worried about\n> the extra overhead :)\n\nI agree with this. I introduced a nice bug on myself when just starting\nwith it though, which is why I made it optional. Careful conversion and\ntesting should be good enough protection.\n\n> I would do a regular seek() here in addition to the sysseek() below. I\n> am not certain one of the many userspace buffering layers Perl can\n> potentially use doesn't do anything funky with its offset accounting.\n> \n> I would also put a tell() here after the sysseek and throw an error if\n> it returns a non-zero value just in case. Yes, I'm really paranoid\n> about this stuff and have a huge distrust of userspace I/O layers :)\n\nI went ahead and threw in a sysseek(,,SEEK_CUR) with the tell and added\na seek to the sysseek(,,SEEK_SET), so we should be protected on the\nbuffered and unbuffered sides.\n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"86936","messageId":"48A1B13E.2050603@griep.us","threadId":"14900","inReplyTo":"20080812031442.GB14051@untitled","subject":"Re: [PATCH 2/3] git-svn: Make it scream by minimizing temp files","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-12T15:50:22Z","receivedAt":"2008-08-12T15:50:22Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Eric Wong wrote:\n> The above is dead code now that we're using the versions in Git::,\n> right?\n> \n> Why create a temporary variable? (and break the sacred 80-column limit).\n\nMy cherry-picking & squashing skills are not yet up to snuff. The dead\ncode and unnecessary variable have now been removed.\n\n > I missed this before, but $tmp_fh->filename doesn't interpolate correctly.\n> \n> (\"write ${\\$tmp_fh->filename}: $!\\n\")\n> or\n> (\"write \", $tmp_fh->filename, \": $!\\n\") works.\n> \n> I believe the latter form is more idiomatic so we should probably use\n> that...\n\nDone, in the latter form, and fixed in a couple other places.\n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"86937","messageId":"48A1B202.5000204@griep.us","threadId":"14900","inReplyTo":"20080812033700.GC14051@untitled","subject":"Re: [PATCH 3/3] git-svn: Reduce temp file usage when dealing with non-links","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-12T15:53:38Z","receivedAt":"2008-08-12T15:53:38Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Eric Wong wrote:\n> Sweet optimization! Thanks!\n\nI'm glad to have found something significant.\n\n> One thing, again, can you please make sure things don't exceed\n> 80-columns when using 8 character-wide tabs?\n> \n> I'm not sure how much it matters to the guys maintaining Git.pm, but\n> that's the standard for here and the Linux kernel (although it\n> unfortunately seems to have gotten more lax in recent years...).\n> \n> Larger monitors can't help me because I use big fonts that would require\n> me to move my neck or eyes to see across the screen, leading to more eye\n> and neck strain (I have a bad neck). I very much wish ANSI had\n> standardized on something even smaller, perhaps 64-char wide terminals\n> :)\n\nI added a regex to the standard pre-commit hook to check line length, and\nnow it is working properly, even with tabs. If people are interested, I\ncould submit a patch for the standard pre-commit hook that includes a line\nlength check.\n\nOverall, expect new patches to be sent in reply to each email as version 2's.\n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"86938","messageId":"1218556818-14006-1-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":"1218470035-13864-2-git-send-email-marcus@griep.us","subject":"[PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-12T16:00:18Z","receivedAt":"2008-08-12T16:00:18Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"This patch offers a generic interface to allow temp files to be\ncached while using an instance of the 'Git' package. If many\ntemp files are created and destroyed during the execution of a\nprogram, this caching mechanism can help reduce the amount of\nfiles created and destroyed by the filesystem.\n\nThe temp_acquire method provides a weak guarantee that a temp\nfile will not be stolen by subsequent requests. If a file is\nlocked when another acquire request is made, a simple error is\nthrown.\n\nSigned-off-by: Marcus Griep \n---\n perl/Git.pm | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 123 insertions(+), 2 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex e1ca5b4..405f68f 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -57,7 +57,8 @@ require Exporter;\n command_output_pipe command_input_pipe command_close_pipe\n command_bidi_pipe command_close_bidi_pipe\n version exec_path hash_object git_cmd_try\n- remote_refs);\n+ remote_refs\n+ temp_acquire temp_release temp_reset);\n \n \n =head1 DESCRIPTION\n@@ -99,7 +100,9 @@ use Carp qw(carp croak); # but croak is bad - throw instead\n use Error qw(:try);\n use Cwd qw(abs_path);\n use IPC::Open2 qw(open2);\n-\n+use File::Temp ();\n+require File::Spec;\n+use Fcntl qw(SEEK_SET SEEK_CUR);\n }\n \n \n@@ -933,6 +936,124 @@ sub _close_cat_blob {\n \tdelete @$self{@vars};\n }\n \n+\n+{ # %TEMP_* Lexical Context\n+\n+my (%TEMP_LOCKS, %TEMP_FILES);\n+\n+=item temp_acquire ( NAME )\n+\n+Attempts to retreive the temporary file mapped to the string C. If an\n+associated temp file has not been created this session or was closed, it is\n+created, cached, and set for autoflush and binmode.\n+\n+Internally locks the file mapped to C. This lock must be released with\n+C when the temp file is no longer needed. Subsequent attempts\n+to retrieve temporary files mapped to the same C while still locked will\n+cause an error. This locking mechanism provides a weak guarantee and is not\n+threadsafe. It does provide some error checking to help prevent temp file refs\n+writing over one another.\n+\n+In general, the L returned should not be closed by consumers as\n+it defeats the purpose of this caching mechanism. If you need to close the temp\n+file handle, then you should use L or another temp file faculty\n+directly. If a handle is closed and then requested again, then a warning will\n+issue.\n+\n+=cut\n+\n+sub temp_acquire {\n+\tmy ($self, $name) = _maybe_self(@_);\n+\n+\tmy $temp_fd = _temp_cache($name);\n+\n+\t$TEMP_LOCKS{$temp_fd} = 1;\n+\t$temp_fd;\n+}\n+\n+=item temp_release ( NAME )\n+\n+=item temp_release ( FILEHANDLE )\n+\n+Releases a lock acquired through C. Can be called either with\n+the C mapping used when acquiring the temp file or with the C\n+referencing a locked temp file.\n+\n+Warns if an attempt is made to release a file that is not locked.\n+\n+The temp file will be truncated before being released. This can help to reduce\n+disk I/O where the system is smart enough to detect the truncation while data\n+is in the output buffers. Beware that after the temp file is released and\n+truncated, any operations on that file may fail miserably until it is\n+re-acquired. All contents are lost between each release and acquire mapped to\n+the same string.\n+\n+=cut\n+\n+sub temp_release {\n+\tmy ($self, $temp_fd, $trunc) = _maybe_self(@_);\n+\n+\tif (ref($temp_fd) ne 'File::Temp') {\n+\t\t$temp_fd = $TEMP_FILES{$temp_fd};\n+\t}\n+\tunless ($TEMP_LOCKS{$temp_fd}) {\n+\t\tcarp \"Attempt to release temp file '\",\n+\t\t\t$temp_fd, \"' that has not been locked\";\n+\t}\n+\ttemp_reset($temp_fd) if $trunc and $temp_fd->opened;\n+\n+\t$TEMP_LOCKS{$temp_fd} = 0;\n+\tundef;\n+}\n+\n+sub _temp_cache {\n+\tmy ($name) = @_;\n+\n+\tmy $temp_fd = \\$TEMP_FILES{$name};\n+\tif (defined $$temp_fd and $$temp_fd->opened) {\n+\t\tif ($TEMP_LOCKS{$$temp_fd}) {\n+\t\t\tthrow Error::Simple(\"Temp file with moniker '\",\n+\t\t\t\t$name, \"' already in use\");\n+\t\t}\n+\t} else {\n+\t\tif (defined $$temp_fd) {\n+\t\t\t# then we're here because of a closed handle.\n+\t\t\tcarp \"Temp file '\", $name,\n+\t\t\t\t\"' was closed. Opening replacement.\";\n+\t\t}\n+\t\t$$temp_fd = File::Temp->new(\n+\t\t\tTEMPLATE => 'Git_XXXXXX',\n+\t\t\tDIR => File::Spec->tmpdir\n+\t\t\t) or throw Error::Simple(\"couldn't open new temp file\");\n+\t\t$$temp_fd->autoflush;\n+\t\tbinmode $$temp_fd;\n+\t}\n+\t$$temp_fd;\n+}\n+\n+=item temp_reset ( FILEHANDLE )\n+\n+Truncates and resets the position of the C.\n+\n+=cut\n+\n+sub temp_reset {\n+\tmy ($self, $temp_fd) = _maybe_self(@_);\n+\n+\ttruncate $temp_fd, 0\n+\t\tor throw Error::Simple(\"couldn't truncate file\");\n+\tsysseek($temp_fd, 0, SEEK_SET) and seek($temp_fd, 0, SEEK_SET)\n+\t\tor throw Error::Simple(\"couldn't seek to beginning of file\");\n+\tsysseek($temp_fd, 0, SEEK_CUR) == 0 and tell($temp_fd) == 0\n+\t\tor throw Error::Simple(\"expected file position to be reset\");\n+}\n+\n+sub END {\n+\tunlink values %TEMP_FILES if %TEMP_FILES;\n+}\n+\n+} # %TEMP_* Lexical Context\n+\n =back\n \n =head1 ERROR HANDLING\n-- \n1.6.0.rc2.6.g8eda3\n"},{"id":"86939","messageId":"1218556853-25906-1-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":"1218470035-13864-3-git-send-email-marcus@griep.us","subject":"[PATCH 2/3] git-svn: Make it incrementally faster by minimizing temp files","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-12T16:00:53Z","receivedAt":"2008-08-12T16:00:53Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Currently, git-svn would create a temp file on four occasions:\n1. Reading a blob out of the object db\n2. Creating a delta from svn\n3. Hashing and writing a blob into the object db\n4. Reading a blob out of the object db (in another place in code)\n\nAny time git-svn did the above, it would dutifully create and then\ndelete said temp file. Unfortunately, this means that between 2-4\ntemporary files are created/deleted per file 'add/modify'-ed in\nsvn (O(n)). This causes significant overhead and helps the inode\ncounter to spin beautifully.\n\nBy its nature, git-svn is a serial beast. Thus, reusing a temp file\ndoes not pose significant problems. \"truncate and seek\" takes much\nless time than \"unlink and create\". This patch centralizes the\ntempfile creation and holds onto the tempfile until they are deleted\non exit. This significantly reduces file overhead, now requiring\nat most three (3) temp files per run (O(1)).\n\nSigned-off-by: Marcus Griep \n---\n git-svn.perl | 35 ++++++++++++++++++-----------------\n 1 files changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 4dc3380..9eae5e8 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1265,7 +1265,7 @@ sub md5sum {\n \tmy $arg = shift;\n \tmy $ref = ref $arg;\n \tmy $md5 = Digest::MD5->new();\n- if ($ref eq 'GLOB' || $ref eq 'IO::File') {\n+ if ($ref eq 'GLOB' || $ref eq 'IO::File' || $ref eq 'File::Temp') {\n \t\t$md5->addfile($arg) or croak $!;\n \t} elsif ($ref eq 'SCALAR') {\n \t\t$md5->add($$arg) or croak $!;\n@@ -1328,6 +1328,7 @@ BEGIN {\n \t}\n }\n \n+\n my (%LOCKFILES, %INDEX_FILES);\n END {\n \tunlink keys %LOCKFILES if %LOCKFILES;\n@@ -3230,13 +3231,11 @@ sub change_file_prop {\n \n sub apply_textdelta {\n \tmy ($self, $fb, $exp) = @_;\n-\tmy $fh = IO::File->new_tmpfile;\n-\t$fh->autoflush(1);\n+\tmy $fh = Git::temp_acquire('svn_delta');\n \t# $fh gets auto-closed() by SVN::TxDelta::apply(),\n \t# (but $base does not,) so dup() it for reading in close_file\n \topen my $dup, '<&', $fh or croak $!;\n-\tmy $base = IO::File->new_tmpfile;\n-\t$base->autoflush(1);\n+\tmy $base = Git::temp_acquire('git_blob');\n \tif ($fb->{blob}) {\n \t\tprint $base 'link ' if ($fb->{mode_a} == 120000);\n \t\tmy $size = $::_repository->cat_blob($fb->{blob}, $base);\n@@ -3251,9 +3250,9 @@ sub apply_textdelta {\n \t\t}\n \t}\n \tseek $base, 0, 0 or croak $!;\n-\t$fb->{fh} = $dup;\n+\t$fb->{fh} = $fh;\n \t$fb->{base} = $base;\n-\t[ SVN::TxDelta::apply($base, $fh, undef, $fb->{path}, $fb->{pool}) ];\n+\t[ SVN::TxDelta::apply($base, $dup, undef, $fb->{path}, $fb->{pool}) ];\n }\n \n sub close_file {\n@@ -3282,22 +3281,25 @@ sub close_file {\n \t\t\t}\n \t\t}\n \n-\t\tmy ($tmp_fh, $tmp_filename) = File::Temp::tempfile(UNLINK => 1);\n+\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n \t\tmy $result;\n \t\twhile ($result = sysread($fh, my $string, 1024)) {\n \t\t\tmy $wrote = syswrite($tmp_fh, $string, $result);\n \t\t\tdefined($wrote) && $wrote == $result\n-\t\t\t\tor croak(\"write $tmp_filename: $!\\n\");\n+\t\t\t\tor croak(\"write \",\n+\t\t\t\t\t$tmp_fh->filename, \": $!\\n\");\n \t\t}\n \t\tdefined $result or croak $!;\n-\t\tclose $tmp_fh or croak $!;\n \n-\t\tclose $fh or croak $!;\n \n-\t\t$hash = $::_repository->hash_and_insert_object($tmp_filename);\n-\t\tunlink($tmp_filename);\n+\t\tGit::temp_release($fh, 1);\n+\n+\t\t$hash = $::_repository->hash_and_insert_object(\n+\t\t\t\t$tmp_fh->filename);\n \t\t$hash =~ /^[a-f\\d]{40}$/ or die \"not a sha1: $hash\\n\";\n-\t\tclose $fb->{base} or croak $!;\n+\n+\t\tGit::temp_release($fb->{base}, 1);\n+\t\tGit::temp_release($tmp_fh, 1);\n \t} else {\n \t\t$hash = $fb->{blob} or die \"no blob information\\n\";\n \t}\n@@ -3667,7 +3669,7 @@ sub chg_file {\n \t} elsif ($m->{mode_b} !~ /755$/ && $m->{mode_a} =~ /755$/) {\n \t\t$self->change_file_prop($fbat,'svn:executable',undef);\n \t}\n-\tmy $fh = IO::File->new_tmpfile or croak $!;\n+\tmy $fh = Git::temp_acquire('git_blob');\n \tif ($m->{mode_b} =~ /^120/) {\n \t\tprint $fh 'link ' or croak $!;\n \t\t$self->change_file_prop($fbat,'svn:special','*');\n@@ -3686,9 +3688,8 @@ sub chg_file {\n \tmy $atd = $self->apply_textdelta($fbat, undef, $pool);\n \tmy $got = SVN::TxDelta::send_stream($fh, @$atd, $pool);\n \tdie \"Checksum mismatch\\nexpected: $exp\\ngot: $got\\n\" if ($got ne $exp);\n+\tGit::temp_release($fh, 1);\n \t$pool->clear;\n-\n-\tclose $fh or croak $!;\n }\n \n sub D {\n-- \n1.6.0.rc2.6.g8eda3\n"},{"id":"86940","messageId":"1218556876-26554-1-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":"1218470035-13864-4-git-send-email-marcus@griep.us","subject":"[PATCH 3/3] git-svn: Reduce temp file usage when dealing with non-links","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-12T16:01:16Z","receivedAt":"2008-08-12T16:01:16Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Currently, in sub 'close_file', git-svn creates a temporary file and\ncopies the contents of the blob to be written into it. This is useful\nfor symlinks because svn stores symlinks in the form:\n\nlink $FILE_PATH\n\nGit creates a blob only out of '$FILE_PATH' and uses file mode to\nindicate that the blob should be interpreted as a symlink.\n\nAs git-hash-object is invoked with --stdin-paths, a duplicate of the\nlink from svn must be created that leaves off the first five bytes,\ni.e. 'link '. However, this is wholly unnecessary for normal blobs,\nthough, as we already have a temp file with their contents. Copying\nthe entire file gains nothing, and effectively requires a file to be\nwritten twice before making it into the object db.\n\nThis patch corrects that issue, holding onto the substr-like\nduplication for symlinks, but skipping it altogether for normal blobs\nby reusing the existing temp file.\n\nSigned-off-by: Marcus Griep \n---\n git-svn.perl | 46 ++++++++++++++++++++++------------------------\n 1 files changed, 22 insertions(+), 24 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 9eae5e8..95d1510 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -3268,38 +3268,36 @@ sub close_file {\n \t\t\t\t \"expected: $exp\\n got: $got\\n\";\n \t\t\t}\n \t\t}\n-\t\tsysseek($fh, 0, 0) or croak $!;\n \t\tif ($fb->{mode_b} == 120000) {\n-\t\t\teval {\n-\t\t\t\tsysread($fh, my $buf, 5) == 5 or croak $!;\n-\t\t\t\t$buf eq 'link ' or die \"$path has mode 120000\",\n-\t\t\t\t\t\t \" but is not a link\";\n-\t\t\t};\n-\t\t\tif ($@) {\n-\t\t\t\twarn \"$@\\n\";\n-\t\t\t\tsysseek($fh, 0, 0) or croak $!;\n-\t\t\t}\n-\t\t}\n-\n-\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n-\t\tmy $result;\n-\t\twhile ($result = sysread($fh, my $string, 1024)) {\n-\t\t\tmy $wrote = syswrite($tmp_fh, $string, $result);\n-\t\t\tdefined($wrote) && $wrote == $result\n-\t\t\t\tor croak(\"write \",\n-\t\t\t\t\t$tmp_fh->filename, \": $!\\n\");\n-\t\t}\n-\t\tdefined $result or croak $!;\n+\t\t\tsysseek($fh, 0, 0) or croak $!;\n+\t\t\tsysread($fh, my $buf, 5) == 5 or croak $!;\n \n+\t\t\tunless ($buf eq 'link ') {\n+\t\t\t\twarn \"$path has mode 120000\",\n+\t\t\t\t\t\t\" but is not a link\\n\";\n+\t\t\t} else {\n+\t\t\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n+\t\t\t\tmy $res;\n+\t\t\t\twhile ($res = sysread($fh, my $str, 1024)) {\n+\t\t\t\t\tmy $out = syswrite($tmp_fh, $str, $res);\n+\t\t\t\t\tdefined($out) && $out == $res\n+\t\t\t\t\t\tor croak(\"write \",\n+\t\t\t\t\t\t\t$tmp_fh->filename,\n+\t\t\t\t\t\t\t\": $!\\n\");\n+\t\t\t\t}\n+\t\t\t\tdefined $result or croak $!;\n \n-\t\tGit::temp_release($fh, 1);\n+\t\t\t\t($fh, $tmp_fh) = ($tmp_fh, $fh);\n+\t\t\t\tGit::temp_release($tmp_fh, 1);\n+\t\t\t}\n+\t\t}\n \n \t\t$hash = $::_repository->hash_and_insert_object(\n-\t\t\t\t$tmp_fh->filename);\n+\t\t\t\t$fh->filename);\n \t\t$hash =~ /^[a-f\\d]{40}$/ or die \"not a sha1: $hash\\n\";\n \n \t\tGit::temp_release($fb->{base}, 1);\n-\t\tGit::temp_release($tmp_fh, 1);\n+\t\tGit::temp_release($fh, 1);\n \t} else {\n \t\t$hash = $fb->{blob} or die \"no blob information\\n\";\n \t}\n-- \n1.6.0.rc2.6.g8eda3\n"},{"id":"86947","messageId":"1218559539-24304-1-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":"1218556876-26554-1-git-send-email-marcus@griep.us","subject":"[PATCH v2 3/3] git-svn: Reduce temp file usage when dealing with non-links","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-12T16:45:39Z","receivedAt":"2008-08-12T16:45:39Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Currently, in sub 'close_file', git-svn creates a temporary file and\ncopies the contents of the blob to be written into it. This is useful\nfor symlinks because svn stores symlinks in the form:\n\nlink $FILE_PATH\n\nGit creates a blob only out of '$FILE_PATH' and uses file mode to\nindicate that the blob should be interpreted as a symlink.\n\nAs git-hash-object is invoked with --stdin-paths, a duplicate of the\nlink from svn must be created that leaves off the first five bytes,\ni.e. 'link '. However, this is wholly unnecessary for normal blobs,\nthough, as we already have a temp file with their contents. Copying\nthe entire file gains nothing, and effectively requires a file to be\nwritten twice before making it into the object db.\n\nThis patch corrects that issue, holding onto the substr-like\nduplication for symlinks, but skipping it altogether for normal blobs\nby reusing the existing temp file.\n\nSigned-off-by: Marcus Griep \n---\n\nSorry for the second version. I was silly and didn't run the\n\"perl typo checker\". This is corrected and tested via \"full-svn-test\".\n\n git-svn.perl | 46 ++++++++++++++++++++++------------------------\n 1 files changed, 22 insertions(+), 24 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 9eae5e8..099fd02 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -3268,38 +3268,36 @@ sub close_file {\n \t\t\t\t \"expected: $exp\\n got: $got\\n\";\n \t\t\t}\n \t\t}\n-\t\tsysseek($fh, 0, 0) or croak $!;\n \t\tif ($fb->{mode_b} == 120000) {\n-\t\t\teval {\n-\t\t\t\tsysread($fh, my $buf, 5) == 5 or croak $!;\n-\t\t\t\t$buf eq 'link ' or die \"$path has mode 120000\",\n-\t\t\t\t\t\t \" but is not a link\";\n-\t\t\t};\n-\t\t\tif ($@) {\n-\t\t\t\twarn \"$@\\n\";\n-\t\t\t\tsysseek($fh, 0, 0) or croak $!;\n-\t\t\t}\n-\t\t}\n-\n-\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n-\t\tmy $result;\n-\t\twhile ($result = sysread($fh, my $string, 1024)) {\n-\t\t\tmy $wrote = syswrite($tmp_fh, $string, $result);\n-\t\t\tdefined($wrote) && $wrote == $result\n-\t\t\t\tor croak(\"write \",\n-\t\t\t\t\t$tmp_fh->filename, \": $!\\n\");\n-\t\t}\n-\t\tdefined $result or croak $!;\n+\t\t\tsysseek($fh, 0, 0) or croak $!;\n+\t\t\tsysread($fh, my $buf, 5) == 5 or croak $!;\n \n+\t\t\tunless ($buf eq 'link ') {\n+\t\t\t\twarn \"$path has mode 120000\",\n+\t\t\t\t\t\t\" but is not a link\\n\";\n+\t\t\t} else {\n+\t\t\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n+\t\t\t\tmy $res;\n+\t\t\t\twhile ($res = sysread($fh, my $str, 1024)) {\n+\t\t\t\t\tmy $out = syswrite($tmp_fh, $str, $res);\n+\t\t\t\t\tdefined($out) && $out == $res\n+\t\t\t\t\t\tor croak(\"write \",\n+\t\t\t\t\t\t\t$tmp_fh->filename,\n+\t\t\t\t\t\t\t\": $!\\n\");\n+\t\t\t\t}\n+\t\t\t\tdefined $res or croak $!;\n \n-\t\tGit::temp_release($fh, 1);\n+\t\t\t\t($fh, $tmp_fh) = ($tmp_fh, $fh);\n+\t\t\t\tGit::temp_release($tmp_fh, 1);\n+\t\t\t}\n+\t\t}\n \n \t\t$hash = $::_repository->hash_and_insert_object(\n-\t\t\t\t$tmp_fh->filename);\n+\t\t\t\t$fh->filename);\n \t\t$hash =~ /^[a-f\\d]{40}$/ or die \"not a sha1: $hash\\n\";\n \n \t\tGit::temp_release($fb->{base}, 1);\n-\t\tGit::temp_release($tmp_fh, 1);\n+\t\tGit::temp_release($fh, 1);\n \t} else {\n \t\t$hash = $fb->{blob} or die \"no blob information\\n\";\n \t}\n-- \n1.6.0.rc2.6.g8eda3\n"},{"id":"87004","messageId":"20080813032813.GA5904@untitled","threadId":"14900","inReplyTo":"1218556818-14006-1-git-send-email-marcus@griep.us","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-13T03:28:13Z","receivedAt":"2008-08-13T03:28:13Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Marcus Griep wrote:\n> This patch offers a generic interface to allow temp files to be\n> cached while using an instance of the 'Git' package. If many\n> temp files are created and destroyed during the execution of a\n> program, this caching mechanism can help reduce the amount of\n> files created and destroyed by the filesystem.\n> \n> The temp_acquire method provides a weak guarantee that a temp\n> file will not be stolen by subsequent requests. If a file is\n> locked when another acquire request is made, a simple error is\n> thrown.\n> \n> Signed-off-by: Marcus Griep \n\nAcked-by: Eric Wong \n\n> ---\n> perl/Git.pm | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 1 files changed, 123 insertions(+), 2 deletions(-)\n> \n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index e1ca5b4..405f68f 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -57,7 +57,8 @@ require Exporter;\n> command_output_pipe command_input_pipe command_close_pipe\n> command_bidi_pipe command_close_bidi_pipe\n> version exec_path hash_object git_cmd_try\n> - remote_refs);\n> + remote_refs\n> + temp_acquire temp_release temp_reset);\n> \n> \n> =head1 DESCRIPTION\n> @@ -99,7 +100,9 @@ use Carp qw(carp croak); # but croak is bad - throw instead\n> use Error qw(:try);\n> use Cwd qw(abs_path);\n> use IPC::Open2 qw(open2);\n> -\n> +use File::Temp ();\n> +require File::Spec;\n> +use Fcntl qw(SEEK_SET SEEK_CUR);\n> }\n> \n> \n> @@ -933,6 +936,124 @@ sub _close_cat_blob {\n> \tdelete @$self{@vars};\n> }\n> \n> +\n> +{ # %TEMP_* Lexical Context\n> +\n> +my (%TEMP_LOCKS, %TEMP_FILES);\n> +\n> +=item temp_acquire ( NAME )\n> +\n> +Attempts to retreive the temporary file mapped to the string C. If an\n> +associated temp file has not been created this session or was closed, it is\n> +created, cached, and set for autoflush and binmode.\n> +\n> +Internally locks the file mapped to C. This lock must be released with\n> +C when the temp file is no longer needed. Subsequent attempts\n> +to retrieve temporary files mapped to the same C while still locked will\n> +cause an error. This locking mechanism provides a weak guarantee and is not\n> +threadsafe. It does provide some error checking to help prevent temp file refs\n> +writing over one another.\n> +\n> +In general, the L returned should not be closed by consumers as\n> +it defeats the purpose of this caching mechanism. If you need to close the temp\n> +file handle, then you should use L or another temp file faculty\n> +directly. If a handle is closed and then requested again, then a warning will\n> +issue.\n> +\n> +=cut\n> +\n> +sub temp_acquire {\n> +\tmy ($self, $name) = _maybe_self(@_);\n> +\n> +\tmy $temp_fd = _temp_cache($name);\n> +\n> +\t$TEMP_LOCKS{$temp_fd} = 1;\n> +\t$temp_fd;\n> +}\n> +\n> +=item temp_release ( NAME )\n> +\n> +=item temp_release ( FILEHANDLE )\n> +\n> +Releases a lock acquired through C. Can be called either with\n> +the C mapping used when acquiring the temp file or with the C\n> +referencing a locked temp file.\n> +\n> +Warns if an attempt is made to release a file that is not locked.\n> +\n> +The temp file will be truncated before being released. This can help to reduce\n> +disk I/O where the system is smart enough to detect the truncation while data\n> +is in the output buffers. Beware that after the temp file is released and\n> +truncated, any operations on that file may fail miserably until it is\n> +re-acquired. All contents are lost between each release and acquire mapped to\n> +the same string.\n> +\n> +=cut\n> +\n> +sub temp_release {\n> +\tmy ($self, $temp_fd, $trunc) = _maybe_self(@_);\n> +\n> +\tif (ref($temp_fd) ne 'File::Temp') {\n> +\t\t$temp_fd = $TEMP_FILES{$temp_fd};\n> +\t}\n> +\tunless ($TEMP_LOCKS{$temp_fd}) {\n> +\t\tcarp \"Attempt to release temp file '\",\n> +\t\t\t$temp_fd, \"' that has not been locked\";\n> +\t}\n> +\ttemp_reset($temp_fd) if $trunc and $temp_fd->opened;\n> +\n> +\t$TEMP_LOCKS{$temp_fd} = 0;\n> +\tundef;\n> +}\n> +\n> +sub _temp_cache {\n> +\tmy ($name) = @_;\n> +\n> +\tmy $temp_fd = \\$TEMP_FILES{$name};\n> +\tif (defined $$temp_fd and $$temp_fd->opened) {\n> +\t\tif ($TEMP_LOCKS{$$temp_fd}) {\n> +\t\t\tthrow Error::Simple(\"Temp file with moniker '\",\n> +\t\t\t\t$name, \"' already in use\");\n> +\t\t}\n> +\t} else {\n> +\t\tif (defined $$temp_fd) {\n> +\t\t\t# then we're here because of a closed handle.\n> +\t\t\tcarp \"Temp file '\", $name,\n> +\t\t\t\t\"' was closed. Opening replacement.\";\n> +\t\t}\n> +\t\t$$temp_fd = File::Temp->new(\n> +\t\t\tTEMPLATE => 'Git_XXXXXX',\n> +\t\t\tDIR => File::Spec->tmpdir\n> +\t\t\t) or throw Error::Simple(\"couldn't open new temp file\");\n> +\t\t$$temp_fd->autoflush;\n> +\t\tbinmode $$temp_fd;\n> +\t}\n> +\t$$temp_fd;\n> +}\n> +\n> +=item temp_reset ( FILEHANDLE )\n> +\n> +Truncates and resets the position of the C.\n> +\n> +=cut\n> +\n> +sub temp_reset {\n> +\tmy ($self, $temp_fd) = _maybe_self(@_);\n> +\n> +\ttruncate $temp_fd, 0\n> +\t\tor throw Error::Simple(\"couldn't truncate file\");\n> +\tsysseek($temp_fd, 0, SEEK_SET) and seek($temp_fd, 0, SEEK_SET)\n> +\t\tor throw Error::Simple(\"couldn't seek to beginning of file\");\n> +\tsysseek($temp_fd, 0, SEEK_CUR) == 0 and tell($temp_fd) == 0\n> +\t\tor throw Error::Simple(\"expected file position to be reset\");\n> +}\n> +\n> +sub END {\n> +\tunlink values %TEMP_FILES if %TEMP_FILES;\n> +}\n> +\n> +} # %TEMP_* Lexical Context\n> +\n> =back\n> \n> =head1 ERROR HANDLING\n> -- \n> 1.6.0.rc2.6.g8eda3\n"},{"id":"87005","messageId":"20080813032900.GB5904@untitled","threadId":"14900","inReplyTo":"1218556853-25906-1-git-send-email-marcus@griep.us","subject":"Re: [PATCH 2/3] git-svn: Make it incrementally faster by minimizing temp files","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-13T03:29:00Z","receivedAt":"2008-08-13T03:29:00Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Marcus Griep wrote:\n> Currently, git-svn would create a temp file on four occasions:\n> 1. Reading a blob out of the object db\n> 2. Creating a delta from svn\n> 3. Hashing and writing a blob into the object db\n> 4. Reading a blob out of the object db (in another place in code)\n> \n> Any time git-svn did the above, it would dutifully create and then\n> delete said temp file. Unfortunately, this means that between 2-4\n> temporary files are created/deleted per file 'add/modify'-ed in\n> svn (O(n)). This causes significant overhead and helps the inode\n> counter to spin beautifully.\n> \n> By its nature, git-svn is a serial beast. Thus, reusing a temp file\n> does not pose significant problems. \"truncate and seek\" takes much\n> less time than \"unlink and create\". This patch centralizes the\n> tempfile creation and holds onto the tempfile until they are deleted\n> on exit. This significantly reduces file overhead, now requiring\n> at most three (3) temp files per run (O(1)).\n> \n> Signed-off-by: Marcus Griep \n\nAcked-by: Eric Wong \n\n> ---\n> git-svn.perl | 35 ++++++++++++++++++-----------------\n> 1 files changed, 18 insertions(+), 17 deletions(-)\n> \n> diff --git a/git-svn.perl b/git-svn.perl\n> index 4dc3380..9eae5e8 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -1265,7 +1265,7 @@ sub md5sum {\n> \tmy $arg = shift;\n> \tmy $ref = ref $arg;\n> \tmy $md5 = Digest::MD5->new();\n> - if ($ref eq 'GLOB' || $ref eq 'IO::File') {\n> + if ($ref eq 'GLOB' || $ref eq 'IO::File' || $ref eq 'File::Temp') {\n> \t\t$md5->addfile($arg) or croak $!;\n> \t} elsif ($ref eq 'SCALAR') {\n> \t\t$md5->add($$arg) or croak $!;\n> @@ -1328,6 +1328,7 @@ BEGIN {\n> \t}\n> }\n> \n> +\n> my (%LOCKFILES, %INDEX_FILES);\n> END {\n> \tunlink keys %LOCKFILES if %LOCKFILES;\n> @@ -3230,13 +3231,11 @@ sub change_file_prop {\n> \n> sub apply_textdelta {\n> \tmy ($self, $fb, $exp) = @_;\n> -\tmy $fh = IO::File->new_tmpfile;\n> -\t$fh->autoflush(1);\n> +\tmy $fh = Git::temp_acquire('svn_delta');\n> \t# $fh gets auto-closed() by SVN::TxDelta::apply(),\n> \t# (but $base does not,) so dup() it for reading in close_file\n> \topen my $dup, '<&', $fh or croak $!;\n> -\tmy $base = IO::File->new_tmpfile;\n> -\t$base->autoflush(1);\n> +\tmy $base = Git::temp_acquire('git_blob');\n> \tif ($fb->{blob}) {\n> \t\tprint $base 'link ' if ($fb->{mode_a} == 120000);\n> \t\tmy $size = $::_repository->cat_blob($fb->{blob}, $base);\n> @@ -3251,9 +3250,9 @@ sub apply_textdelta {\n> \t\t}\n> \t}\n> \tseek $base, 0, 0 or croak $!;\n> -\t$fb->{fh} = $dup;\n> +\t$fb->{fh} = $fh;\n> \t$fb->{base} = $base;\n> -\t[ SVN::TxDelta::apply($base, $fh, undef, $fb->{path}, $fb->{pool}) ];\n> +\t[ SVN::TxDelta::apply($base, $dup, undef, $fb->{path}, $fb->{pool}) ];\n> }\n> \n> sub close_file {\n> @@ -3282,22 +3281,25 @@ sub close_file {\n> \t\t\t}\n> \t\t}\n> \n> -\t\tmy ($tmp_fh, $tmp_filename) = File::Temp::tempfile(UNLINK => 1);\n> +\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n> \t\tmy $result;\n> \t\twhile ($result = sysread($fh, my $string, 1024)) {\n> \t\t\tmy $wrote = syswrite($tmp_fh, $string, $result);\n> \t\t\tdefined($wrote) && $wrote == $result\n> -\t\t\t\tor croak(\"write $tmp_filename: $!\\n\");\n> +\t\t\t\tor croak(\"write \",\n> +\t\t\t\t\t$tmp_fh->filename, \": $!\\n\");\n> \t\t}\n> \t\tdefined $result or croak $!;\n> -\t\tclose $tmp_fh or croak $!;\n> \n> -\t\tclose $fh or croak $!;\n> \n> -\t\t$hash = $::_repository->hash_and_insert_object($tmp_filename);\n> -\t\tunlink($tmp_filename);\n> +\t\tGit::temp_release($fh, 1);\n> +\n> +\t\t$hash = $::_repository->hash_and_insert_object(\n> +\t\t\t\t$tmp_fh->filename);\n> \t\t$hash =~ /^[a-f\\d]{40}$/ or die \"not a sha1: $hash\\n\";\n> -\t\tclose $fb->{base} or croak $!;\n> +\n> +\t\tGit::temp_release($fb->{base}, 1);\n> +\t\tGit::temp_release($tmp_fh, 1);\n> \t} else {\n> \t\t$hash = $fb->{blob} or die \"no blob information\\n\";\n> \t}\n> @@ -3667,7 +3669,7 @@ sub chg_file {\n> \t} elsif ($m->{mode_b} !~ /755$/ && $m->{mode_a} =~ /755$/) {\n> \t\t$self->change_file_prop($fbat,'svn:executable',undef);\n> \t}\n> -\tmy $fh = IO::File->new_tmpfile or croak $!;\n> +\tmy $fh = Git::temp_acquire('git_blob');\n> \tif ($m->{mode_b} =~ /^120/) {\n> \t\tprint $fh 'link ' or croak $!;\n> \t\t$self->change_file_prop($fbat,'svn:special','*');\n> @@ -3686,9 +3688,8 @@ sub chg_file {\n> \tmy $atd = $self->apply_textdelta($fbat, undef, $pool);\n> \tmy $got = SVN::TxDelta::send_stream($fh, @$atd, $pool);\n> \tdie \"Checksum mismatch\\nexpected: $exp\\ngot: $got\\n\" if ($got ne $exp);\n> +\tGit::temp_release($fh, 1);\n> \t$pool->clear;\n> -\n> -\tclose $fh or croak $!;\n> }\n> \n> sub D {\n> -- \n> 1.6.0.rc2.6.g8eda3\n"},{"id":"87006","messageId":"20080813032956.GC5904@untitled","threadId":"14900","inReplyTo":"1218556876-26554-1-git-send-email-marcus@griep.us","subject":"Re: [PATCH 3/3] git-svn: Reduce temp file usage when dealing with non-links","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-13T03:29:56Z","receivedAt":"2008-08-13T03:29:56Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Marcus Griep wrote:\n> Currently, in sub 'close_file', git-svn creates a temporary file and\n> copies the contents of the blob to be written into it. This is useful\n> for symlinks because svn stores symlinks in the form:\n> \n> link $FILE_PATH\n> \n> Git creates a blob only out of '$FILE_PATH' and uses file mode to\n> indicate that the blob should be interpreted as a symlink.\n> \n> As git-hash-object is invoked with --stdin-paths, a duplicate of the\n> link from svn must be created that leaves off the first five bytes,\n> i.e. 'link '. However, this is wholly unnecessary for normal blobs,\n> though, as we already have a temp file with their contents. Copying\n> the entire file gains nothing, and effectively requires a file to be\n> written twice before making it into the object db.\n> \n> This patch corrects that issue, holding onto the substr-like\n> duplication for symlinks, but skipping it altogether for normal blobs\n> by reusing the existing temp file.\n> \n> Signed-off-by: Marcus Griep \n\nThank you Marcus!\n\nAcked-by: Eric Wong \n\n> ---\n> git-svn.perl | 46 ++++++++++++++++++++++------------------------\n> 1 files changed, 22 insertions(+), 24 deletions(-)\n> \n> diff --git a/git-svn.perl b/git-svn.perl\n> index 9eae5e8..95d1510 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -3268,38 +3268,36 @@ sub close_file {\n> \t\t\t\t \"expected: $exp\\n got: $got\\n\";\n> \t\t\t}\n> \t\t}\n> -\t\tsysseek($fh, 0, 0) or croak $!;\n> \t\tif ($fb->{mode_b} == 120000) {\n> -\t\t\teval {\n> -\t\t\t\tsysread($fh, my $buf, 5) == 5 or croak $!;\n> -\t\t\t\t$buf eq 'link ' or die \"$path has mode 120000\",\n> -\t\t\t\t\t\t \" but is not a link\";\n> -\t\t\t};\n> -\t\t\tif ($@) {\n> -\t\t\t\twarn \"$@\\n\";\n> -\t\t\t\tsysseek($fh, 0, 0) or croak $!;\n> -\t\t\t}\n> -\t\t}\n> -\n> -\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n> -\t\tmy $result;\n> -\t\twhile ($result = sysread($fh, my $string, 1024)) {\n> -\t\t\tmy $wrote = syswrite($tmp_fh, $string, $result);\n> -\t\t\tdefined($wrote) && $wrote == $result\n> -\t\t\t\tor croak(\"write \",\n> -\t\t\t\t\t$tmp_fh->filename, \": $!\\n\");\n> -\t\t}\n> -\t\tdefined $result or croak $!;\n> +\t\t\tsysseek($fh, 0, 0) or croak $!;\n> +\t\t\tsysread($fh, my $buf, 5) == 5 or croak $!;\n> \n> +\t\t\tunless ($buf eq 'link ') {\n> +\t\t\t\twarn \"$path has mode 120000\",\n> +\t\t\t\t\t\t\" but is not a link\\n\";\n> +\t\t\t} else {\n> +\t\t\t\tmy $tmp_fh = Git::temp_acquire('svn_hash');\n> +\t\t\t\tmy $res;\n> +\t\t\t\twhile ($res = sysread($fh, my $str, 1024)) {\n> +\t\t\t\t\tmy $out = syswrite($tmp_fh, $str, $res);\n> +\t\t\t\t\tdefined($out) && $out == $res\n> +\t\t\t\t\t\tor croak(\"write \",\n> +\t\t\t\t\t\t\t$tmp_fh->filename,\n> +\t\t\t\t\t\t\t\": $!\\n\");\n> +\t\t\t\t}\n> +\t\t\t\tdefined $result or croak $!;\n> \n> -\t\tGit::temp_release($fh, 1);\n> +\t\t\t\t($fh, $tmp_fh) = ($tmp_fh, $fh);\n> +\t\t\t\tGit::temp_release($tmp_fh, 1);\n> +\t\t\t}\n> +\t\t}\n> \n> \t\t$hash = $::_repository->hash_and_insert_object(\n> -\t\t\t\t$tmp_fh->filename);\n> +\t\t\t\t$fh->filename);\n> \t\t$hash =~ /^[a-f\\d]{40}$/ or die \"not a sha1: $hash\\n\";\n> \n> \t\tGit::temp_release($fb->{base}, 1);\n> -\t\tGit::temp_release($tmp_fh, 1);\n> +\t\tGit::temp_release($fh, 1);\n> \t} else {\n> \t\t$hash = $fb->{blob} or die \"no blob information\\n\";\n> \t}\n> -- \n> 1.6.0.rc2.6.g8eda3\n"},{"id":"87008","messageId":"48A2583A.1000509@griep.us","threadId":"14900","inReplyTo":"20080813032956.GC5904@untitled","subject":"Re: [PATCH 3/3] git-svn: Reduce temp file usage when dealing with non-links","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-13T03:42:50Z","receivedAt":"2008-08-13T03:42:50Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Eric Wong wrote:\n> Thank you Marcus!\n> \n> Acked-by: Eric Wong \n\nErrr, you want to Ack [PATCH v2 3/3]; not this one; there's one minor\ntypo when I shortened the variable $result to $res.\n\n>> +\t\t\t\tmy $res;\n>> +\t\t\t\twhile ($res = sysread($fh, my $str, 1024)) {\n>> +\t\t\t\t\tmy $out = syswrite($tmp_fh, $str, $res);\n>> +\t\t\t\t\tdefined($out) && $out == $res\n>> +\t\t\t\t\t\tor croak(\"write \",\n>> +\t\t\t\t\t\t\t$tmp_fh->filename,\n>> +\t\t\t\t\t\t\t\": $!\\n\");\n>> +\t\t\t\t}\n>> +\t\t\t\tdefined $result or croak $!;\n\nThat last line causes compilation errors with 'use strict'. It is fixed\nin the alternate version.\n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"87009","messageId":"20080813035211.GA31792@untitled","threadId":"14900","inReplyTo":"48A2583A.1000509@griep.us","subject":"Re: [PATCH 3/3] git-svn: Reduce temp file usage when dealing with non-links","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-13T03:52:11Z","receivedAt":"2008-08-13T03:52:11Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Marcus Griep wrote:\n> Eric Wong wrote:\n> > Thank you Marcus!\n> > \n> > Acked-by: Eric Wong \n> \n> Errr, you want to Ack [PATCH v2 3/3]; not this one; there's one minor\n> typo when I shortened the variable $result to $res.\n> \n> >> +\t\t\t\tmy $res;\n> >> +\t\t\t\twhile ($res = sysread($fh, my $str, 1024)) {\n> >> +\t\t\t\t\tmy $out = syswrite($tmp_fh, $str, $res);\n> >> +\t\t\t\t\tdefined($out) && $out == $res\n> >> +\t\t\t\t\t\tor croak(\"write \",\n> >> +\t\t\t\t\t\t\t$tmp_fh->filename,\n> >> +\t\t\t\t\t\t\t\": $!\\n\");\n> >> +\t\t\t\t}\n> >> +\t\t\t\tdefined $result or croak $!;\n> \n> That last line causes compilation errors with 'use strict'. It is fixed\n> in the alternate version.\n\nOops, I applied the right patch but managed to reopen the wrong one when\nI acked it.\n\nI've just pushed out my repository with acks to\n git://bogomips.org/git-svn.git\n\n-- \nEric Wong\n"},{"id":"87092","messageId":"48A33E70.8060804@gmail.com","threadId":"14900","inReplyTo":"1218470035-13864-2-git-send-email-marcus@griep.us","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-08-13T20:05:04Z","receivedAt":"2008-08-13T20:05:04Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Marcus Griep wrote:\n> diff --git a/perl/Git.pm b/perl/Git.pm\n>\n> +require File::Spec;\n\nThis makes Git.pm dependent on Perl 5.6.1. Some tests (like\nt3701-add-interactive.sh) seem to work with pretty much any Perl version\nout there, and requiring File::Spec changes this. Hence to avoid\ncomplaints about failing tests, I suggest that you add a check for\nFile::Spec availability at the beginning of any test that (indirectly)\nuses Git.pm.\n\n(All my statements are untested... ;-))\n\n-- Lea\n"},{"id":"87095","messageId":"48A34068.9060509@griep.us","threadId":"14900","inReplyTo":"48A33E70.8060804@gmail.com","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-13T20:13:28Z","receivedAt":"2008-08-13T20:13:28Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Yeesh; didn't realize it would create that heavy of a dependency. Perhaps this should be split into a submodule so that Git.pm doesn't require the newer dependency. Eric/Junio?\n\nLea Wiemann wrote:\n> Marcus Griep wrote:\n>> diff --git a/perl/Git.pm b/perl/Git.pm\n>>\n>> +require File::Spec;\n> \n> This makes Git.pm dependent on Perl 5.6.1. Some tests (like\n> t3701-add-interactive.sh) seem to work with pretty much any Perl version\n> out there, and requiring File::Spec changes this. Hence to avoid\n> complaints about failing tests, I suggest that you add a check for\n> File::Spec availability at the beginning of any test that (indirectly)\n> uses Git.pm.\n> \n> (All my statements are untested... ;-))\n> \n> -- Lea\n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"87097","messageId":"48A34493.2000307@gmail.com","threadId":"14900","inReplyTo":"48A34068.9060509@griep.us","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Marcus Griep","fromEmail":"neoeinstein@gmail.com","sentAt":"2008-08-13T20:31:15Z","receivedAt":"2008-08-13T20:31:15Z","isPatch":true,"sender":{"key":"neoeinstein@gmail.com","avatar":"https://gravatar.com/avatar/75d467077b37e56699d408fb97545e9a92a2907ff1feea4ba3a4b861f7cb7af4?d=mp&s=160"},"body":"Hrmmm... From what I see in CPAN, File::Spec has been around \nperl since 1998 (around v5.4.7). Based on this is it safe-ish \nto assume availability of File::Spec?\n\nOr, as I said earlier, should we kick out a submodule for the tempfile\nfunctions?\n\nMarcus Griep wrote:\n> Yeesh; didn't realize it would create that heavy of a dependency.\n> Perhaps this should be split into a submodule so that Git.pm doesn't\n> require the newer dependency. Eric/Junio?\n>\n> Lea Wiemann wrote:\n>> This makes Git.pm dependent on Perl 5.6.1. Some tests (like\n>> t3701-add-interactive.sh) seem to work with pretty much any Perl version\n>> out there, and requiring File::Spec changes this. Hence to avoid\n>> complaints about failing tests, I suggest that you add a check for\n>> File::Spec availability at the beginning of any test that (indirectly)\n>> uses Git.pm.\n>>\n>> (All my statements are untested... ;-))\n>>\n>> -- Lea\n> \n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"87099","messageId":"7vskt8mz0g.fsf@gitster.siamese.dyndns.org","threadId":"14900","inReplyTo":"48A33E70.8060804@gmail.com","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-13T20:38:23Z","receivedAt":"2008-08-13T20:38:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann writes:\n\n> Marcus Griep wrote:\n>> diff --git a/perl/Git.pm b/perl/Git.pm\n>>\n>> +require File::Spec;\n>\n> This makes Git.pm dependent on Perl 5.6.1.\n\nOuch. Thanks for being extra careful.\n\nUnfortunately I've already pulled these changes via Eric.\n\nAmong the existing Perl scripts, cvsexportcommit and cvsimport already do\nuse it, so do svnimport and cidaemon in contrib.\n\n> ... Hence to avoid\n> complaints about failing tests, I suggest that you add a check for\n> File::Spec availability at the beginning of any test that (indirectly)\n> uses Git.pm.\n\nHmm, wouldn't something like this (untested) be more contained?\n\n---\n perl/Git.pm | 16 ++++++++++++++--\n 1 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 405f68f..2a92945 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -95,14 +95,26 @@ increase notwithstanding).\n \n =cut\n \n+my $tmpdir;\n \n use Carp qw(carp croak); # but croak is bad - throw instead\n use Error qw(:try);\n use Cwd qw(abs_path);\n use IPC::Open2 qw(open2);\n use File::Temp ();\n-require File::Spec;\n use Fcntl qw(SEEK_SET SEEK_CUR);\n+\n+\teval { require File::Spec; };\n+\tif ($@) {\n+\t\tfor (@ENV{qw(TMPDIR TEMP TMP)}, \"/tmp\") {\n+\t\t\tif (test -d $_) {\n+\t\t\t\t$tmpdir = $_;\n+\t\t\t\tlast;\n+\t\t\t}\n+\t\t}\n+\t} else {\n+\t\t$tmpdir = File::Spec->tmpdir;\n+\t}\n }\n \n \n@@ -1023,7 +1035,7 @@ sub _temp_cache {\n \t\t}\n \t\t$$temp_fd = File::Temp->new(\n \t\t\tTEMPLATE => 'Git_XXXXXX',\n-\t\t\tDIR => File::Spec->tmpdir\n+\t\t\tDIR => $tmpdir,\n \t\t\t) or throw Error::Simple(\"couldn't open new temp file\");\n \t\t$$temp_fd->autoflush;\n \t\tbinmode $$temp_fd;\n"},{"id":"87104","messageId":"20080813205233.GQ18960@genesis.frugalware.org","threadId":"14900","inReplyTo":"48A33E70.8060804@gmail.com","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-13T20:52:33Z","receivedAt":"2008-08-13T20:52:33Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Wed, Aug 13, 2008 at 10:05:04PM +0200, Lea Wiemann wrote:\n> This makes Git.pm dependent on Perl 5.6.1.\n\nHuh, am I right about it was released on 2001.04.09?\n(http://use.perl.org/article.pl?sid=01/04/09/123230) Sounds like\ndepending on it is really not a problem.\n"},{"id":"87118","messageId":"48A36002.1030705@gmail.com","threadId":"14900","inReplyTo":"7vskt8mz0g.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-08-13T22:28:18Z","receivedAt":"2008-08-13T22:28:18Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> Lea Wiemann writes:\n> \n>> Marcus Griep wrote:\n>>> +require File::Spec;\n>> This makes Git.pm dependent on Perl 5.6.1.\n\nOuch, I misquoted. It's File::Temp that was introduced in Perl 5.6.1,\nnot File::Spec. (I think it's probably save to assume that File::Spec\n[added in 5.4.5] is available everywhere.)\n\n> Hmm, wouldn't something like this (untested) be more contained?\n\nUh, sorry for making you write unnecessary code. Replicating File::Temp\nfunctionality is probably a bit too tricky because of temp-file safety,\nthough I haven't checked the code. It's probably not worth the effort\nanyway; I was really just concerned about not having the test suite fail\nin the 0.1% of cases where someone doesn't have Perl >5.6.1.\n\nAlso, adding \"use 5.006001\" may help with erroring out with a proper\nerror message for older perl versions. I'll send a follow-up to this\nmessage.\n\n-- Lea\n"},{"id":"87119","messageId":"1218666615-5152-1-git-send-email-LeWiemann@gmail.com","threadId":"14900","inReplyTo":"48A36002.1030705@gmail.com","subject":"[PATCH] Git.pm: require Perl 5.6.1","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-08-13T22:30:15Z","receivedAt":"2008-08-13T22:30:15Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"File::Temp is only in Perl 5.6.1, so Git.pm won't usually work with\nolder Perl versions; this gives a more descriptive error message\n('Perl version too old'), rather than erroring out with a 'File::Temp\nnot found' message. (File::Temp is probably not installable on older\nPerl version anyway.)\n\nSigned-off-by: Lea Wiemann \n---\n perl/Git.pm | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 405f68f..91c3fc6 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -96,6 +96,9 @@ increase notwithstanding).\n =cut\n \n \n+# File::Temp is only in core as of Perl 5.6.1.\n+use 5.006001;\n+\n use Carp qw(carp croak); # but croak is bad - throw instead\n use Error qw(:try);\n use Cwd qw(abs_path);\n-- \n1.5.6.3.539.g5ceae\n"},{"id":"87157","messageId":"7vwsikds99.fsf@gitster.siamese.dyndns.org","threadId":"14900","inReplyTo":"1218470035-13864-2-git-send-email-marcus@griep.us","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-14T06:29:06Z","receivedAt":"2008-08-14T06:29:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marcus Griep writes:\n\n> This patch offers a generic interface to allow temp files to be\n> cached while using an instance of the 'Git' package....\n\nBy the way, I think your commit title has a typo: s/cul/cili/. I've\nalready pulled this via Eric, so it will stay in the history forever,\nthough...\n"},{"id":"87161","messageId":"20080814065800.GA16918@untitled","threadId":"14900","inReplyTo":"7vskt8mz0g.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-08-14T06:58:00Z","receivedAt":"2008-08-14T06:58:00Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Lea Wiemann writes:\n> \n> > Marcus Griep wrote:\n> >> diff --git a/perl/Git.pm b/perl/Git.pm\n> >>\n> >> +require File::Spec;\n> >\n> > This makes Git.pm dependent on Perl 5.6.1.\n> \n> Ouch. Thanks for being extra careful.\n> \n> Unfortunately I've already pulled these changes via Eric.\n> \n> Among the existing Perl scripts, cvsexportcommit and cvsimport already do\n> use it, so do svnimport and cidaemon in contrib.\n> \n> > ... Hence to avoid\n> > complaints about failing tests, I suggest that you add a check for\n> > File::Spec availability at the beginning of any test that (indirectly)\n> > uses Git.pm.\n> \n> Hmm, wouldn't something like this (untested) be more contained?\n> \n> ---\n> perl/Git.pm | 16 ++++++++++++++--\n> 1 files changed, 14 insertions(+), 2 deletions(-)\n> \n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 405f68f..2a92945 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -1023,7 +1035,7 @@ sub _temp_cache {\n> \t\t}\n\nWhat about just lazy requiring inside _temp_cache() so it\nwon't get loaded by folks that don't need it? (completely untested):\n\n\t\teval { require File::Temp };\n\t\tif ($@) {\n\t\t\tthrow Error::Simple(\"couldn't require File::Temp: $@\");\n\t\t}\n\t\teval { require File::Spec };\n\t\tif ($@) {\n\t\t\tthrow Error::Simple(\"couldn't require File::Spec: $@\");\n\t\t}\n\nIt'll also remove the minor performance hit CGI/gitweb users got since\nwe won't load these extra modules during startup.\n\n> \t\t$$temp_fd = File::Temp->new(\n> \t\t\tTEMPLATE => 'Git_XXXXXX',\n> -\t\t\tDIR => File::Spec->tmpdir\n> +\t\t\tDIR => $tmpdir,\n> \t\t\t) or throw Error::Simple(\"couldn't open new temp file\");\n> \t\t$$temp_fd->autoflush;\n> \t\tbinmode $$temp_fd;\n\nFwiw, git-svn has been a File::Temp user for a few months since Adam's\ncat-file optimization; but it also has lower visibility since boxes\nwith <5.6.1 probably don't have SVN. git-svn previously used\nIO::File->new_tmpfile exclusively (and very heavily).\n\n-- \nEric Wong\n"},{"id":"87196","messageId":"48A442AA.8090505@griep.us","threadId":"14900","inReplyTo":"7vwsikds99.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-14T14:35:22Z","receivedAt":"2008-08-14T14:35:22Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> By the way, I think your commit title has a typo: s/cul/cili/. I've\n> already pulled this via Eric, so it will stay in the history forever,\n> though...\n\nActually, I meant faculties. While in common use faculty is\nusually meant as a body of teachers, the primary definition is\n\"An inherent power or ability\". Adding the caching of temp files gave\nGit.pm another power or ability, hence a new faculty.\n\nThe caching mechanism itself is also a facility that can be used by\nothers, and, of course, would also apply. :-P\n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"87317","messageId":"1218813032-18203-1-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":"20080814065800.GA16918@untitled","subject":"[PATCH] Git.pm: Make File::Spec and File::Temp requirement lazy","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-15T15:10:32Z","receivedAt":"2008-08-15T15:10:32Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"This will ensure that the API at large is accessible to nearly\nall Perl versions, while only the temp file caching API is tied to\nthe File::Temp and File::Spec modules being available.\n\nSigned-off-by: Marcus Griep \n---\n\n Eric Wong wrote:\n > What about just lazy requiring inside _temp_cache() so it\n > won't get loaded by folks that don't need it? (completely untested):\n > \n > \t\teval { require File::Temp };\n > \t\tif ($@) {\n > \t\t\tthrow Error::Simple(\"couldn't require File::Temp: $@\");\n > \t\t}\n > \t\teval { require File::Spec };\n > \t\tif ($@) {\n > \t\t\tthrow Error::Simple(\"couldn't require File::Spec: $@\");\n > \t\t}\n > \n > It'll also remove the minor performance hit CGI/gitweb users got since\n > we won't load these extra modules during startup.\n\n This recommendation is implemented with this patch, but in such a way that only\n the first test will be used, and that result cached. That way we aren't doing\n a compile _every_ time we want a temporary file, just the first time.\n\n perl/Git.pm | 14 ++++++++++++--\n 1 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 405f68f..9b6b637 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -100,8 +100,6 @@ use Carp qw(carp croak); # but croak is bad - throw instead\n use Error qw(:try);\n use Cwd qw(abs_path);\n use IPC::Open2 qw(open2);\n-use File::Temp ();\n-require File::Spec;\n use Fcntl qw(SEEK_SET SEEK_CUR);\n }\n \n@@ -940,6 +938,7 @@ sub _close_cat_blob {\n { # %TEMP_* Lexical Context\n \n my (%TEMP_LOCKS, %TEMP_FILES);\n+my $require_test;\n \n =item temp_acquire ( NAME )\n \n@@ -1009,6 +1008,8 @@ sub temp_release {\n sub _temp_cache {\n \tmy ($name) = @_;\n \n+\t_verify_require();\n+\n \tmy $temp_fd = \\$TEMP_FILES{$name};\n \tif (defined $$temp_fd and $$temp_fd->opened) {\n \t\tif ($TEMP_LOCKS{$$temp_fd}) {\n@@ -1031,6 +1032,15 @@ sub _temp_cache {\n \t$$temp_fd;\n }\n \n+sub _verify_require {\n+\tunless (defined $require_test) {\n+\t\t$require_test = \"\";\n+\t\teval { require File::Temp; require File::Spec; };\n+\t\t$require_test .= \"$@\";\n+\t}\n+\t$require_test and throw Error::Simple($require_test);\n+}\n+\n =item temp_reset ( FILEHANDLE )\n \n Truncates and resets the position of the C.\n-- \n1.6.0.rc2.6.g8eda3\n"},{"id":"87343","messageId":"3e8340490808151231p700ca76fub16700708f2942bb@mail.gmail.com","threadId":"14900","inReplyTo":"1218813032-18203-1-git-send-email-marcus@griep.us","subject":"Re: [PATCH] Git.pm: Make File::Spec and File::Temp requirement lazy","fromName":"Bryan Donlan","fromEmail":"bdonlan@gmail.com","sentAt":"2008-08-15T19:31:25Z","receivedAt":"2008-08-15T19:31:25Z","isPatch":true,"sender":{"key":"bdonlan@gmail.com","avatar":null},"body":"On Fri, Aug 15, 2008 at 11:10 AM, Marcus Griep wrote:\n> This will ensure that the API at large is accessible to nearly\n> all Perl versions, while only the temp file caching API is tied to\n> the File::Temp and File::Spec modules being available.\n>\n> Signed-off-by: Marcus Griep \n> ---\n>\n> Eric Wong wrote:\n> > What about just lazy requiring inside _temp_cache() so it\n> > won't get loaded by folks that don't need it? (completely untested):\n> >\n> > eval { require File::Temp };\n> > if ($@) {\n> > throw Error::Simple(\"couldn't require File::Temp: $@\");\n> > }\n> > eval { require File::Spec };\n> > if ($@) {\n> > throw Error::Simple(\"couldn't require File::Spec: $@\");\n> > }\n> >\n> > It'll also remove the minor performance hit CGI/gitweb users got since\n> > we won't load these extra modules during startup.\n>\n> This recommendation is implemented with this patch, but in such a way that only\n> the first test will be used, and that result cached. That way we aren't doing\n> a compile _every_ time we want a temporary file, just the first time.\n\nperl's 'require' will only attempt to load a module the first time it\nis used; subsequent attempts will result in a quick no-op, so there's\nno need to further cache. Unless you mean to cache a negative result?\nIn which case, the require should be quite fast, as it'll quickly get\na 'file not found' error without needing to do any compilation.\n"},{"id":"87346","messageId":"48A5DD0B.8050204@griep.us","threadId":"14900","inReplyTo":"3e8340490808151231p700ca76fub16700708f2942bb@mail.gmail.com","subject":"Re: [PATCH] Git.pm: Make File::Spec and File::Temp requirement lazy","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-15T19:46:19Z","receivedAt":"2008-08-15T19:46:19Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"Bryan Donlan wrote:\n> perl's 'require' will only attempt to load a module the first time it\n> is used; subsequent attempts will result in a quick no-op, so there's\n> no need to further cache. Unless you mean to cache a negative result?\n> In which case, the require should be quite fast, as it'll quickly get\n> a 'file not found' error without needing to do any compilation.\n\nAha. Point taken. I was under the mistaken impression that an entirely\nnew context was set up in an 'eval', but it is much more lightweight\nthan that. A simplified patch is forthcoming.\n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"87347","messageId":"1218830039-11567-1-git-send-email-marcus@griep.us","threadId":"14900","inReplyTo":"1218813032-18203-1-git-send-email-marcus@griep.us","subject":"[PATCH v2] Git.pm: Make File::Spec and File::Temp requirement lazy","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-15T19:53:59Z","receivedAt":"2008-08-15T19:53:59Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"This will ensure that the API at large is accessible to nearly\nall Perl versions, while only the temp file caching API is tied to\nthe File::Temp and File::Spec modules being available.\n\nSigned-off-by: Marcus Griep \n---\n\n Even shorter and sweeter now that I understand a bit more about Perl's\n exec functionality. This patch no longer has unnecessary caching and is\n about as short and sweet as it gets. Thanks for helping me learn more\n about Perl, Bryan.\n\n perl/Git.pm | 9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 405f68f..102e6a4 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -100,8 +100,6 @@ use Carp qw(carp croak); # but croak is bad - throw instead\n use Error qw(:try);\n use Cwd qw(abs_path);\n use IPC::Open2 qw(open2);\n-use File::Temp ();\n-require File::Spec;\n use Fcntl qw(SEEK_SET SEEK_CUR);\n }\n \n@@ -1009,6 +1007,8 @@ sub temp_release {\n sub _temp_cache {\n \tmy ($name) = @_;\n \n+\t_verify_require();\n+\n \tmy $temp_fd = \\$TEMP_FILES{$name};\n \tif (defined $$temp_fd and $$temp_fd->opened) {\n \t\tif ($TEMP_LOCKS{$$temp_fd}) {\n@@ -1031,6 +1031,11 @@ sub _temp_cache {\n \t$$temp_fd;\n }\n \n+sub _verify_require {\n+\teval { require File::Temp; require File::Spec; };\n+\t$@ and throw Error::Simple($@);\n+}\n+\n =item temp_reset ( FILEHANDLE )\n \n Truncates and resets the position of the C.\n-- \n1.6.0.rc3.10.g5a13c\n"}]}