325375 - bonsai's cvsview2 script fails when multiple source files in URL

Status: VERIFIED FIXED

(mozilla.org :: CVS: Administration, task, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

The URL 
http://bonsai.mozilla.org/cvsview2.cgi?command=DIRECTORY&subdir=mozilla/security/nss/lib/util&files=secoid.c%2Bsecoidt.h&root=/cvsroot
worked last week, but fails today.  The error message says:

> Software error: Unexpected EOF at cvsblame.pl line 133.
>
> For help, please send mail to the webmaster ([e-mail]), giving 
> this error message and the time and date of the error. 

Such URLs are sent out by the cvsmailfilter daemon to report checkins, and
are used by people who review recent checkins.

The problem seems to be due to multiple file names in the URL.  If I remove
all but one of the file names from the URL, then it works.  e.g.
http://bonsai.mozilla.org/cvsview2.cgi?command=DIRECTORY&subdir=mozilla/security/nss/lib/util&files=secoid.c&root=/cvsroot
works.  

This problem makes reviewing recent checkins way too difficult.

Comment 1

[Dave Miller [:justdave]]

OK, I wonder why this has ever worked....

According to the source, you should be using a colon, not a comma.  According to the file history on that script, it used to use a plus.  It has never used a comma.

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/webtools/bonsai/cvsview2.cgi&mark=475#468

Comment 2

[Dave Miller [:justdave]]

cvsmailfilter hasn't been touched in ages.  This is likely something that was locally hacked on bonsai.mozilla.org and was never committed the cvs (and this got lost in the last upgrade).

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I don't understand your remark about a comma.  %2B is a '+'.  
The cvsmailfilter script has been using %2B for at least two years.
'+' or %2B should continue to work.

Comment 4

[Dave Miller [:justdave]]

(In reply to comment #3)
> I don't understand your remark about a comma.  %2B is a '+'.  
> The cvsmailfilter script has been using %2B for at least two years.
> '+' or %2B should continue to work.

Sorry, I see to have confused myself trying to look at that, that's what I get for answering email when I'm dead tired.  So yes, short answer is, the recent bonsai upgrade broke cvsmailfilter.

OK, so this is a bonsai problem, essentially the checkin from attachment 167487 [details] [diff] on bug 261616 essentially broke existing outside scripts.

It'd be nice to fix cvsmailfilter to use the new URL format (since it is more readable in the URL as well), however, that still doesn't help people looking at old cvsmailfilter mail (of which there's a lot of it out there, it's archived to at least one newsgroup).

My recommendation for this is to both fix cvsmailfilter so it uses a colon and fix cvsview2.cgi so that either a colon or a plus works.

Comment 5

[Dave Miller [:justdave]]

guess I'll take this.  I've been having to do some other hacking on bonsai recently anyway because of the performance problems, so I can probably fit this in and get away with it. :)

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Dave, At this point, I'd settle for your previous suggestion:

> It'd be nice to fix cvsmailfilter to use the new URL format (since it is more
> readable in the URL as well), 

It's more important to be able to use new cvsmailfilter postings in real time
than to be able to use old archived cvsmailfilter postings, IMO.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Please add a comment to this bug stating where the source to the 
cvsmailfilter script can be found (via cvs or lxr). 
Or, alternatively, attach a copy to this bug.  

I am willing to hack it to work with the current cvsview2.cgi
I am probably the last person who touched it, but it was so long ago
that I've forgotten its name or how to find it.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Where are the priority level definitions for webtools bugs?  
Since I don't presently have access to them, I'm guessing about this priority.  
We might as well shut down m.d.t.crypto.checkins until this is fixed.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #7)
> Please add a comment to this bug stating where the source to the 
> cvsmailfilter script can be found (via cvs or lxr). 

cvs -d:pserver:[e-mail]:/cvsroot co CVSROOT/cvsmailfilter.pl

(IIRC, you can't get to CVSROOT on the master cvs unless you're cvsadm, but you can get to the mirrored part of it on the mirror.)

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Thanks, David.  
I see my previous change to this file was rev 1.8, which Myk checked in for me.

<whine>
I also see that this file used '+' in revs 1.1 through 1.5, and used %2B
(which is '+') from revs 1.6 onward, and has never used ':' for this purpose.
So, how is it that cvsview2 got changed to use ':' and cvsmailfilter didn't?
</whine>

In any case, I will attach a patch to change cvsmailfiler.pl to agree with 
cvsview2 shortly.  Thanks again for the path.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Is there any reason why bug 261616 should have restricted viewing?
According the the RCS log for cvsview2.cgi, the fix for that bug was 
the cause of this bug.

> 1.40 <[e-mail]> 2004-11-30 17:11
> Use : instead of + as file separator to avoid + being converted to a space.
> Bug #261616 r=timeless

But that bug is apparently restricted.  :-/

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1, change '%2B' to ':' in cvsmailfilter.pl

Don't know who to ask for this review.  
Timeless, feel free to nominate someone else. ;-)

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

I suspect this bug now belongs to a different product/component, since it's
not a change to any web tools, per se.  

Maybe mozilla.org/CVS Administration
or    mozilla.org/Server Ops
?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 215542 [details] [diff] [review]
patch v1, change '%2B' to ':' in cvsmailfilter.pl

This patch was marked r+, and now it isn't.  
Apparently when the bug was moved from webtools to mozilla.org, the review
flags got cleared.  Also appears review flags are not available to be
set for bugs in this product.  So, the question is: 
what is the procedure for making this bug+patch go forward?

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Here's a tax free $100 cash offer to anyone who can get this fixed before 
midnight tonight, california time.  (Offer expires on 11:59:59 PST on 24 
March 2006. Void where prohibited.)  cvs checkin log will determine the winner.

We're seeing MANY NSS checkins a day, mostly compound (involving more than 
one source file) and the present state makes them WAY too difficult to review.

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Committed, after getting justdave's approval.  And don't bother with the money -- seriously, this should have happened faster.

Could you please comment here once you've seen that it's sent email that works?

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Verified.  Fixed.  Many thanks.