Re: [PATCH 1/3] Git.pm: Add faculties to allow temp files to be cached

From: Eric Wong <normalperson@yhbt.net>
Date: Aug 14, 2008, 06:58 UTC
Message-ID: <20080814065800.GA16918@untitled>
In-Reply-To: <7vskt8mz0g.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote:

Show 33 quoted lines

> Lea Wiemann <lewiemann@gmail.com> writes:
> 
> > Marcus Griep wrote:
> >> diff --git a/perl/Git.pm b/perl/Git.pm
> >>
> >> +require File::Spec;
> >
> > This makes Git.pm dependent on Perl 5.6.1.
> 
> Ouch.  Thanks for being extra careful.
> 
> Unfortunately I've already pulled these changes via Eric.
> 
> Among the existing Perl scripts, cvsexportcommit and cvsimport already do
> use it, so do svnimport and cidaemon in contrib.
> 
> > ...  Hence to avoid
> > complaints about failing tests, I suggest that you add a check for
> > File::Spec availability at the beginning of any test that (indirectly)
> > uses Git.pm.
> 
> Hmm, wouldn't something like this (untested) be more contained?
> 
> ---
>  perl/Git.pm |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 405f68f..2a92945 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -1023,7 +1035,7 @@ sub _temp_cache {
>  		}

What about just lazy requiring inside _temp_cache() so it won't get loaded by folks that don't need it? (completely untested):

		eval { require File::Temp };
		if ($@) {
			throw Error::Simple("couldn't require File::Temp: $@");
		}
		eval { require File::Spec };
		if ($@) {
			throw Error::Simple("couldn't require File::Spec: $@");
		}

It'll also remove the minor performance hit CGI/gitweb users got since we won't load these extra modules during startup.

Show 7 quoted lines

>  		$$temp_fd = File::Temp->new(
>  			TEMPLATE => 'Git_XXXXXX',
> -			DIR => File::Spec->tmpdir
> +			DIR => $tmpdir,
>  			) or throw Error::Simple("couldn't open new temp file");
>  		$$temp_fd->autoflush;
>  		binmode $$temp_fd;

Fwiw, git-svn has been a File::Temp user for a few months since Adam's cat-file optimization; but it also has lower visibility since boxes with <5.6.1 probably don't have SVN. git-svn previously used IO::File->new_tmpfile exclusively (and very heavily).

-- 
Eric Wong