Bug 5049 - [review] sa-update should allow comments in channelfiles
Summary: [review] sa-update should allow comments in channelfiles
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: sa-update (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: Other other
: P5 normal
Target Milestone: 3.1.5
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard: ready to commit
Keywords:
Depends on:
Blocks:
 
Reported: 2006-08-14 06:03 UTC by Daryl C. W. O'Shea
Modified: 2006-08-22 16:31 UTC (History)
0 users


Attachment Type Modified Status Actions Submitter/CLA Status
suggested patch patch None Theo Van Dinter [HasCLA]
alternative patch patch None Daryl C. W. O'Shea [HasCLA]
better patch patch None Daryl C. W. O'Shea [HasCLA]
Show Obsolete (1) Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Daryl C. W. O'Shea 2006-08-14 06:03:56 UTC 
sa-update should allow for comments in channelfiles.

The same goes for gpgkeyfiles.  I assume, but haven't checked, that comments
aren't supported in that file either.
Comment 1 Theo Van Dinter 2006-08-16 03:00:21 UTC 
Created attachment 3653 [details]
suggested patch
Comment 2 Justin Mason 2006-08-20 19:31:09 UTC 
+1
Comment 3 Sidney Markowitz 2006-08-20 19:54:19 UTC 
+1
Comment 4 Theo Van Dinter 2006-08-20 21:07:06 UTC 
Sending        sa-update.raw
Transmitting file data .
Committed revision 433045.
Comment 5 Daryl C. W. O'Shea 2006-08-21 04:02:00 UTC 
Created attachment 3662 [details]
alternative patch

sorry, still way behind having to run my friend's theatre chain...

The committed patch doesn't handle blank lines, leading whitespace, or
commments at the end of a line.

This patch handles the above, but doesn't deal with someone wanting a # to be
included.  I can't think of a reason why you'd need it though, so I didn't
bother supporting something like \# as we do with other config files.
Comment 6 Daryl C. W. O'Shea 2006-08-21 04:03:00 UTC 
reopening
Comment 7 Sidney Markowitz 2006-08-21 04:10:54 UTC 
I agree with that

+1
Comment 8 Justin Mason 2006-08-21 23:14:07 UTC 
well, since you've done it: +1
Comment 9 Theo Van Dinter 2006-08-22 01:27:20 UTC 
I'm -0.5.  The RE being used is a bit confusing:

s/\s*(?:#.*)?//g

why not:

s/\s*\#.*$//

The first RE seems questionable, since for instance, it'll always match on every
string (though it may not actually do anything).

Also:

+    next unless $key;		# skip blank lines (that remain)

would then have to change to:

next if ($key =~ /^\s*$/);

or

next unless ($key =~ /\S/);
Comment 10 Daryl C. W. O'Shea 2006-08-22 07:11:35 UTC 
Created attachment 3664 [details]
better patch

Fair enough.  Your alternative isn't workable, though, since it doesn't handle
leading whitespace and trailing whitespace in the absence of a comment on the
line.

How about we just do what we do in Parser.pm, except leave out support for
hashes since they'll never be needed/aren't valid.

This patch also fixes the channel validation (if one wasn't valid, the next one
wasn't checked).  It also dbg's that a channel isn't valid and is being
skipped.
Comment 11 Sidney Markowitz 2006-08-22 08:33:53 UTC 
That looks a lot clearer to me.

+1 unless someone jumps in with a yet cleaner version.
Comment 12 Justin Mason 2006-08-22 14:17:47 UTC 
+1
Comment 13 Theo Van Dinter 2006-08-22 15:23:09 UTC 
sure, +1
Comment 14 Daryl C. W. O'Shea 2006-08-22 23:31:29 UTC 
[dos@FC5-VPC 3.1]$ svn ci -m "bug 5049: handle comments and whitespace in
sa-update config files and fix an error in channel name validation"
Sending        sa-update.raw
Transmitting file data .
Committed revision 433797.

[dos@FC5-VPC trunk]$ svn ci -m "bug 5049: handle comments and whitespace in
sa-update config files and fix an error in channel name validation"
Sending        sa-update.raw
Transmitting file data .
Committed revision 433798.
[dos@FC5-VPC trunk]$