Created attachment 696 [details]
patch agaonst PerMsgStatus.pm

FLAG: I'm not really comfortable with such a long patch for such a small bug.
couldn't we just change
$line .= "\tautolearn=";
to
$line .= " autolearn=";

I agree your patch is good for 2.60, but it seems to be a little complex for
2.51. Even it is well tested as you say, I don't think it's worth the risk, and
it puts us on a slippery slope. I want to see patches to 2.51 as small as possible.

UN-FLAG: Err... never mind. I just re-read the patch, and I see why it's so long
now. I'm not confident enough to OKAY it, but I retract my FLAG. I don't see
this as a bug that _needs_ fixing, as it's purely cosmetic; however, I won't
object to its inclusion.

I could put together a "simpler" patch (less changed lines but even more 
complex code) for 2.50. 

FLAG: well, the first concern is use of sprintf where it's not necessary.the second is: what exactly is the problem you're trying to fix here?  The reason there's a "\t" before the autolearn is that tab is used before the other tags as well, so I used it for autolearn as well.third: the original version is more efficient because it doesn't call Text::Wrap and then unwrap the header down below if the option's set.Unless someone can convince me otherwise, this isn't a bug but a feature request, and therefore it shouldn't be applied against 2.5.

the problem: It's about the tab *after* autolearn, not the one in front; we 
have tabs to indent each wrapped line, but not between fields. Because the 
autolearn field was just inserted before the version field and code is/was so 
ugly, the "wrap-tab" was used instead of a a space. It's probably just a 
cosmetic problem, at least if there's not somebody out there who relies on the 
tabs to occur only at the start of each line (to unwrap the head himself). 
 
sprintf: I used sprintf because it makes it (IMHO) more clear the we create a 
formatted header and we used it in the first line anyway. 
 
efficiency: I thought so, too, but the difference in time is one tenth of a 
second -- on a P100. Not a really measurable hit in efficiency but some gain 
in code cleanness. (The header was partly wrapped and unwrapped before, just 
the call to Text::Wrap was skipped.) 
 
conclusion: I will create a simpler patch for 2.5x, which will just take care 
of the tab instead of a space after autolearn. (It won't be beautiful bit will 
keep the amount of changed lines low.) 

Subject: Re: [SAdev]  Tab instead of space in X-Spam-Status

On Fri, Feb 28, 2003 at 08:39:47AM -0800, bugzilla-daemon@hughes-family.org wrote:
> the problem: It's about the tab *after* autolearn, not the one in front; we 
> have tabs to indent each wrapped line, but not between fields. Because the 

ok, that's a simple fix.

> cosmetic problem, at least if there's not somebody out there who relies on the 
> tabs to occur only at the start of each line (to unwrap the head himself). 

Then it's a bug for them (whitespace is whitespace) IMHO.

> sprintf: I used sprintf because it makes it (IMHO) more clear the we create a 
> formatted header and we used it in the first line anyway. 

Yes, but the first line is formatting something.  The rest of them are
just "%s" which has no formatting.

> efficiency: I thought so, too, but the difference in time is one tenth of a 
> second -- on a P100. Not a really measurable hit in efficiency but some gain 
> in code cleanness. (The header was partly wrapped and unwrapped before, just 
> the call to Text::Wrap was skipped.) 

If it's really 1/10 second, then get those sprintf's out of there!
That's a huge amount of time for those 2 lines of code.

And I don't think it's cleaner at all.  If that were the case, we'd be
using sprintf everywhere and not doing text concat anywhere.

> conclusion: I will create a simpler patch for 2.5x, which will just take care 
> of the tab instead of a space after autolearn. (It won't be beautiful bit will 
> keep the amount of changed lines low.) 

:)

Created attachment 700 [details]
simpler patch against PerMsgStatus.pm

OKAY: 700 looks fine to me.

Perhaps for 2.60 we should just make a single long X-Spam-Status line, and then if fold_headers is 1, we fold the whole thing at once?

Subject: Re:  Tab instead of space in X-Spam-Status

On Friday 28 February 2003 17:45 CET you wrote:
> > cosmetic problem, at least if there's not somebody out there who relies
> > on the tabs to occur only at the start of each line (to unwrap the head
> > himself).
>
> Then it's a bug for them (whitespace is whitespace) IMHO.

Who's got the domain kluge.net, eh? :o)

> > sprintf: I used sprintf because it makes it (IMHO) more clear the we
> > create a formatted header and we used it in the first line anyway.
>
> Yes, but the first line is formatting something.  The rest of them are
> just "%s" which has no formatting.

