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: 27274
Status: resolved
Priority: 0/
Queue: SVN-Notify
People
Owner: Nobody in particular
Requestors: jpeacock [...] rowman.com
Cc:
AdminCc:
Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: 2.66

History Show all quoted text
  Thu May 24 08:43:22 2007 jpeacock [...] rowman.com - Ticket created
Subject: SVN-Notify-0.26 breaks SVN::Notify::Config (and others)
Date: Thu, 24 May 2007 08:41:42 -0400
To: John Peacock via RT <bug-SVN-Notify [...] rt.cpan.org>
From: John Peacock <jpeacock [...] rowman.com>
I got a bug report vs. SVN::Notify::Config for test failures: http://rt.cpan.org/Ticket/Display.html?id=26209 and it took me ages to recreate it (since I didn't notice that you'd updated SVN::Notify). Once it was pointed out that it was 0.26 that triggers the error, I was able to finally debug it. The problem is this line: --- lib/SVN/Notify.pm~ 2007-03-27 03:00:40.000000000 -0400 +++ lib/SVN/Notify.pm 2007-05-24 06:08:56.000000000 -0400 @@ -955,7 +955,7 @@ sub prepare_recipients { if (/$rx/) { $self->_dbpnt( qq{"$_" matched $rx}) if $self->{verbose} > 2; push @$tos, $email unless $seen{$email}++; - splice @$regexen, $i, 2; + #splice @$regexen, $i, 2; } } # Grab the context if it's needed for the subject. You are iterating over the $regexen using a loop, but in the line above you are splicing out any regex that matches. The end result is that everytime you match one of the regexes, you are *skipping* the next regex (since you can't both iterate over the index of the array *and* remove array elements casually). The above trivial patch restores SVN::Notify::Config's tests. Thanks John p.s. RT.cpan.org seems to be on the fritz, which is why I am cc'ing you directly... -- John Peacock Director of Information Research and Technology Rowman & Littlefield Publishing Group 4501 Forbes Blvd Suite H Lanham, MD 20706 301-459-3366 x.5010 fax 301-429-5747
  Thu May 24 12:53:42 2007 david [...] kineticode.com - Correspondence added
