Michael Wallendahl wrote:
> Thank you for your comments, Max! Replies inline:
>
> Max Bowsher wrote:
>
>> Michael Wallendahl wrote:
...
>>> Removed dependency on 'sendmail' by using Net::SMTP package.
>>> Included code
>>> submitted by Benjamin Garret and Jeff Cave to dev@ list on April 9,
>>> 2004.
>>> Unix systems may be able to just set $mailserver to "localhost".
>>> Requires
>>> Net::SMTP. Adds $mailserver, $mailerDebugLevel,
>>> $anonymousFromAddressPrefix.
>>> Removed check logic for 'sendmail' program as it is not needed now.
>>
>> Not acceptable as is.
>>
>> You can add the additional option to use SMTP instead of sendmail, but
>> you should leave the sendmail functionality intact, and make this a
>> configuration option. The script should not require Net::SMTP when
>> running in sendmail mode.
>
> Ok, I'll start to think of a way to accomplish this.
A couple of "if"s and a "require" instead of "use" should suffice, I think.
> One potentional
> problem I see is that future changes to the sendmail section of the code
> (such as the ongoing talk about limiting the max size of an e-mail)
> would also need to be synchronized in the Net::SMTP section and I am
> worried about it being forgotten.
The message composition code is seperate from the message sending code - it
shouldn't be an issue.
> In the majority of cases though, does setting $mailserver to "localhost"
> provide similar functionality compared to calling 'sendmail' directly?
It's perfectly reasonable for a Unix mail system to have a functional
'sendmail' and delivery process, but no running SMTP listener, if all
externally received email is via POP/IMAP.
>>> Set $mail_from to "subversion" if $author not defined (e.g. by an
>>> anonymous
>>> commit). Still can be overridden by '--from' option as before. Set
>>> $anonymousFromAddressPrefix to "" if you want previous script behavior.
>>> Included code submitted by Benjamin Garret and Jeff Cave to dev@
>>> list on
>>> April 9, 2004.
>>
>> Actually, make that 4 seperate patches.
>>
>> If I'm reading the code right, the previous behaviour would be "From: ",
>> or "From: @your.host.name". Neither seem very useful, so "if you want
>> previous script behavior" is a bit redundant, isn't it?
>
> I'm not married to this change as I don't allow Anonymous commits to my
> Repositories. I'll just revert it back to how the existing script
> behaves.
OK. But note that I'm not opposed to the change, just it's mixing with 3
other changes.
>> Avoid whitespace modification in areas you aren't otherwise touching.
>
> Some areas of the script have a blank line before a
> subroutine/function/logical block, while others don't. Was just aiming
> for consistency, but am happy to leave those parts alone.
Doing overall cleanup is fine, but it's a seperate change for yet another
patch! :-)
> If I ran over 80 characters I rewrapped the line.
Good.
> I do feel that we should put an "end of user configuration section" line
> in there however so that Perl novices will know when to stop modifying
> stuff.
Yes, that's a good idea.
>>> +# This package exists just to delete the temporary directory.
>>> +package Temp::Delete;
>>> +
>>> +sub new
>>> +{
>>> + bless {}, shift;
>>> +}
>>
>> Is the "shift" doing anything useful?
>
> Ahh, you caught me trying to reuse someone else's code without total
> grokking. I will find out for sure if shift is needed, or just rip it
> all out in favor of removing the File::Temp requirement and having the
> user simply configure a variable that points to the preferred temporary
> directory location.
!!!
It would be a bit of a waste to chuck all that code over 1 tiny word!
I'm pretty sure that "bless {};" is sufficient.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jan 26 10:25:04 2005