Skip Menu |

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the SVN-Notify CPAN distribution.

Report information
The Basics
Id: 17844
Status: resolved
Priority: 0/
Queue: SVN-Notify
People
Owner: Nobody in particular
Requestors: jpeacock [...] cpan.org
Cc:
AdminCc:
Bug Information
Severity: Important
Broken in: 2.52
Fixed in: 2.53

History Show all quoted text
  Fri Feb 24 07:04:55 2006 jpeacock [...] cpan.org - Ticket created
Subject: _find_exe() throwing errors with SVN::Notify::Mirror
I was trying to followup on the changes you made to find the various executables (svnlook in particular). And I'm getting this: Modification of a read-only value attempted at /usr/lib/perl5/site_perl/5.8.7/SVN/Notify.pm line 1566. Modification of a read-only value attempted at /usr/lib/perl5/site_perl/5.8.7/SVN/Notify.pm line 1566. I don't know why you aren't getting that with your own tests. I've attached a trivial fix. I'm cheating and calling SVN::Notify::_find_exe() from SVN::Notify::Mirror, even though it is a private method, since I need the same functionality myself... John
Subject: mychanges.diff
--- ../SVN-Notify-2.52/lib/SVN/Notify.pm 2006-02-19 13:50:25.000000000 -0500 +++ /usr/lib/perl5/site_perl/5.8.7/SVN/Notify.pm 2006-02-24 06:48:46.000000000 -0500 @@ -1563,8 +1563,8 @@ sub _find_exe { my $exe = shift; require File::Spec; for my $path (File::Spec->path, '/usr/local/bin', '/usr/sbin') { - $path = File::Spec->catfile($path, $exe); - return $path if -x $path && -f _; + my $exepath = File::Spec->catfile($path, $exe); + return $exepath if -x $exepath && -f _; } return; }
  Fri Feb 24 12:38:33 2006 david [...] kineticode.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 09:37:31 -0800
To: bug-SVN-Notify [...] rt.cpan.org
From: "David E. Wheeler" <david [...] kineticode.com>
On Feb 24, 2006, at 04:04, via RT wrote: Show quoted text
> I was trying to followup on the changes you made to find the various > executables (svnlook in particular). And I'm getting this: > > Modification of a read-only value attempted at > /usr/lib/perl5/site_perl/5.8.7/SVN/Notify.pm line 1566. > Modification of a read-only value attempted at > /usr/lib/perl5/site_perl/5.8.7/SVN/Notify.pm line 1566.
Oh, duh. Show quoted text
> I don't know why you aren't getting that with your own tests. I've > attached a trivial fix.
Because my tests were never getting to '/usr/local/bin'. Show quoted text
> I'm cheating and calling SVN::Notify::_find_exe() from > SVN::Notify::Mirror, even though it is a private method, since I need > the same functionality myself...
That's okay. :-) I guess it wouldn't hurt to make it public. Any issue with me changing it to find_exe()? Best, David
  Fri Feb 24 12:38:35 2006 The RT System itself - Status changed from 'new' to 'open'
  Fri Feb 24 12:45:15 2006 jpeacock [...] rowman.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 12:44:43 -0500
To: bug-SVN-Notify [...] rt.cpan.org
From: John Peacock <jpeacock [...] rowman.com>
David Wheeler via RT wrote: Show quoted text
>> I don't know why you aren't getting that with your own tests. I've >> attached a trivial fix.
> > Because my tests were never getting to '/usr/local/bin'.
So, I'll make SVN::Notify::Mirror depend on SVN::Notify 2.53 then, shall I? ;-) Show quoted text
>
>> I'm cheating and calling SVN::Notify::_find_exe() from >> SVN::Notify::Mirror, even though it is a private method, since I need >> the same functionality myself...
> > That's okay. :-) I guess it wouldn't hurt to make it public. Any > issue with me changing it to find_exe()?
If it's public, you have to document it. I don't think it's worth it; I just mentioned so that you didn't go ahead and rename it without telling me. Since my module so clearly doesn't work if you module isn't installed, it isn't so bad that I'm calling into your package. A certain level of incestuous behavior is acceptable for tightly coupled modules. Alternatively, you could just always populate $self->{svn} just like you do for svnlook, on the general principal that child classes would find that useful... John -- John Peacock Director of Information Research and Technology Rowman & Littlefield Publishing Group 4501 Forbes Boulevard Suite H Lanham, MD 20706 301-459-3366 x.5010 fax 301-429-5748
  Fri Feb 24 12:46:34 2006 david [...] kineticode.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 09:45:39 -0800
