Git 1.6.0.1 breaks git-svn with perl 5.8.0

PATCH In review Tom G. Christensen 7 participants 23 messages Jun 15, 2016

Share & tools

Tom G. Christensen

author

2016-06-15 22:45 UTC #0 lore raw

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

2016-06-15 22:45 UTC #1 lore raw

Quoted 6 lines · 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

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

Signature · 1 line

Matthieu

Abhijit Menon-Sen

2016-06-15 22:45 UTC #2 lore raw

PATCH 1/1 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 <[email protected]>

---

Quoted 7 lines · At 2008-09-01 11:46:17 +0200, [email protected] wrote:

Quoted 3 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(-)

perl/Git.pm +4 -1

index 102e6a4..4e901b6 100644
@@ -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($@);
 }

Signature · 1 line

1.6.0.49.gea35

Tom G. Christensen

author

2016-06-15 22:45 UTC #3 lore raw

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

Quoted 4 lines · 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.

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

2016-06-15 22:45 UTC #4 lore raw

PATCH 1/1 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 <[email protected]>

---

Quoted 3 lines · At 2008-09-01 12:21:02 +0200, [email protected] 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(-)

perl/Git.pm +7 -4

index 102e6a4..f383ff8 100644
@@ -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;
 	}

Signature · 1 line

1.6.0.1.161.g7f314

Tom G. Christensen

author

2016-06-15 22:45 UTC #5 lore raw

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

Quoted 12 lines · 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 <[email protected]>
---
Quoted 2 lines · At 2008-09-01 12:21:02 +0200, [email protected] 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

2016-06-15 22:45 UTC #6 lore raw

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

Quoted 7 lines · Tom G. Christensen

Quoted 6 lines · 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 <[email protected]>
---

Quoted 7 lines

Quoted 6 lines
Quoted 2 lines · At 2008-09-01 12:21:02 +0200, [email protected] 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

2016-06-15 22:45 UTC #7 lore raw

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

Quoted 3 lines · At 2008-09-06 22:27:56 -0700, [email protected] 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

2016-06-15 22:45 UTC #8 lore raw

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

Quoted 3 lines · Abhijit Menon-Sen

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.

Signature · 5 lines

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

Abhijit Menon-Sen

2016-06-15 22:45 UTC #9 lore raw

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

Quoted 4 lines · At 2008-09-07 12:50:56 -0400, [email protected] 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 <[email protected]>
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 <[email protected]>

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

perl/Git.pm +8 -5

index 102e6a4..b0498ca 100644
@@ -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;
 	}

Signature · 1 line

1.6.0.1.196.g01914

Marcus Griep

2016-06-15 22:45 UTC #10 lore raw

PATCH 1/1 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 <[email protected]>

---

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(-)

git-svn.perl +2 -2

index ee3f5ed..a6d75db 100755
@@ -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);

perl/Git.pm +23 -6

index 102e6a4..3ea3fb2 100644
@@ -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;
 }

Signature · 1 line

1.6.0.1.400.gd2470

Marcus Griep

2016-06-15 22:45 UTC #11 lore raw

PATCH v2 1/1 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 <[email protected]>

---

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:

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

git-svn.perl +2 -2

index ee3f5ed..c92bd8e 100755
@@ -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);

perl/Git.pm +30 ~~-12~~

96 lines not on this page · raw message

Signature · 1 line

1.6.0.1.400.gd2470

Eric Wong

2016-06-15 22:45 UTC #12 lore raw

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

Quoted 9 lines · 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 <[email protected]>
---

Quoted 8 lines

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>

Quoted 135 lines

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

git-svn.perl +2 -2

index ee3f5ed..c92bd8e 100755
@@ -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);

perl/Git.pm +30 ~~-12~~

index 102e6a4..3f5514c 100644
@@ -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

Signature · 1 line

1.6.0.1.400.gd2470

Junio C Hamano

2016-06-15 22:45 UTC #13 lore raw

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

Quoted 19 lines · Eric Wong

Quoted 9 lines · 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 <[email protected]>
---
Quoted 8 lines
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'.

Quoted 7 lines

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

2016-06-15 22:45 UTC #14 lore raw

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

Quoted 24 lines · Junio C Hamano

Quoted 22 lines · Eric Wong
Quoted 9 lines · 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 <[email protected]>
---
Quoted 8 lines
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).

Quoted 8 lines

Quoted 4 lines
<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.

Quoted 1 line

This is a 'maint' material, right?

Yes, definitely.

Signature · 1 line

Eric Wong

Abhijit Menon-Sen

2016-06-15 22:45 UTC #15 lore raw

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

In reply to Marcus Griep

perl/Git.pm sub _temp_cache { · line 1027

Quoted 5 lines · At 2008-09-08 12:53:01 -0400, [email protected] wrote:

perl/Git.pm +3 -0

+		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

2016-06-15 22:45 UTC #16 lore raw

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

Quoted 7 lines · Abhijit Menon-Sen

perl/Git.pm sub _temp_cache { · line 1027
Quoted 4 lines · At 2008-09-08 12:53:01 -0400, [email protected] wrote:
perl/Git.pm +3 -0
+		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.

Signature · 5 lines

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

Tom G. Christensen

author Tested-by on v2 1/1

2016-06-15 22:45 UTC #17 lore raw

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

In reply to Marcus Griep

Quoted 9 lines · 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 <[email protected]>
---

Quoted 9 lines

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.

stderr.three 2008-09-09 07:34:42.000000000 +0000 renamed from expect.three 2008-09-09 07:34:39.000000000 +0000 +1 -0

@@ -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 <[email protected]>

-tgc

Marcus Griep

2016-06-15 22:45 UTC #18 lore raw

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

Quoted 12 lines · Tom G. Christensen

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.
stderr.three 2008-09-09 07:34:42.000000000 +0000 renamed from expect.three 2008-09-09 07:34:39.000000000 +0000 +1 -0
@@ -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/*'

Quoted 1 line

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

Signature · 5 lines

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

Tom G. Christensen

author

2016-06-15 22:45 UTC #19 lore raw

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

Quoted 13 lines · Marcus Griep

Quoted 12 lines · Tom G. Christensen
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.
stderr.three 2008-09-09 07:34:42.000000000 +0000 renamed from expect.three 2008-09-09 07:34:39.000000000 +0000 +1 -0
@@ -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/*'

Quoted 7 lines

Quoted 1 line
* 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

2016-06-15 22:45 UTC #20 lore raw

PATCH 1/1 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(-)

git-svn.perl +1 -1

index ecacf74..a97e1ca 100755
@@ -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

Signature · 1 line

1.6.0.1.415.g7bb82

Marcus Griep

2016-06-15 22:45 UTC #21 lore raw

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

Sorry:

Signed-off-by Marcus Griep <[email protected]>

Quoted 20 lines · 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(-)

git-svn.perl +1 -1

index ecacf74..a97e1ca 100755
@@ -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

Signature · 5 lines

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

Tom G. Christensen

author

2016-06-15 22:45 UTC #22 lore raw

In reply to Matthieu Moy

Quoted 9 lines · Matthieu Moy

Quoted 6 lines · 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

Quoted 4 lines

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