Git 1.6.0.1 breaks git-svn with perl 5.8.0

Git 1.6.0.1 breaks git-svn with perl 5.8.0

[Tom G. Christensen]

The problem is that Git.pm depends on the "new" method of File::Temp 
introduced in 0.14.
perl 5.8.0 only has File::Temp 0.13 while 0.14 was included with 5.8.1 
and later.
The breakage was introduced in this commit:
e41352b24e29eba43d00a3fd117befaef1d594bc

-tgc

mock-chroot> ./t9100-git-svn-basic.sh -i -v
Initialized empty Git repository in 
/builddir/build/BUILD/git-1.6.0.1/t/trash directory/.git/
* define NO_SVN_TESTS to skip git-svn tests
* expecting success:
         mkdir import &&
         cd import &&
         echo foo > foo &&
         ln -s foo foo.link
         mkdir -p dir/a/b/c/d/e &&
         echo "deep dir" > dir/a/b/c/d/e/file &&
         mkdir bar &&
         echo "zzz" > bar/zzz &&
         echo "#!/bin/sh" > exec.sh &&
         chmod +x exec.sh &&
         svn import -m "import for git-svn" . "$svnrepo" >/dev/null &&
         cd .. &&
         rm -rf import &&
         git-svn init "$svnrepo"
Parentheses missing around "my" list at 
/builddir/build/BUILD/git-1.6.0.1/t/../git-svn line 4387.
*   ok 1: initialize git-svn

* expecting success: git-svn fetch
Parentheses missing around "my" list at 
/builddir/build/BUILD/git-1.6.0.1/t/../git-svn line 4387.
Can't locate object method "new" via package "File::Temp" at 
/builddir/build/BUILD/git-1.6.0.1/t/../perl/blib/lib/Git.pm line 1024.

* FAIL 2: import an SVN revision into git
         git-svn fetch
mock-chroot> exit

Re: Git 1.6.0.1 breaks git-svn with perl 5.8.0

[Matthieu Moy]

"Tom G. Christensen" <tgc@statsbiblioteket.dk> writes:

> The problem is that Git.pm depends on the "new" method of File::Temp
> introduced in 0.14.
> perl 5.8.0 only has File::Temp 0.13 while 0.14 was included with 5.8.1
> and later.
> The breakage was introduced in this commit:
> e41352b24e29eba43d00a3fd117befaef1d594bc

Isn't that fixed by c14c8ceb13b299892f286757e22e6af4f6cffab5 ?
(Git.pm: Make File::Spec and File::Temp requirement lazy, a few
commits before 1.6.0)

-- 
Matthieu

[PATCH] Git.pm: Require File::Temp 0.14 for new()

[Abhijit Menon-Sen]

File::Temp->new() was introduced in File::Temp 0.14, but 5.8.0 shipped
with File::Temp 0.13, as pointed out by Tom G. Christensen. Since the
dependency is optional anyway, we can require 0.14.

Signed-off-by: Abhijit Menon-Sen <ams@toroid.org>
---

At 2008-09-01 11:46:17 +0200, Matthieu.Moy@imag.fr wrote:
>
> > The problem is that Git.pm depends on the "new" method of File::Temp
> > introduced in 0.14. [...]
> 
> Isn't that fixed by c14c8ceb13b299892f286757e22e6af4f6cffab5 ?
> (Git.pm: Make File::Spec and File::Temp requirement lazy, a few
> commits before 1.6.0)

No, because _verify_require only checks that File::Temp exists, not that
File::Temp 0.14 exists. This patch fixes that.

-- ams

 perl/Git.pm |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 102e6a4..4e901b6 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1032,7 +1032,10 @@ sub _temp_cache {
 }
 
 sub _verify_require {
-	eval { require File::Temp; require File::Spec; };
+	eval {
+            require File::Spec;
+            require File::Temp; File::Temp->VERSION(0.14);
+        };
 	$@ and throw Error::Simple($@);
 }
 
-- 
1.6.0.49.gea35

Re: [PATCH] Git.pm: Require File::Temp 0.14 for new()

[Tom G. Christensen]

Abhijit Menon-Sen wrote:
> File::Temp->new() was introduced in File::Temp 0.14, but 5.8.0 shipped
> with File::Temp 0.13, as pointed out by Tom G. Christensen. Since the
> dependency is optional anyway, we can require 0.14.
> 
IMHO not acceptable since RHEL3 users will not be able to use git-svn 
anymore.
Wouldn't it be possible to accomplish the same as File::Temp->New with 
the old File::Temp?

-tgc

[PATCH] Git.pm: Use File::Temp->tempfile instead of ->new

[Abhijit Menon-Sen]

Perl 5.8.0 ships with File::Temp 0.13, which does not have the new()
interface introduced in 0.14, as pointed out by Tom G. Christensen.