Not really. I prefer 
    $line .= sprintf("autolearn=%s ",
               $self->{auto_learn_status} ? "spam" : "ham"
             );
over
    $line .= "autolearn=";
    $line .= $self->{auto_learn_status} ? "spam" : "ham";
    $line .= " ";
But that's probably another question fo taste.

> > efficiency: I thought so, too, but the difference in time is one tenth
> > of a second -- on a P100. Not a really measurable hit in efficiency but
> > some gain in code cleanness. (The header was partly wrapped and
> > unwrapped before, just the call to Text::Wrap was skipped.)
>
> If it's really 1/10 second, then get those sprintf's out of there!
> That's a huge amount of time for those 2 lines of code.

Errr... make that 1/100. On a P100 where one SA run takes >10s.

> And I don't think it's cleaner at all.  If that were the case, we'd be
> using sprintf everywhere and not doing text concat anywhere.

Not "everywhere" but we should do so everywhere where there is a formatted 
string put together. IMHO.

> Perhaps for 2.60 we should just make a single long X-Spam-Status line,
> and then if fold_headers is 1, we fold the whole thing at once?

Then we'd really confuse the people relying on the current style (having the 
first three items in the first line, tests in the second+, etc.)

Cheers,
Malte

Subject: Re: [SAdev]  Tab instead of space in X-Spam-Status

On Fri, Feb 28, 2003 at 09:29:06AM -0800, bugzilla-daemon@hughes-family.org wrote:
> > Then it's a bug for them (whitespace is whitespace) IMHO.
> 
> Who's got the domain kluge.net, eh? :o)

Some of my code doesn't work on it?  Tell me which one, I'll go fix it. :)

> Then we'd really confuse the people relying on the current style (having the 
> first three items in the first line, tests in the second+, etc.)

I'm starting to think we ought to just break out all of the headers into
seperate X-SpamAssassin-* headers.  score, version, tests, etc.  <shrug>

Subject: Re:  Tab instead of space in X-Spam-Status

On Friday 28 February 2003 18:40 CET you wrote:
> > Who's got the domain kluge.net, eh? :o)
>
> Some of my code doesn't work on it?  Tell me which one, I'll go fix it.

I meant something like "never underestimate a klu(d)ge" ;-)

> > Then we'd really confuse the people relying on the current style
> > (having the first three items in the first line, tests in the second+,
> > etc.)
>
> I'm starting to think we ought to just break out all of the headers into
> seperate X-SpamAssassin-* headers.  score, version, tests, etc.  <shrug>

I think it's allright the way we've got it now.

Subject: Re: [SAdev]  Tab instead of space in X-Spam-Status

On Fri, Feb 28, 2003 at 09:53:24AM -0800, bugzilla-daemon@hughes-family.org wrote:
> > I'm starting to think we ought to just break out all of the headers into
> > seperate X-SpamAssassin-* headers.  score, version, tests, etc.  <shrug>
> 
> I think it's allright the way we've got it now.

Yeah, I'm just trying to think ahead.  There'll inevitably be a time
where we have more information to put in X-Spam-Status, and we'll have
to figure out how it should go in, and fold it, and ...  <shrug>

Subject: Re:  Tab instead of space in X-Spam-Status

On Friday 28 February 2003 18:55 CET you wrote:
> > > I'm starting to think we ought to just break out all of the headers
> > > into seperate X-SpamAssassin-* headers.  score, version, tests, etc. 
> > > <shrug>
> >
> > I think it's allright the way we've got it now.
>
> Yeah, I'm just trying to think ahead.  There'll inevitably be a time
> where we have more information to put in X-Spam-Status, and we'll have
> to figure out how it should go in, and fold it, and ...  <shrug>

Hmmm... yeah. I think when we've got so many fields it's ok to do it like 
this:

x-spam-status = flag ", " "hits=" hpoints " " "required=" rpoints "\n"
                "\t" "tests=" test-list "\n"
                [ other-fields " " ] "version=" ver
flag      = "Yes" | "No"
test-list = test [ "," test ]* [ "," "\n"
                "\t     " test [ "," test ]* ]*
other-fields = name "=" value [ " " name "=" value ]* [ "\n"
                "\t" name "=" value [ " " name "=" value ]* ]*

Or, to make it short: You can always assume the first line and the block of 
tests to stay the same and the version to be the last field. But in between 
may occur any combination of folded name=value pairs. Folding is always 
done by "\n\t", fields are separated with spaces. That should be quite 
forward-compatible.

appied to STABLE