Subject: Re: [rt.cpan.org #27274] SVN-Notify-0.26 breaks SVN::Notify::Config (and others)
Date: Thu, 24 May 2007 09:52:56 -0700
To: bug-SVN-Notify [...] rt.cpan.org
From: "David E. Wheeler" <david [...] kineticode.com>
On May 24, 2007, at 03:21, John Peacock wrote: Show quoted text
> The above trivial patch restores SVN::Notify::Config's tests.
Do SVN::Notify's tests still pass? I'm certain that I added that for a reason… Thanks, David
  Thu May 24 12:53:43 2007 The RT System itself - Status changed from 'new' to 'open'
  Thu May 24 13:32:08 2007 jpeacock [...] rowman.com - Correspondence added
Subject: Re: [rt.cpan.org #27274] SVN-Notify-0.26 breaks SVN::Notify::Config (and others)
Date: Thu, 24 May 2007 13:13:51 -0400
To: bug-SVN-Notify [...] rt.cpan.org
From: John Peacock <jpeacock [...] rowman.com>
David Wheeler via RT wrote: Show quoted text
> Do SVN::Notify's tests still pass? I'm certain that I added that for > a reason…
Yes, *your* tests pass, but are you testing multiple recipients for overlapping path's? SVN::Notify::Config's uses the empty path (matches every repository commit) for globals (like PATH). Besides, just from first principles, the code is wrong. You can't access an array by index in a for() loop and delete elements from the array inside the loop (unless you also decrement the index at the same time). You are guaranteed to skip elements. #!perl -wl my @array = (1,2,3,4,5,6,7,8,9); for (my $i=0; $i < $#array; $i++) { print "processing $i"; if ( $i%2 == 1 ) { splice @array, $i, 1; } } foreach my $i ( @array ) { print $i; } which prints processing 0 processing 1 processing 2 processing 3 processing 4 processing 5 1 3 4 6 7 9 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
  Thu May 24 13:39:22 2007 david [...] kineticode.com - Correspondence added
Subject: Re: [rt.cpan.org #27274] SVN-Notify-0.26 breaks SVN::Notify::Config (and others)
Date: Thu, 24 May 2007 10:37:24 -0700
To: bug-SVN-Notify [...] rt.cpan.org
From: "David E. Wheeler" <david [...] kineticode.com>
On May 24, 2007, at 10:32, John Peacock via RT wrote: Show quoted text
> Yes, *your* tests pass, but are you testing multiple recipients for > overlapping path's? SVN::Notify::Config's uses the empty path > (matches > every repository commit) for globals (like PATH). > > Besides, just from first principles, the code is wrong. You can't > access an array by index in a for() loop and delete elements from the > array inside the loop (unless you also decrement the index at the same > time). You are guaranteed to skip elements.
Quite right. My local Perl is broken (too much Ruby development these days), but does this work? Index: Notify.pm =================================================================== --- Notify.pm (revision 3299) +++ Notify.pm (working copy) @@ -961,8 +961,9 @@ my %seen; while (<$fh>) { s/[\n\r\/\\]+$//; - for (my $i = 0; $i < @$regexen; $i += 2) { - my ($email, $rx) = @{$regexen}[$i, $i + 1]; + @rxen = @$regexen; + for (my $i = 0; $i < @rxen; $i += 2) { + my ($email, $rx) = @rxen[$i, $i + 1]; # If the directory matches the regex, save the email. if (/$rx/) { $self->_dbpnt( qq{"$_" matched $rx}) if $self-> {verbose} > 2; Thanks, David
  Thu May 24 14:09:15 2007 jpeacock [...] rowman.com - Correspondence added
Subject: Re: [rt.cpan.org #27274] SVN-Notify-0.26 breaks SVN::Notify::Config (and others)
Date: Thu, 24 May 2007 14:08:57 -0400
To: bug-SVN-Notify [...] rt.cpan.org
From: John Peacock <jpeacock [...] rowman.com>
David Wheeler via RT wrote: Show quoted text
> Quite right. My local Perl is broken (too much Ruby development these > days), but does this work?
It does, but I'm not entirely convinced that is the correct fix. I *think* (based on the comments) that you are considering the possibility that the same e-mail address (key) will match multiple paths and you only want to send a single e-mail. In this case, the previous line: push @$tos, $email unless $seen{$email}++; has the appropriate effect. I suspect the splice() came out the sensible notion that if you have already matched once in this file, you don't want to check the same regex again. But I 'm not 100% sure that is what that code does... :( For reference purposes, SVN::Notify::Config works like having multiple to_regex_map entries on the command line of svnnotify. All matching paths must generate a matching 'to' entry in order for everything to work properly. 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
  Thu May 24 14:45:21 2007 david [...] kineticode.com - Correspondence added
Subject: Re: [rt.cpan.org #27274] SVN-Notify-0.26 breaks SVN::Notify::Config (and others)
Date: Thu, 24 May 2007 11:43:37 -0700
To: bug-SVN-Notify [...] rt.cpan.org
From: "David E. Wheeler" <david [...] kineticode.com>
On May 24, 2007, at 11:09, John Peacock via RT wrote: Show quoted text
> I *think* (based on the comments) that you are considering the > possibility that the same e-mail address (key) will match multiple > paths > and you only want to send a single e-mail.
Right, each email address should be added as a recipient only once. Show quoted text
> In this case, the previous line: > > push @$tos, $email unless $seen{$email}++; > > has the appropriate effect. I suspect the splice() came out the > sensible notion that if you have already matched once in this file, > you > don't want to check the same regex again. But I 'm not 100% sure that > is what that code does... :(
It looks to me like it does, but you're right about the push statement covering the issue. Show quoted text
> For reference purposes, SVN::Notify::Config works like having multiple > to_regex_map entries on the command line of svnnotify. All matching > paths must generate a matching 'to' entry in order for everything to > work properly.
Oh, so they *all* have to work? So it's best, from your POV, not to change the array at all. Okay, that works for me, since we do have the push line to cover the issue, as you pointed out. Best, David
  Thu May 24 15:15:05 2007 jpeacock [...] rowman.com - Correspondence added
Subject: Re: [rt.cpan.org #27274] SVN-Notify-2.65 breaks SVN::Notify::Config (and others)
Date: Thu, 24 May 2007 15:13:05 -0400
To: bug-SVN-Notify [...] rt.cpan.org
From: John Peacock <jpeacock [...] rowman.com>
David Wheeler via RT wrote: Show quoted text
> Oh, so they *all* have to work? So it's best, from your POV, not to > change the array at all. Okay, that works for me, since we do have > the push line to cover the issue, as you pointed out.
Yeah, if you have this structure: /project1/trunk /project1/trunk/accounting and the following config file: #!/usr/bin/perl -MSVN::Notify::Config=$0 --- #YAML:1.0 '': PATH: "/usr/local/bin:/usr/bin" '/project1/trunk': handler: Dummy user_domain: example.com to: - trunk@example.com - other@example.com with_diff: 1 '/project1/trunk/accounting': handler: Dummy from: tags@example.com to: - cfo@example.com smtp: 127.0.0.1 Then a specific commit of /project1/trunk/accounting/somefile.txt should generate 3 'to' entries (each of the above will match). In the above case the Dummy handler just dumps out the $notify object as YAML, but it could be two e-mail's with diff and one without... So if you can release 2.6501 without the offending splice(), then I can close my bug too... ;-) 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
  Sun Jun 03 10:06:35 2007 jpeacock [...] rowman.com - Correspondence added
Subject: Re: [rt.cpan.org #27274] SVN-Notify-2.65 breaks SVN::Notify::Config (and others)
Date: Sun, 03 Jun 2007 10:04:00 -0400
To: bug-SVN-Notify [...] rt.cpan.org
From: John Peacock <jpeacock [...] rowman.com>
PING - Any chance you can squeeze out a new release of SVN::Notify with the fix we discussed? Is there anything I can do to help? 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
  Sun Jun 17 01:11:44 2007 david [...] kineticode.com - Correspondence added
Subject: Re: [rt.cpan.org #27274] SVN-Notify-2.65 breaks SVN::Notify::Config (and others)
Date: Sat, 16 Jun 2007 22:11:21 -0700
To: bug-SVN-Notify [...] rt.cpan.org
From: "David E. Wheeler" <david [...] kineticode.com>
On Jun 3, 2007, at 07:06, John Peacock via RT wrote: Show quoted text
> PING - Any chance you can squeeze out a new release of SVN::Notify > with the fix > we discussed? Is there anything I can do to help?
Sorry, I've just been so slammed. I suck. I've just now uploaded 2.66 with the fix. Apologies for the delay. Best, David
  Sun Jun 17 11:07:36 2007 jpeacock [...] rowman.com - Correspondence added
Subject: Re: [rt.cpan.org #27274] SVN-Notify-2.65 breaks SVN::Notify::Config (and others)
Date: Sun, 17 Jun 2007 11:07:42 -0400
To: bug-SVN-Notify [...] rt.cpan.org
From: John Peacock <jpeacock [...] rowman.com>
David Wheeler via RT wrote: Show quoted text
> Sorry, I've just been so slammed. I suck. I've just now uploaded 2.66 > with the fix. Apologies for the delay.
That's OK! I appreciate your getting to it. John -- John Peacock Director of Information Research and Technology Rowman & Littlefield Publishing Group 4501 Forbes Blvd Suite H Lanham, MD 20706 301-459-3366 x.5010 fax 301-429-5747
  Wed Feb 06 13:56:37 2008 dwheeler [...] cpan.org - Status changed from 'open' to 'resolved'
  Wed Feb 06 13:56:53 2008 dwheeler [...] cpan.org - Fixed in 2.66 added