Signed-off-by: Abhijit Menon-Sen <ams@toroid.org>
---

At 2008-09-01 12:21:02 +0200, tgc@statsbiblioteket.dk wrote:
>
> Wouldn't it be possible to accomplish the same as File::Temp->New with
> the old File::Temp?

OK, does this one work for you?

-- ams

 perl/Git.pm |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 102e6a4..f383ff8 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1021,10 +1021,13 @@ sub _temp_cache {
 			carp "Temp file '", $name,
 				"' was closed. Opening replacement.";
 		}
-		$$temp_fd = File::Temp->new(
-			TEMPLATE => 'Git_XXXXXX',
-			DIR => File::Spec->tmpdir
-			) or throw Error::Simple("couldn't open new temp file");
+		eval {
+			($$temp_fd) = File::Temp->tempfile(
+				'Git_XXXXXX',
+				DIR => File::Spec->tmpdir
+			);
+		};
+		throw Error::Simple("couldn't open new temp file") if $@;
 		$$temp_fd->autoflush;
 		binmode $$temp_fd;
 	}
-- 
1.6.0.1.161.g7f314

Re: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new

[Tom G. Christensen]

Abhijit Menon-Sen wrote:
> Perl 5.8.0 ships with File::Temp 0.13, which does not have the new()
> interface introduced in 0.14, as pointed out by Tom G. Christensen.
> 
> Signed-off-by: Abhijit Menon-Sen <ams@toroid.org>
> ---
> 
> At 2008-09-01 12:21:02 +0200, tgc@statsbiblioteket.dk wrote:
>> Wouldn't it be possible to accomplish the same as File::Temp->New with
>> the old File::Temp?
> 
> OK, does this one work for you?
> 
Nope.
I applied it to 1.6.0.1 and now get this error instead:

Parentheses missing around "my" list at 
/builddir/build/BUILD/git-1.6.0.1/t/../git-svn line 4387.
Attempt to release temp file '' that has not been locked at 
/builddir/build/BUILD/git-1.6.0.1/t/../git-svn line 3294
Can't call method "opened" on an undefined value at 
/builddir/build/BUILD/git-1.6.0.1/t/../perl/blib/lib/Git.pm line 1001.

-tgc

Re: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new

[Junio C Hamano]

"Tom G. Christensen" <tgc@statsbiblioteket.dk> writes:

> Abhijit Menon-Sen wrote:
>> Perl 5.8.0 ships with File::Temp 0.13, which does not have the new()
>> interface introduced in 0.14, as pointed out by Tom G. Christensen.
>>
>> Signed-off-by: Abhijit Menon-Sen <ams@toroid.org>
>> ---
>>
>> At 2008-09-01 12:21:02 +0200, tgc@statsbiblioteket.dk wrote:
>>> Wouldn't it be possible to accomplish the same as File::Temp->New with
>>> the old File::Temp?
>>
>> OK, does this one work for you?
>>
> Nope.

Somebody involved in this thread care to report the current status please?

Re: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new

[Abhijit Menon-Sen]

At 2008-09-06 22:27:56 -0700, gitster@pobox.com wrote:
>
> Somebody involved in this thread care to report the current status
> please?

My patch is broken. I sent Tom an update, but he encountered an error
with that too. I'm trying to figure out what's wrong, and will post a
new patch to the list when I have it working.

-- ams

Re: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new

[Marcus Griep]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

Abhijit Menon-Sen wrote:
> My patch is broken. I sent Tom an update, but he encountered an error
> with that too. I'm trying to figure out what's wrong, and will post a
> new patch to the list when I have it working.

As I was the one who brought File::Temp->new over from git-svn, I can
take a look at reducing the dependency to ->tempfile.  If you don't patch
in first, I'll get one in as well.

-- 
Marcus Griep
GPG Key ID: 0x5E968152
——
http://www.boohaunt.net
את.ψο´


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new

[Abhijit Menon-Sen]

At 2008-09-07 12:50:56 -0400, neoeinstein@gmail.com wrote:
>
> As I was the one who brought File::Temp->new over from git-svn, I can
> take a look at reducing the dependency to ->tempfile.  If you don't
> patch in first, I'll get one in as well.

Please do.

Here's my latest patch, in case you find it useful as a starting point.
I do not have more time or inclination to work on this at the moment.
The problem with this patch is that the caller (git-svn in this case)
assumes that a File::Temp object will be returned, and depends on the
OO interface (e.g. by calling $tmp_fh->filename).

Unfortunately, I do not think the problem is limited to filename(). I
blessed $$temp_fd into a stub package that provided a fake filename()
method, but t9100-git-svn-basic.sh failed a little further on, and I
did not investigate further. Anyway, I don't think emulating the
File::Temp OO interface is a good solution at all.

