Michael Wallendahl wrote:
>
>> I am resubmitting my patch with a proper subject line and minor
>> edits. I also rewrote my log file to follow standards.
>> I believe that having commit-email.pl work out of the box on Windows
>> systems will aid in its adoption.
>
> I am following the suggestions in the HACKING file and resubmitting my
> patch as it has been a while without any responses.
Thanks!
> The commit-email.pl Perl script is an excellent tool and I believe that
> Windows admins would derive an even greater benefit if it ran on Windows
> out of the box. Attached are the necessary modifications to the script
> so that it runs on Windows as well as Unix systems.
>
> The "cost" of these modifications is that the script now requires a few
> additional Perl packages which may or may not already be installed on
> the Unix system (they were found on my FreeBSD 5.3 test box). These
> packages are included with the free Windows ActivePerl distribution.
> The additional packages are File::Temp and Net::SMTP.
And Cwd.
I think File::Temp and Cwd are OK. Further comments on Net::SMTP later.
> Made commit-email.pl work on both Windows and Unix systems:
>
> Three major changes had to be made in order for the script to run under
> Windows as well as Unix:
>
> 1. Eliminate hard coded /tmp path for temporary directory.
> 2. Remove dependency on external sendmail program.
> 3. Rework the 'safe_read_from_pipe' subroutine to not fork under Windows
> systems.
Please could each of these become seperate patches?
It's a lot easier to review 3 seperate concise changes, than 3 interleaved
concepts.
> 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.
> 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?
> Index: commit-email.pl.in
> ===================================================================
> --- commit-email.pl.in (revision 12791)
> +++ commit-email.pl.in (working copy)
...
> @@ -82,7 +101,6 @@
> exit 1 unless $ok;
> }
>
> -
> ######################################################################
> # Initial setup/command-line handling.
>
Avoid whitespace modification in areas you aren't otherwise touching.
...
> +# Create an object that when DESTROY'ed will delete the temporary
> +# 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.
Let's call it something less standard-looking.
What about SVNCommitEmail::Cleanup ?
> @@ -404,11 +449,20 @@
> {
> $subject = "r$rev - $dirlist";
> }
> +
> if ($subject_prefix =~ /\w/)
> {
> $subject = "$subject_prefix $subject";
> }
> +
> my $mail_from = $author;
> + if ((! defined $mail_from) || $mail_from eq "" )
> + {
> + if (! ($anonymousFromAddressPrefix eq ""))
> + {
> + $mail_from = $anonymousFromAddressPrefix;
> + }
> + }
Simplify:
+ if ( $mail_from eq "" )
+ {
+ $mail_from = $anonymousFromAddressPrefix;
+ }
> +# This package exists just to delete the temporary directory.
> +package Temp::Delete;
> +
> +sub new
> +{
> + bless {}, shift;
> +}
Is the "shift" doing anything useful?
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 02:54:33 2005