# Git 1.6.0.1 breaks git-svn with perl 5.8.0 23 messages from 2008-09-01 to 2008-09-10. Participants: Tom G. Christensen, Matthieu Moy, Abhijit Menon-Sen, Junio C Hamano, Marcus Griep, Eric Wong. Thread: https://gitlist.dev/t/15317 ## Tom G. Christensen, 2008-09-01 09:27 Subject: Git 1.6.0.1 breaks git-svn with perl 5.8.0 Message-ID: <48BBB59F.9080204@statsbiblioteket.dk> URL: https://gitlist.dev/e/48BBB59F.9080204%40statsbiblioteket.dk ``` 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, 2008-09-01 09:46 Subject: Re: Git 1.6.0.1 breaks git-svn with perl 5.8.0 Message-ID: URL: https://gitlist.dev/e/vpqvdxggpw6.fsf%40bauges.imag.fr In-Reply-To: <48BBB59F.9080204@statsbiblioteket.dk> ``` "Tom G. Christensen" 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 ``` ## Abhijit Menon-Sen, 2008-09-01 10:04 Subject: [PATCH] Git.pm: Require File::Temp 0.14 for new() Message-ID: <20080901100435.GC6555@toroid.org> URL: https://gitlist.dev/e/20080901100435.GC6555%40toroid.org In-Reply-To: ``` 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 --- 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 ``` ## Tom G. Christensen, 2008-09-01 10:13 Subject: Re: Git 1.6.0.1 breaks git-svn with perl 5.8.0 Message-ID: <48BBC060.6080809@statsbiblioteket.dk> URL: https://gitlist.dev/e/48BBC060.6080809%40statsbiblioteket.dk In-Reply-To: ``` Matthieu Moy wrote: > "Tom G. Christensen" 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 ``` ## Tom G. Christensen, 2008-09-01 10:21 Subject: Re: [PATCH] Git.pm: Require File::Temp 0.14 for new() Message-ID: <48BBC20E.20808@statsbiblioteket.dk> URL: https://gitlist.dev/e/48BBC20E.20808%40statsbiblioteket.dk In-Reply-To: <20080901100435.GC6555@toroid.org> ``` 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, 2008-09-01 10:42 Subject: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <20080901104222.GA10026@toroid.org> URL: https://gitlist.dev/e/20080901104222.GA10026%40toroid.org In-Reply-To: <48BBC20E.20808@statsbiblioteket.dk> ``` 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 --- 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, 2008-09-01 11:03 Subject: Re: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <48BBCBEA.8000301@statsbiblioteket.dk> URL: https://gitlist.dev/e/48BBCBEA.8000301%40statsbiblioteket.dk In-Reply-To: <20080901104222.GA10026@toroid.org> ``` 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 > --- > > 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, 2008-09-07 05:27 Subject: Re: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <7vskscplsz.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vskscplsz.fsf%40gitster.siamese.dyndns.org In-Reply-To: <48BBCBEA.8000301@statsbiblioteket.dk> ``` "Tom G. Christensen" 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 >> --- >> >> 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, 2008-09-07 09:26 Subject: Re: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <20080907092624.GA8470@toroid.org> URL: https://gitlist.dev/e/20080907092624.GA8470%40toroid.org In-Reply-To: <7vskscplsz.fsf@gitster.siamese.dyndns.org> ``` 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, 2008-09-07 16:50 Subject: Re: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <48C40670.6090106@gmail.com> URL: https://gitlist.dev/e/48C40670.6090106%40gmail.com In-Reply-To: <20080907092624.GA8470@toroid.org> ``` 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, 2008-09-08 08:05 Subject: Re: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <20080908080548.GA3206@toroid.org> URL: https://gitlist.dev/e/20080908080548.GA3206%40toroid.org In-Reply-To: <48C40670.6090106@gmail.com> ``` 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 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 --- 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, 2008-09-08 15:51 Subject: [PATCH] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <1220889063-20387-1-git-send-email-marcus@griep.us> URL: https://gitlist.dev/e/1220889063-20387-1-git-send-email-marcus%40griep.us In-Reply-To: <20080908080548.GA3206@toroid.org> ``` 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 --- 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, 2008-09-08 16:53 Subject: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <1220892781-24343-1-git-send-email-marcus@griep.us> URL: https://gitlist.dev/e/1220892781-24343-1-git-send-email-marcus%40griep.us In-Reply-To: <1220889063-20387-1-git-send-email-marcus@griep.us> ``` 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 --- 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, 2008-09-09 01:53 Subject: Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <20080909015338.GA15974@yp-box.dyndns.org> URL: https://gitlist.dev/e/20080909015338.GA15974%40yp-box.dyndns.org In-Reply-To: <1220892781-24343-1-git-send-email-marcus@griep.us> ``` 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 > --- > > 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). Can we rename temp_fname() to temp_path(), though? "fname" just doesn't look right in the API to me... > 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 ``` ## Abhijit Menon-Sen, 2008-09-09 02:06 Subject: Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <20080909020605.GA19190@toroid.org> URL: https://gitlist.dev/e/20080909020605.GA19190%40toroid.org In-Reply-To: <1220892781-24343-1-git-send-email-marcus@griep.us> ``` 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 ``` ## Tom G. Christensen, 2008-09-09 07:41 Subject: Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <48C628AA.4020100@statsbiblioteket.dk> URL: https://gitlist.dev/e/48C628AA.4020100%40statsbiblioteket.dk In-Reply-To: <1220892781-24343-1-git-send-email-marcus@griep.us> ``` 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 > --- > > 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 ``` ## Marcus Griep, 2008-09-09 17:35 Subject: Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <48C6B3F0.505@griep.us> URL: https://gitlist.dev/e/48C6B3F0.505%40griep.us In-Reply-To: <20080909020605.GA19190@toroid.org> ``` 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 את.ψο´ ``` ## Marcus Griep, 2008-09-09 17:55 Subject: Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <48C6B886.7020108@griep.us> URL: https://gitlist.dev/e/48C6B886.7020108%40griep.us In-Reply-To: <48C628AA.4020100@statsbiblioteket.dk> ``` 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 את.ψο´ ``` ## Junio C Hamano, 2008-09-10 03:53 Subject: Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <7vwshkzmfv.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwshkzmfv.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080909015338.GA15974@yp-box.dyndns.org> ``` Eric Wong writes: > 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 >> --- >> >> 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). > > > Can we rename temp_fname() to temp_path(), though? "fname" > just doesn't look right in the API to me... > 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? ``` ## Tom G. Christensen, 2008-09-10 07:16 Subject: Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <48C7745F.1070008@statsbiblioteket.dk> URL: https://gitlist.dev/e/48C7745F.1070008%40statsbiblioteket.dk In-Reply-To: <48C6B886.7020108@griep.us> ``` 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 ``` ## Eric Wong, 2008-09-10 10:09 Subject: Re: [PATCH v2] Git.pm: Use File::Temp->tempfile instead of ->new Message-ID: <20080910100958.GE13085@hand.yhbt.net> URL: https://gitlist.dev/e/20080910100958.GE13085%40hand.yhbt.net In-Reply-To: <7vwshkzmfv.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > Eric Wong writes: > > > 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 > >> --- > >> > >> 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). > > > > > > Can we rename temp_fname() to temp_path(), though? "fname" > > just doesn't look right in the API to me... > > > > 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 ``` ## Marcus Griep, 2008-09-10 15:09 Subject: [PATCH] git-svn: Fixes my() parameter list syntax error in pre-5.8 Perl Message-ID: <1221059386-3972-1-git-send-email-marcus@griep.us> URL: https://gitlist.dev/e/1221059386-3972-1-git-send-email-marcus%40griep.us In-Reply-To: <48C7745F.1070008@statsbiblioteket.dk> ``` --- 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, 2008-09-10 15:11 Subject: Re: [PATCH] git-svn: Fixes my() parameter list syntax error in pre-5.8 Perl Message-ID: <48C7E393.2070006@griep.us> URL: https://gitlist.dev/e/48C7E393.2070006%40griep.us In-Reply-To: <1221059386-3972-1-git-send-email-marcus@griep.us> ``` Sorry: Signed-off-by: Marcus Griep 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 את.ψο´ ```