Created attachment 909 [details]
fix

well spotted, thanks for the fix.

OKAY: looks good to me. :)

Created attachment 915 [details]
proposed patch

I fixed this with the proposed patch that I attached, which removes the
score/thresh line.  Otherwise, you always get a score/thresh line in the report,
when it might not be desired in that format, or at all.

Sam, I've just checked in the first patch -- to use get_report() instead of
{test_logs}.  That's OK.

Also, your second patch is correct -- to match 2.4x, the report should NOT
include that line.  I don't know where that came from.  However, to do this in
2.54 would break compat with 2.50-3.  So we need buy-in from the other devs.  folks?

It appears that the score/thresh line prepended to the report in 2.53 spamd is
in support of the -R and -r options to spamc.  The spamc man page indicates this
line always appears at the beginning of the report returned.  If I use spamc -R
against spamd with my patch applied, the line does not appear.  With the
original 2.53 spamd source it does.  It seems more appropriate that this line be
generated by spamc since spamc is not the only client of spamd.

I could try to work up a patch for spamc to correspond with my patch for spamd
to preserve the documented functionality of -R and -r for spamc.  Let me know if
that is the way that you want to go...

I'd rather have incompatibility between major releases than bug-fix releases.
If this is really a bug, lets fix it in 2.60 so that 2.5x is consistent.

But I need to get this bug out of the way for 2.54.  So lets make a decision:
either things are ok now and we close the bug, or the issue needs to be resolved
and it'll be fixed in 2.60 which should get a new ticket.

OK, Sam, you're right -- I'll add some code to get spamc to generate that line,
it shouldn't be too tricky.  So the matrix will be:

2.4x: no -R/-r flags on spamc, REPORT in protocol gives just report, no hdr line
2.5[0-3]: -R/-r supported, -r and REPORT all give report+hdr line
2.54: -R/-r supported, -r gives report+hdr, REPORT gives just report a la 2.4x
2.60: -R/-r supported, -r gives report+hdr, REPORT gives just report a la 2.4x

taking bug

Created attachment 942 [details]
spamc now creates that line, wire protocol 2.4x compatible

okay, this now is:
patch 915: correct patch on spamd side
patch 942: correct patch on spamc side.

It implements what I mentioned in the last comment, ie. spamc adds the line and
the wire-protocol does not include it.  So it's fully 2.4x compatible, but breaks
compat with 2.50, 2.51, 2.52, and 2.53.  But IMO the behaviour in those releases
is a bug.

Dan, Theo, OKAYs?

ISSUE: as stated before, I would like to have all 2.5x releases compatible with
one another unless this is a major enough bug that it warrants the
incompatibility.   I'd rather fix 2.60 and have it be incompatible with 2.5x. 
At least then we can say that as long as the minor version number (Major.Minor,
followed by bug-fix release #  (which we really ought to change into a x.y.z or
something)) is the same, spamc/spamd interoperate, but we don't guarantee
interoperability between major/minor versions.

Subject: Re:  REPORT effectively ignores local config 


bugzilla-daemon@hughes-family.org said:

> ISSUE: as stated before, I would like to have all 2.5x releases
> compatible wi th one another unless this is a major enough bug that it
> warrants the incompatibility.   I'd rather fix 2.60 and have it be
> incompatible with 2.5x.
>  
> At least then we can say that as long as the minor version number
> (Major.Mino r, followed by bug-fix release #  (which we really ought to
> change into a x.y.z or something)) is the same, spamc/spamd
> interoperate, but we don't guarantee interoperability between
> major/minor versions.

See your point -- but there's another issue -- spamd wire-level
compatibility.  Currently (and with your suggestion above) 2.4x is
incompatible with 2.5x, and 2.5x would be incompatible with 2.6x. 

I suggest instead we regard this as a bug in 2.5[0123], so instead it's
"the correct format is what 2.4x, 2.54 and 2.6x use, 2.5[0123] were
buggy".

--j.

BTW, I think this needs a clearer summary matrix, so here it is.  "-R"/"-r"
refers to the spamc usage; REP is the "REPORT" verb in the protocol; RIFS is the
"REPORT_IFSPAM" verb in the protocol.   "spamheader" refers to the "0/5" line
used at top of the -r report in 2.5x/2.6x.

2.4x: -R does not exist, -r does not exist, REPORT is just report, RIFS is
report if spam or "" if not spam.

2.5[0123]: -R is spamheader+report, -r is spamheader+report if spam or "" if not
spam, REPORT is spamheader+report, RIFS is spamheader+report if spam or "" if
not spam.

2.54, Theo's suggestion: -R is spamheader+report, -r is spamheader+report if
spam or "" if not spam, REPORT is spamheader+report, RIFS is spamheader+report
if spam or "" if not spam.

2.54, my suggestion: -R is spamheader+report, -r is spamheader+report if spam or
"" if not spam, REPORT is just report, RIFS is just report if spam or "" if not
spam.

2.60: -R is spamheader+report, -r is spamheader+report if spam or "" if not
spam, REPORT is just report, RIFS is just report if spam or "" if not spam.

So to restate, the question is: should we (a) treat it as a bug and fix it for
2.4x/2.6x compatibility in 2.5x -- or (b) say "2.5x uses a different wire format".

> So to restate, the question is: should we (a) treat it as a bug and fix it for
> 2.4x/2.6x compatibility in 2.5x -- or (b) say "2.5x uses a different wire
> format".

I say: if 2.5x works as it's supposed to with the current code, ignoring
non-2.5x versions, leave the code alone.

That said, if jm and quinlan agree it should change, have at it.  I can see the
"but it's broken in 2.5x because it's different" argument, I would just rather
set the precident that x.yq releases will be compatible with x.yz releases, but
not necessarily with different x or y values.

OK, after discussion, we've agreed on the following.
2.4x and 2.6x will do the "right thing" as discussed.  However the 2.5x
series will maintain internal compatibility, and will continue with
the current behaviour.  We will fix it to use get_report() instead of
the {test_logs} text, so the user's report text is included.

So -- devs -- we need OKAYs on patch 909 (the get_report() one) and a
forthcoming patch which fixes the documentation to properly reflect this.

Created attachment 952 [details]
doco fix

OKAY: 952 looks fine. :)

OKAY: documentation fix looks fine

ok!  2.5x documents it; 2.6x does what 2.4x did.  so fixed