To: bug-SVN-Notify [...] rt.cpan.org
From: "David E. Wheeler" <david [...] kineticode.com>
On Feb 24, 2006, at 09:38, David Wheeler via RT wrote: Show quoted text
> That's okay. :-) I guess it wouldn't hurt to make it public. Any > issue with me changing it to find_exe()?
I've turned it into a class method: my $exe = SVN::Notify->find_exe($exe_name); I'll get out another release today, unless someone sends me a patch in response to this: http://www.justatheory.com/computers/programming/perl/ port_svn_notify_to_windows.html Best, David
  Fri Feb 24 12:48:06 2006 david [...] kineticode.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 09:47:13 -0800
To: bug-SVN-Notify [...] rt.cpan.org
From: "David E. Wheeler" <david [...] kineticode.com>
On Feb 24, 2006, at 09:45, John Peacock via RT wrote: Show quoted text
> So, I'll make SVN::Notify::Mirror depend on SVN::Notify 2.53 then, > shall > I? ;-)
Sure, why not? Show quoted text
> If it's public, you have to document it. I don't think it's worth > it; I > just mentioned so that you didn't go ahead and rename it without > telling > me. Since my module so clearly doesn't work if you module isn't > installed, it isn't so bad that I'm calling into your package. A > certain level of incestuous behavior is acceptable for tightly coupled > modules.
Yeah, but I don't mind making it public, either. Other subclasses might need it, no? Show quoted text
> Alternatively, you could just always populate $self->{svn} just > like you > do for svnlook, on the general principal that child classes would find > that useful...
Yeah, but SVN::Notify doesn't use it, so I'd rather not waste the cycles looking for it unless it's needed. Best, David
  Fri Feb 24 12:51:02 2006 jpeacock [...] rowman.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 12:50:28 -0500
To: bug-SVN-Notify [...] rt.cpan.org
From: John Peacock <jpeacock [...] rowman.com>
David Wheeler via RT wrote: Show quoted text
> I've turned it into a class method: > > my $exe = SVN::Notify->find_exe($exe_name); > > I'll get out another release today, unless someone sends me a patch > in response to this:
FWIW, I just figured out a better way to fix up your original code so it doesn't require another variable (watch the wrapping): --- ../SVN-Notify-2.52/lib/SVN/Notify.pm 2006-02-19 13:50:25.000000000 -0500 +++ /usr/lib/perl5/site_perl/5.8.7/SVN/Notify.pm 2006-02-24 12:46:45.000000000 -0500 @@ -1562,7 +1562,7 @@ sub _dbpnt { print __PACKAGE__, ": $_[1] sub _find_exe { my $exe = shift; require File::Spec; - for my $path (File::Spec->path, '/usr/local/bin', '/usr/sbin') { + for my $path (File::Spec->path, ['/usr/local/bin', '/usr/sbin']) { $path = File::Spec->catfile($path, $exe); return $path if -x $path && -f _; } By making it an anonymous array, it gets around the readonly mess... John -- John Peacock Director of Information Research and Technology Rowman & Littlefield Publishing Group 4501 Forbes Boulevard Suite H Lanham, MD 20706 301-459-3366 x.5010 fax 301-429-5748
  Fri Feb 24 12:55:28 2006 david [...] kineticode.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 09:54:30 -0800
