gnb at fastmail.fm
Mon Jan 23 21:49:52 EST 2012
I've been told I should do reviews more openly. Ok, here goes.
commit "rename: ensure user owns both source and dest for Bug #3586 workaround"
Ok, but why?
commit "nntpd: use defaultdomain in conjunction with newspostuser to create Reply-To header addresses"
commit "nntpd: when adding newsgroup "post" addresses to Reply-To, check for "poster""
commit "nntpd: fix handling of Followup-To:poster when using From: header"
I find the add_header() function really confusing, and hard to work
out what it's supposed to do. Is there any chance of a comment
and/or some tests?
In add_header(), the variable sep could be a const char *.
+ newdest = buf_release(&buf);
will leak the string, as newdest is never free()d (and indeed in some other branches
of the logic, cannot be). A better solution would be
const char *newdest = NULL;
newdest = buf_cstring(&buf);
Otherwise, looks good. I'm a little surprised we didn't explicitly handle
"Followup-To: poster", it's been around since RFC1036.
commit "nntpd: added 'newsaddheaders' option..."
The documentation doesn't describe what happens if the incoming message
already contains the To: or Reply-To: header fields. From reading the code, I
think they're passed through untouched, perhaps it would be nice to document
the intended semantics.
Otherwise, looks good.
More information about the Cyrus-devel