(Tom: I'm sorry I ran out of energy before I could put together a
complete fix for you.)

-- ams

From: Abhijit Menon-Sen <ams@toroid.org>
Date: Mon, 8 Sep 2008 12:26:59 +0530
Subject: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new

Perl 5.8.0 ships with File::Temp 0.13, which does not have the new()
interface introduced in 0.14, as pointed out by Tom G. Christensen.

Signed-off-by: Abhijit Menon-Sen <ams@toroid.org>
---
 perl/Git.pm |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 102e6a4..b0498ca 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -991,7 +991,7 @@ the same string.
 sub temp_release {
 	my ($self, $temp_fd, $trunc) = _maybe_self(@_);
 
-	if (ref($temp_fd) ne 'File::Temp') {
+	if (exists $TEMP_FILES{$temp_fd}) {
 		$temp_fd = $TEMP_FILES{$temp_fd};
 	}
 	unless ($TEMP_LOCKS{$temp_fd}) {
@@ -1021,10 +1021,13 @@ sub _temp_cache {
 			carp "Temp file '", $name,
 				"' was closed. Opening replacement.";
 		}
-		$$temp_fd = File::Temp->new(
-			TEMPLATE => 'Git_XXXXXX',
-			DIR => File::Spec->tmpdir
-			) or throw Error::Simple("couldn't open new temp file");
+		eval {
+			($$temp_fd) = File::Temp->tempfile(
+				'Git_XXXXXX',
+				DIR => File::Spec->tmpdir
+			);
+		};
+		throw Error::Simple("couldn't open new temp file") if $@;
 		$$temp_fd->autoflush;
 		binmode $$temp_fd;
 	}
-- 
1.6.0.1.196.g01914

[PATCH] Git.pm: Use File::Temp->tempfile instead of ->new

[Marcus Griep]

Perl 5.8.0 ships with File::Temp 0.13, which does not have the new()
interface introduced in 0.14, as pointed out by Tom G. Christensen.

This modifies Git.pm to use the more established tempfile() interface
and updates 'git svn' to match.

Signed-off-by: Marcus Griep <marcus@griep.us>
---

 Per the earlier patch versions by Abhijit Menon-Sen and Tom G. Christensen.
 Both of you may want to run a test and add your 'Tested-by' to the thread
 if everything works out before Eric Wong adds his 'Acked-by'.

 git-svn.perl |    4 ++--
 perl/Git.pm  |   29 +++++++++++++++++++++++------
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index ee3f5ed..a6d75db 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3304,7 +3304,7 @@ sub close_file {
 					my $out = syswrite($tmp_fh, $str, $res);
 					defined($out) && $out == $res
 						or croak("write ",
-							$tmp_fh->filename,
+							Git::temp_filename($tmp_fh),
 							": $!\n");
 				}
 				defined $res or croak $!;
@@ -3315,7 +3315,7 @@ sub close_file {
 		}
 
 		$hash = $::_repository->hash_and_insert_object(
-				$fh->filename);
+				Git::temp_filename($fh));
 		$hash =~ /^[a-f\d]{40}$/ or die "not a sha1: $hash\n";
 
 		Git::temp_release($fb->{base}, 1);
diff --git a/perl/Git.pm b/perl/Git.pm
index 102e6a4..3ea3fb2 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -937,7 +937,7 @@ sub _close_cat_blob {
 
 { # %TEMP_* Lexical Context
 
-my (%TEMP_LOCKS, %TEMP_FILES);
+my (%TEMP_LOCKS, %TEMP_FILES, %TEMP_FILENAMES);
 
 =item temp_acquire ( NAME )
 
@@ -991,7 +991,7 @@ the same string.
 sub temp_release {
 	my ($self, $temp_fd, $trunc) = _maybe_self(@_);
 
-	if (ref($temp_fd) ne 'File::Temp') {
+	if (exists $TEMP_FILES{$temp_fd}) {
 		$temp_fd = $TEMP_FILES{$temp_fd};
 	}
 	unless ($TEMP_LOCKS{$temp_fd}) {
@@ -1021,12 +1021,12 @@ sub _temp_cache {
 			carp "Temp file '", $name,
 				"' was closed. Opening replacement.";
 		}
-		$$temp_fd = File::Temp->new(
-			TEMPLATE => 'Git_XXXXXX',
-			DIR => File::Spec->tmpdir
-			) or throw Error::Simple("couldn't open new temp file");
+		my $fname;
+		($$temp_fd, $fname) = File::Temp->tempfile('Git_XXXXXX')
+			or throw Error::Simple("couldn't open new temp file");
 		$$temp_fd->autoflush;
 		binmode $$temp_fd;
+		$TEMP_FILENAMES{$$temp_fd} = $fname;
 	}
 	$$temp_fd;
 }
@@ -1053,6 +1053,23 @@ sub temp_reset {
 		or throw Error::Simple("expected file position to be reset");
 }
 
+=item temp_filename ( NAME )
+
+=item temp_filename ( FILEHANDLE )
+
+Returns the filenae associated with the given tempfile.
+
+=cut
+
+sub temp_filename {
+	my ($self, $temp_fd) = _maybe_self(@_);
+
+	if (exists $TEMP_FILES{$temp_fd}) {
+		$temp_fd = $TEMP_FILES{$temp_fd};
+	}
+	$TEMP_FILENAMES{$temp_fd};
+}
+
 sub END {
 	unlink values %TEMP_FILES if %TEMP_FILES;
 }
-- 
1.6.0.1.400.gd2470

[PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new

[Marcus Griep]

Perl 5.8.0 ships with File::Temp 0.13, which does not have the new()
interface introduced in 0.14, as pointed out by Tom G. Christensen.

This modifies Git.pm to use the more established tempfile() interface
and updates 'git svn' to match.

Signed-off-by: Marcus Griep <marcus@griep.us>
---

 This patch v2 cleans up a few code items, corrects a misspelling,
 and ensures that the temp file gets unlinked when we exit, now
 that we are requesting the filename.  Otherwise, the previous
 comments stand:
 
 Per the earlier patch versions by Abhijit Menon-Sen and Tom G. Christensen.
 Both of you may want to run a test and add your 'Tested-by' to the thread
 if everything works out before Eric Wong adds his 'Acked-by'.

 git-svn.perl |    4 ++--
 perl/Git.pm  |   42 ++++++++++++++++++++++++++++++------------
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index ee3f5ed..c92bd8e 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3304,7 +3304,7 @@ sub close_file {
 					my $out = syswrite($tmp_fh, $str, $res);
 					defined($out) && $out == $res
 						or croak("write ",
-							$tmp_fh->filename,
+							Git::temp_fname($tmp_fh),
 							": $!\n");
 				}
 				defined $res or croak $!;
@@ -3315,7 +3315,7 @@ sub close_file {
 		}
 
 		$hash = $::_repository->hash_and_insert_object(
-				$fh->filename);
+				Git::temp_fname($fh));
 		$hash =~ /^[a-f\d]{40}$/ or die "not a sha1: $hash\n";
 
 		Git::temp_release($fb->{base}, 1);
diff --git a/perl/Git.pm b/perl/Git.pm
index 102e6a4..3f5514c 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -58,7 +58,7 @@ require Exporter;
                 command_bidi_pipe command_close_bidi_pipe
                 version exec_path hash_object git_cmd_try
                 remote_refs
-                temp_acquire temp_release temp_reset);
+                temp_acquire temp_release temp_reset temp_fname);
 
 
 =head1 DESCRIPTION
@@ -937,7 +937,7 @@ sub _close_cat_blob {
 
 { # %TEMP_* Lexical Context
 
-my (%TEMP_LOCKS, %TEMP_FILES);
+my (%TEMP_FILEMAP, %TEMP_FILES);
 
 =item temp_acquire ( NAME )
 
@@ -965,7 +965,7 @@ sub temp_acquire {
 
 	my $temp_fd = _temp_cache($name);
 
-	$TEMP_LOCKS{$temp_fd} = 1;
+	$TEMP_FILES{$temp_fd}{locked} = 1;
 	$temp_fd;
 }
 
@@ -991,16 +991,16 @@ the same string.
 sub temp_release {
 	my ($self, $temp_fd, $trunc) = _maybe_self(@_);
 
-	if (ref($temp_fd) ne 'File::Temp') {
+	if (exists $TEMP_FILEMAP{$temp_fd}) {
 		$temp_fd = $TEMP_FILES{$temp_fd};
 	}
-	unless ($TEMP_LOCKS{$temp_fd}) {
+	unless ($TEMP_FILES{$temp_fd}{locked}) {
 		carp "Attempt to release temp file '",
 			$temp_fd, "' that has not been locked";
 	}
 	temp_reset($temp_fd) if $trunc and $temp_fd->opened;
 
-	$TEMP_LOCKS{$temp_fd} = 0;
+	$TEMP_FILES{$temp_fd}{locked} = 0;
 	undef;
 }
 
@@ -1009,9 +1009,9 @@ sub _temp_cache {
 
 	_verify_require();
 
-	my $temp_fd = \$TEMP_FILES{$name};
+	my $temp_fd = \$TEMP_FILEMAP{$name};
 	if (defined $$temp_fd and $$temp_fd->opened) {
-		if ($TEMP_LOCKS{$$temp_fd}) {
+		if ($TEMP_FILES{$$temp_fd}{locked}) {
 			throw Error::Simple("Temp file with moniker '",
 				$name, "' already in use");
 		}
@@ -1021,12 +1021,13 @@ sub _temp_cache {
 			carp "Temp file '", $name,
 				"' was closed. Opening replacement.";
 		}
-		$$temp_fd = File::Temp->new(
-			TEMPLATE => 'Git_XXXXXX',
-			DIR => File::Spec->tmpdir
+		my $fname;
+		($$temp_fd, $fname) = File::Temp->tempfile(
+			'Git_XXXXXX', UNLINK => 1
 			) or throw Error::Simple("couldn't open new temp file");
 		$$temp_fd->autoflush;
 		binmode $$temp_fd;
+		$TEMP_FILES{$$temp_fd}{fname} = $fname;
 	}
 	$$temp_fd;
 }
@@ -1053,8 +1054,25 @@ sub temp_reset {
 		or throw Error::Simple("expected file position to be reset");
 }
 
+=item temp_fname ( NAME )
+
+=item temp_fname ( FILEHANDLE )
+
+Returns the filename associated with the given tempfile.
+
+=cut
+
+sub temp_fname {
+	my ($self, $temp_fd) = _maybe_self(@_);
+
+	if (exists $TEMP_FILEMAP{$temp_fd}) {
+		$temp_fd = $TEMP_FILEMAP{$temp_fd};
+	}
+	$TEMP_FILES{$temp_fd}{fname};
+}
+
 sub END {
-	unlink values %TEMP_FILES if %TEMP_FILES;
+	unlink values %TEMP_FILEMAP if %TEMP_FILEMAP;
 }
 
 } # %TEMP_* Lexical Context
-- 
1.6.0.1.400.gd2470

Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new

[Eric Wong]

Marcus Griep <marcus@griep.us> wrote:
> Perl 5.8.0 ships with File::Temp 0.13, which does not have the new()
> interface introduced in 0.14, as pointed out by Tom G. Christensen.
> 
> This modifies Git.pm to use the more established tempfile() interface
> and updates 'git svn' to match.
> 
> Signed-off-by: Marcus Griep <marcus@griep.us>
> ---
> 
>  This patch v2 cleans up a few code items, corrects a misspelling,
>  and ensures that the temp file gets unlinked when we exit, now
>  that we are requesting the filename.  Otherwise, the previous
>  comments stand:
>  
>  Per the earlier patch versions by Abhijit Menon-Sen and Tom G. Christensen.
>  Both of you may want to run a test and add your 'Tested-by' to the thread
>  if everything works out before Eric Wong adds his 'Acked-by'.

Thanks Marcus, this works for me.
(Perl 5.10.0, so no compatibility issues).

<bikeshed>
Can we rename temp_fname() to temp_path(), though?  "fname"
just doesn't look right in the API to me...
</bikeshed>

>  git-svn.perl |    4 ++--
>  perl/Git.pm  |   42 ++++++++++++++++++++++++++++++------------
>  2 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index ee3f5ed..c92bd8e 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -3304,7 +3304,7 @@ sub close_file {
>  					my $out = syswrite($tmp_fh, $str, $res);
>  					defined($out) && $out == $res
>  						or croak("write ",
> -							$tmp_fh->filename,
> +							Git::temp_fname($tmp_fh),
>  							": $!\n");
>  				}
>  				defined $res or croak $!;
> @@ -3315,7 +3315,7 @@ sub close_file {
>  		}
>  
>  		$hash = $::_repository->hash_and_insert_object(
> -				$fh->filename);
> +				Git::temp_fname($fh));
>  		$hash =~ /^[a-f\d]{40}$/ or die "not a sha1: $hash\n";
>  
>  		Git::temp_release($fb->{base}, 1);
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 102e6a4..3f5514c 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -58,7 +58,7 @@ require Exporter;
>                  command_bidi_pipe command_close_bidi_pipe
>                  version exec_path hash_object git_cmd_try
>                  remote_refs
> -                temp_acquire temp_release temp_reset);
> +                temp_acquire temp_release temp_reset temp_fname);
>  
>  
>  =head1 DESCRIPTION
> @@ -937,7 +937,7 @@ sub _close_cat_blob {
>  
>  { # %TEMP_* Lexical Context
>  
> -my (%TEMP_LOCKS, %TEMP_FILES);
> +my (%TEMP_FILEMAP, %TEMP_FILES);
>  
>  =item temp_acquire ( NAME )
>  
> @@ -965,7 +965,7 @@ sub temp_acquire {
>  
>  	my $temp_fd = _temp_cache($name);
>  
> -	$TEMP_LOCKS{$temp_fd} = 1;
> +	$TEMP_FILES{$temp_fd}{locked} = 1;
>  	$temp_fd;
>  }
>  
> @@ -991,16 +991,16 @@ the same string.
>  sub temp_release {
>  	my ($self, $temp_fd, $trunc) = _maybe_self(@_);
>  
> -	if (ref($temp_fd) ne 'File::Temp') {
> +	if (exists $TEMP_FILEMAP{$temp_fd}) {
>  		$temp_fd = $TEMP_FILES{$temp_fd};
>  	}
> -	unless ($TEMP_LOCKS{$temp_fd}) {
> +	unless ($TEMP_FILES{$temp_fd}{locked}) {
>  		carp "Attempt to release temp file '",
>  			$temp_fd, "' that has not been locked";
>  	}
>  	temp_reset($temp_fd) if $trunc and $temp_fd->opened;
>  
> -	$TEMP_LOCKS{$temp_fd} = 0;
> +	$TEMP_FILES{$temp_fd}{locked} = 0;
>  	undef;
>  }
>  
> @@ -1009,9 +1009,9 @@ sub _temp_cache {
>  
>  	_verify_require();
>  
> -	my $temp_fd = \$TEMP_FILES{$name};
> +	my $temp_fd = \$TEMP_FILEMAP{$name};
>  	if (defined $$temp_fd and $$temp_fd->opened) {
> -		if ($TEMP_LOCKS{$$temp_fd}) {
> +		if ($TEMP_FILES{$$temp_fd}{locked}) {
>  			throw Error::Simple("Temp file with moniker '",
>  				$name, "' already in use");
>  		}
> @@ -1021,12 +1021,13 @@ sub _temp_cache {
>  			carp "Temp file '", $name,
>  				"' was closed. Opening replacement.";
>  		}
> -		$$temp_fd = File::Temp->new(
> -			TEMPLATE => 'Git_XXXXXX',
> -			DIR => File::Spec->tmpdir
> +		my $fname;
> +		($$temp_fd, $fname) = File::Temp->tempfile(
> +			'Git_XXXXXX', UNLINK => 1
>  			) or throw Error::Simple("couldn't open new temp file");
>  		$$temp_fd->autoflush;
>  		binmode $$temp_fd;
> +		$TEMP_FILES{$$temp_fd}{fname} = $fname;
>  	}
>  	$$temp_fd;
>  }
> @@ -1053,8 +1054,25 @@ sub temp_reset {
>  		or throw Error::Simple("expected file position to be reset");
>  }
>  
> +=item temp_fname ( NAME )
> +
> +=item temp_fname ( FILEHANDLE )
> +
> +Returns the filename associated with the given tempfile.
> +
> +=cut
> +
> +sub temp_fname {
> +	my ($self, $temp_fd) = _maybe_self(@_);
> +
> +	if (exists $TEMP_FILEMAP{$temp_fd}) {
> +		$temp_fd = $TEMP_FILEMAP{$temp_fd};
> +	}
> +	$TEMP_FILES{$temp_fd}{fname};
> +}
> +
>  sub END {
> -	unlink values %TEMP_FILES if %TEMP_FILES;
> +	unlink values %TEMP_FILEMAP if %TEMP_FILEMAP;
>  }
>  
>  } # %TEMP_* Lexical Context
> -- 
> 1.6.0.1.400.gd2470

Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Marcus Griep <marcus@griep.us> wrote:
>> Perl 5.8.0 ships with File::Temp 0.13, which does not have the new()
>> interface introduced in 0.14, as pointed out by Tom G. Christensen.
>> 
>> This modifies Git.pm to use the more established tempfile() interface
>> and updates 'git svn' to match.
>> 
>> Signed-off-by: Marcus Griep <marcus@griep.us>
>> ---
>> 
>>  This patch v2 cleans up a few code items, corrects a misspelling,
>>  and ensures that the temp file gets unlinked when we exit, now
>>  that we are requesting the filename.  Otherwise, the previous
>>  comments stand:
>>  
>>  Per the earlier patch versions by Abhijit Menon-Sen and Tom G. Christensen.
>>  Both of you may want to run a test and add your 'Tested-by' to the thread
>>  if everything works out before Eric Wong adds his 'Acked-by'.
>
> Thanks Marcus, this works for me.
> (Perl 5.10.0, so no compatibility issues).
>
> <bikeshed>
> Can we rename temp_fname() to temp_path(), though?  "fname"
> just doesn't look right in the API to me...
> </bikeshed>

Ok, so will you take the patch with bikeshed fixes and feed the result to
me with a pull-request, or shall I do that myself?  I do not mind doing
this either way --- just trying to avoid duplicated work.

This is a 'maint' material, right?

Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <normalperson@yhbt.net> writes:
> 
> > Marcus Griep <marcus@griep.us> wrote:
> >> Perl 5.8.0 ships with File::Temp 0.13, which does not have the new()
> >> interface introduced in 0.14, as pointed out by Tom G. Christensen.
> >> 
> >> This modifies Git.pm to use the more established tempfile() interface
> >> and updates 'git svn' to match.
> >> 
> >> Signed-off-by: Marcus Griep <marcus@griep.us>
> >> ---
> >> 
> >>  This patch v2 cleans up a few code items, corrects a misspelling,
> >>  and ensures that the temp file gets unlinked when we exit, now
> >>  that we are requesting the filename.  Otherwise, the previous
> >>  comments stand:
> >>  
> >>  Per the earlier patch versions by Abhijit Menon-Sen and Tom G. Christensen.
> >>  Both of you may want to run a test and add your 'Tested-by' to the thread
> >>  if everything works out before Eric Wong adds his 'Acked-by'.
> >
> > Thanks Marcus, this works for me.
> > (Perl 5.10.0, so no compatibility issues).
> >
> > <bikeshed>
> > Can we rename temp_fname() to temp_path(), though?  "fname"
> > just doesn't look right in the API to me...
> > </bikeshed>
> 
> Ok, so will you take the patch with bikeshed fixes and feed the result to
> me with a pull-request, or shall I do that myself?  I do not mind doing
> this either way --- just trying to avoid duplicated work.

Please do it for me, thanks.  I've been preoccuppied offline and need
sleep.

> This is a 'maint' material, right?

Yes, definitely.

-- 
Eric Wong

Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new

[Abhijit Menon-Sen]

At 2008-09-08 12:53:01 -0400, marcus@griep.us wrote:
>
> +		my $fname;
> +		($$temp_fd, $fname) = File::Temp->tempfile(
> +			'Git_XXXXXX', UNLINK => 1
>  			) or throw Error::Simple("couldn't open new temp file");

What happens when tempfile croaks on error?

-- ams

Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new

[Marcus Griep]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Abhijit Menon-Sen wrote:
> At 2008-09-08 12:53:01 -0400, marcus@griep.us wrote:
>> +		my $fname;
>> +		($$temp_fd, $fname) = File::Temp->tempfile(
>> +			'Git_XXXXXX', UNLINK => 1
>>  			) or throw Error::Simple("couldn't open new temp file");
> 
> What happens when tempfile croaks on error?

I was indifferent to the idea of catching the ->tempfile croak just to hide it in our
own error (since the ->tempfile croak would have more information), in which case the
extra 'or throw' may be superfluous, but I kept it as a defensive programming practice.

-- 
Marcus Griep
GPG Key ID: 0x5E968152
——
http://www.boohaunt.net
את.ψο´


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new

[Tom G. Christensen]

Marcus Griep wrote:
> Perl 5.8.0 ships with File::Temp 0.13, which does not have the new()
> interface introduced in 0.14, as pointed out by Tom G. Christensen.
> 
> This modifies Git.pm to use the more established tempfile() interface
> and updates 'git svn' to match.
> 
> Signed-off-by: Marcus Griep <marcus@griep.us>
> ---
> 
>  This patch v2 cleans up a few code items, corrects a misspelling,
>  and ensures that the temp file gets unlinked when we exit, now
>  that we are requesting the filename.  Otherwise, the previous
>  comments stand:
> 
>  Per the earlier patch versions by Abhijit Menon-Sen and Tom G. Christensen.
>  Both of you may want to run a test and add your 'Tested-by' to the thread
>  if everything works out before Eric Wong adds his 'Acked-by'.
> 
The testsuite now passes t9100-git-svn-basic.sh and instead fails at 
t9108-git-svn-glob.sh but this appears to be unrelated to the File::Temp 
issue.

--- expect.three        2008-09-09 07:34:39.000000000 +0000
+++ stderr.three        2008-09-09 07:34:42.000000000 +0000
@@ -1,2 +1,3 @@
+Parentheses missing around "my" list at 
/home/tgc/projects/git/t/../git-svn line 4429.
  Only one set of wildcard directories (e.g. '*' or '*/*/*') is 
supported: 'branches/*/t/*'

* FAIL 3: test disallow multi-globs

Tested-by: Tom G. Christensen <tgc@statsbiblioteket.dk>

-tgc

Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new

[Marcus Griep]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

Tom G. Christensen wrote:
> The testsuite now passes t9100-git-svn-basic.sh and instead fails at
> t9108-git-svn-glob.sh but this appears to be unrelated to the File::Temp
> issue.
> 
> --- expect.three        2008-09-09 07:34:39.000000000 +0000
> +++ stderr.three        2008-09-09 07:34:42.000000000 +0000
> @@ -1,2 +1,3 @@
> +Parentheses missing around "my" list at
> /home/tgc/projects/git/t/../git-svn line 4429.
>  Only one set of wildcard directories (e.g. '*' or '*/*/*') is
> supported: 'branches/*/t/*'
> 
> * FAIL 3: test disallow multi-globs

It's probably another back-portability issue (It's not failing in my testsuite).
However, I think that I know the offending line.  Can you give me the text of
your line 4429 in git-svn and I'll submit a separate patch to fix that back
portability issue.

-- 
Marcus Griep
GPG Key ID: 0x5E968152
——
http://www.boohaunt.net
את.ψο´


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new

[Tom G. Christensen]

Marcus Griep wrote:
> Tom G. Christensen wrote:
>> The testsuite now passes t9100-git-svn-basic.sh and instead fails at
>> t9108-git-svn-glob.sh but this appears to be unrelated to the File::Temp
>> issue.
>>
>> --- expect.three        2008-09-09 07:34:39.000000000 +0000
>> +++ stderr.three        2008-09-09 07:34:42.000000000 +0000
>> @@ -1,2 +1,3 @@
>> +Parentheses missing around "my" list at
>> /home/tgc/projects/git/t/../git-svn line 4429.
>>  Only one set of wildcard directories (e.g. '*' or '*/*/*') is
>> supported: 'branches/*/t/*'
>>
>> * FAIL 3: test disallow multi-globs
> 
> It's probably another back-portability issue (It's not failing in my testsuite).
> However, I think that I know the offending line.  Can you give me the text of
> your line 4429 in git-svn and I'll submit a separate patch to fix that back
> portability issue.
> 
git-svn:4429:
         pipe my $rfd, my $wfd or return;

I know next to nothing about perl but I put () around my, my and then 
the test passes.

However it now fails in t9700/test.pl:
t9700 $ ./test.pl -i -v
ok 1 - use Git;
Bareword "STDERR" not allowed while "strict subs" in use at ./test.pl 
line 38.
Execution of ./test.pl aborted due to compilation errors.
1..1
# Looks like your test died just after 1.

Brandon Casey already posted a patch for this that also fixes the use of 
File::Temp->New in t9700/test.pl:
http://article.gmane.org/gmane.comp.version-control.git/92791
Applying it allowed a full a run of the testsuite to complete without error.

'/bin/sh' ./aggregate-results.sh test-results/t*-*
fixed   1
success 3709
failed  0
broken  2
total   3712

-tgc

[PATCH] git-svn: Fixes my() parameter list syntax error in pre-5.8 Perl

[Marcus Griep]

---

 Per Tom G. Christensen's commentary on a breaking test in git-svn.

 git-svn.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index ecacf74..a97e1ca 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4425,7 +4425,7 @@ sub config_pager {
 
 sub run_pager {
 	return unless -t *STDOUT && defined $pager;
-	pipe my $rfd, my $wfd or return;
+	pipe my ($rfd, $wfd) or return;
 	defined(my $pid = fork) or ::fatal "Can't fork: $!";
 	if (!$pid) {
 		open STDOUT, '>&', $wfd or
-- 
1.6.0.1.415.g7bb82

Re: [PATCH] git-svn: Fixes my() parameter list syntax error in pre-5.8 Perl

[Marcus Griep]

Sorry:

Signed-off-by: Marcus Griep <marcus@griep.us>

Marcus Griep wrote:
> ---
> 
>  Per Tom G. Christensen's commentary on a breaking test in git-svn.
> 
>  git-svn.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index ecacf74..a97e1ca 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -4425,7 +4425,7 @@ sub config_pager {
>  
>  sub run_pager {
>  	return unless -t *STDOUT && defined $pager;
> -	pipe my $rfd, my $wfd or return;
> +	pipe my ($rfd, $wfd) or return;
>  	defined(my $pid = fork) or ::fatal "Can't fork: $!";
>  	if (!$pid) {
>  		open STDOUT, '>&', $wfd or

-- 
Marcus Griep
GPG Key ID: 0x5E968152
——
http://www.boohaunt.net
את.ψο´

Re: Git 1.6.0.1 breaks git-svn with perl 5.8.0

[Tom G. Christensen]

Matthieu Moy wrote:
> "Tom G. Christensen" <tgc@statsbiblioteket.dk> writes:
> 
>> The problem is that Git.pm depends on the "new" method of File::Temp
>> introduced in 0.14.
>> perl 5.8.0 only has File::Temp 0.13 while 0.14 was included with 5.8.1
>> and later.
>> The breakage was introduced in this commit:
>> e41352b24e29eba43d00a3fd117befaef1d594bc
> 
> Isn't that fixed by c14c8ceb13b299892f286757e22e6af4f6cffab5 ?
> (Git.pm: Make File::Spec and File::Temp requirement lazy, a few
> commits before 1.6.0)
> 
I don't see how it is relevant.
It does not remove the implicit requirement on File::Temp > 0.13.

-tgc