threads / discuss / 15317

Git 1.6.0.1 breaks git-svn with perl 5.8.0

Subject: Git 1.6.0.1 breaks git-svn with perl 5.8.0

## tl;dr

23 messages between Sep 1, 2008 and Sep 10, 2008.

replies: 22people: 7as markdown or json

Tom G. Christensen· Sep 1, 2008, 09:27 UTC · lore

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
Matthieu Moy· Sep 1, 2008, 09:46 UTC · re: Tom G. Christensen · lore

Re: Git 1.6.0.1 breaks git-svn with perl 5.8.0

"Tom G. Christensen" <tgc@statsbiblioteket.dk> writes:
Show 6 quoted lines
> 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
Abhijit Menon-Sen· Sep 1, 2008, 10:04 UTC · re: Matthieu Moy · lore

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

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:
Show 7 quoted lines
>
> > 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
Tom G. Christensen· Sep 1, 2008, 10:21 UTC · re: Abhijit Menon-Sen · lore

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

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
Abhijit Menon-Sen· Sep 1, 2008, 10:42 UTC · re: Tom G. Christensen · lore

[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>
---
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
Tom G. Christensen· Sep 1, 2008, 11:03 UTC · re: Abhijit Menon-Sen · lore

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

Abhijit Menon-Sen wrote:
Show 12 quoted lines
> 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
Junio C Hamano· Sep 7, 2008, 05:27 UTC · re: Tom G. Christensen · lore

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

"Tom G. Christensen" <tgc@statsbiblioteket.dk> writes:
Show 14 quoted lines
> 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?
Abhijit Menon-Sen· Sep 7, 2008, 09:26 UTC · re: Junio C Hamano · lore

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

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
Marcus Griep· Sep 7, 2008, 16:50 UTC · re: Abhijit Menon-Sen · lore

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

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
את.ψο´
Abhijit Menon-Sen· Sep 8, 2008, 08:05 UTC · re: Marcus Griep · lore

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

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
Marcus Griep· Sep 8, 2008, 15:51 UTC · re: Abhijit Menon-Sen · lore

[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.

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
Marcus Griep· Sep 8, 2008, 16:53 UTC · re: Marcus Griep · lore

[PATCH v2] 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.

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
Eric Wong· Sep 9, 2008, 01:53 UTC · re: Marcus Griep · lore

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

Marcus Griep <marcus@griep.us> wrote:
Show 17 quoted lines
> 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>

Show 135 quoted lines
>  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
Junio C Hamano· Sep 10, 2008, 03:53 UTC · re: Eric Wong · lore

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

Eric Wong <normalperson@yhbt.net> writes:
Show 26 quoted lines
> 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?
Eric Wong· Sep 10, 2008, 10:09 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
> 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
Abhijit Menon-Sen· Sep 9, 2008, 02:06 UTC · re: Marcus Griep · lore

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

At 2008-09-08 12:53:01 -0400, marcus@griep.us wrote:
Show 5 quoted lines
>
> +		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
Marcus Griep· Sep 9, 2008, 17:35 UTC · re: Abhijit Menon-Sen · lore

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

Abhijit Menon-Sen wrote:
Show 7 quoted lines
> 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
את.ψο´
Tom G. Christensen· Sep 9, 2008, 07:41 UTC · re: Marcus Griep · lore

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

Marcus Griep wrote:
Show 18 quoted lines
> 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
Marcus Griep· Sep 9, 2008, 17:55 UTC · re: Tom G. Christensen · lore

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

Tom G. Christensen wrote:
Show 13 quoted lines
> 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
את.ψο´
Tom G. Christensen· Sep 10, 2008, 07:16 UTC · re: Marcus Griep · lore

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

Marcus Griep wrote:
Show 20 quoted lines
> 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
Marcus Griep· Sep 10, 2008, 15:09 UTC · re: Tom G. Christensen · lore

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

---
 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
Marcus Griep· Sep 10, 2008, 15:11 UTC · re: Marcus Griep · lore

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

Sorry:
Signed-off-by: Marcus Griep <marcus@griep.us>
Marcus Griep wrote:
Show 20 quoted lines
> ---
> 
>  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
את.ψο´
Tom G. Christensen· Sep 1, 2008, 10:13 UTC · re: Matthieu Moy · lore

Re: Git 1.6.0.1 breaks git-svn with perl 5.8.0

Matthieu Moy wrote:
Show 13 quoted lines
> "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

← back to recent threads