[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Made commit-email.pl work on both Windows and Unix systems.

From: Max Bowsher <maxb_at_ukf.net>
Date: 2005-01-26 10:21:39 CET

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.


> 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.


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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.