To: bug-SVN-Notify [...] rt.cpan.org
From: "David E. Wheeler" <david [...] kineticode.com>
On Feb 24, 2006, at 09:51, John Peacock via RT wrote: Show quoted text
> --- ../SVN-Notify-2.52/lib/SVN/Notify.pm 2006-02-19 > 13:50:25.000000000 -0500 > +++ /usr/lib/perl5/site_perl/5.8.7/SVN/Notify.pm 2006-02-24 > 12:46:45.000000000 -0500 > @@ -1562,7 +1562,7 @@ sub _dbpnt { print __PACKAGE__, ": $_[1] > sub _find_exe { > my $exe = shift; > require File::Spec; > - for my $path (File::Spec->path, '/usr/local/bin', '/usr/sbin') { > + for my $path (File::Spec->path, ['/usr/local/bin', '/usr/ > sbin']) { > $path = File::Spec->catfile($path, $exe); > return $path if -x $path && -f _; > } > > By making it an anonymous array, it gets around the readonly mess...
Wait a minute. Doesn't that just mean that it will look for "ARRAY (0x1800568)/foo.exe"? Yep, that's what it looks like: % cat try #!/usr/bin/perl -w use strict; for my $path (qw(foo bar), [qw(baz bot)]) { print $path, $/; } % ./try foo bar ARRAY(0x1800568) Best, David
  Fri Feb 24 13:00:07 2006 jpeacock [...] rowman.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 12:59:39 -0500
To: bug-SVN-Notify [...] rt.cpan.org
From: John Peacock <jpeacock [...] rowman.com>
David Wheeler via RT wrote: Show quoted text
> Wait a minute. Doesn't that just mean that it will look for "ARRAY > (0x1800568)/foo.exe"? > > Yep, that's what it looks like: > > % cat try > #!/usr/bin/perl -w > use strict; > for my $path (qw(foo bar), [qw(baz bot)]) { > print $path, $/; > } > % ./try > foo > bar > ARRAY(0x1800568)
Bugger. It "works" though, since I don't get any errors (don't know why it works). Nevermind! :( John -- John Peacock Director of Information Research and Technology Rowman & Littlefield Publishing Group 4501 Forbes Boulevard Suite H Lanham, MD 20706 301-459-3366 x.5010 fax 301-429-5748
  Fri Feb 24 13:02:02 2006 jpeacock [...] rowman.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 13:01:29 -0500
To: bug-SVN-Notify [...] rt.cpan.org
From: John Peacock <jpeacock [...] rowman.com>
John Peacock via RT wrote: Show quoted text
> Bugger. It "works" though, since I don't get any errors (don't know why > it works). Nevermind!
I see that I already have '/usr/local/bin' in my path, so why was I getting an error at all before??? John -- John Peacock Director of Information Research and Technology Rowman & Littlefield Publishing Group 4501 Forbes Boulevard Suite H Lanham, MD 20706 301-459-3366 x.5010 fax 301-429-5748
  Fri Feb 24 13:02:45 2006 david [...] kineticode.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 10:01:52 -0800
To: bug-SVN-Notify [...] rt.cpan.org
From: "David E. Wheeler" <david [...] kineticode.com>
On Feb 24, 2006, at 10:00, John Peacock via RT wrote: Show quoted text
> Bugger. It "works" though, since I don't get any errors (don't > know why > it works). Nevermind!
This would work: for my $path (qw(foo bar), @{ [qw(baz bot)] }) { print $path, $/; } I'll go ahead and do that. Best, David
  Fri Feb 24 13:05:14 2006 david [...] kineticode.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 10:04:24 -0800
To: bug-SVN-Notify [...] rt.cpan.org
From: "David E. Wheeler" <david [...] kineticode.com>
On Feb 24, 2006, at 10:02, John Peacock via RT wrote: Show quoted text
> I see that I already have '/usr/local/bin' in my path, so why was I > getting an error at all before???
I dunno, but I'm glad you did. :-) David
  Fri Feb 24 13:07:25 2006 jpeacock [...] rowman.com - Correspondence added
Subject: Re: [rt.cpan.org #17844] _find_exe() throwing errors with SVN::Notify::Mirror
Date: Fri, 24 Feb 2006 13:06:57 -0500
To: bug-SVN-Notify [...] rt.cpan.org
From: John Peacock <jpeacock [...] rowman.com>
David Wheeler via RT wrote: Show quoted text
> for my $path (qw(foo bar), @{ [qw(baz bot)] }) { > print $path, $/; > }
Yeah, that's what I meant to type. Damn cordless keyboard... ;-) John -- John Peacock Director of Information Research and Technology Rowman & Littlefield Publishing Group 4501 Forbes Boulevard Suite H Lanham, MD 20706 301-459-3366 x.5010 fax 301-429-5748
  Mon Apr 03 19:00:48 2006 dwheeler [...] cpan.org - Status changed from 'open' to 'resolved'
  Mon Apr 03 19:00:49 2006 dwheeler [...] cpan.org - Fixed in 2.53 added