Thank you for your comments, Max! Replies inline:
Max Bowsher wrote:
> Michael Wallendahl wrote:
>
>> The "cost" of these modifications is that the script now requires a few
>> additional Perl packages <...>
>
> And Cwd.
I believe that Cwd is included in the standard Perl library so it is not
an external dependancy. I will be happy to modify the log message to
include a reference to it, however.
>> Three major changes had to be made in order for the script to run under
>> Windows as well as Unix:
>> <...>
>
> Please could each of these become seperate patches?
> It's a lot easier to review 3 seperate concise changes, than 3
> interleaved concepts.
Good point. I wrestled with the idea of having one "logical" change
like this, or several seperate changes. Splitting them up would make it
easier for people to review as long as I am clear in my log messages
that each mini-patch is only part of the solution. I will wait and see
what other feedback I receive on this.
>> 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. 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.
In the majority of cases though, does setting $mailserver to "localhost"
provide similar functionality compared to calling 'sendmail' directly?
>> 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.
> 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. 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.
>> +# directory. The CLEANUP flag to tempdir should do this, but they
>> +# call rmtree with 1 as the last argument which takes extra security
>> +# measures that do not clean up the .svn directories.
>> +my $tmp_dir_cleanup = Temp::Delete->new; # Package Temp::Delete defined
>> + # at end of file.
>
> +# Create an object that when DESTROY'ed will delete the temporary
>
> Let's call it something less standard-looking.
> What about SVNCommitEmail::Cleanup ?
Sounds good, if we end up keeping the File::Temp code. Perhaps I will
strive for smaller changes as my first contribution to Subversion ever. :)
>> +# 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.
Thanks for your input. I will wait a bit to see if there is additional
input before I submit a new patch.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jan 26 04:50:28 2005