You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Michael Wallendahl <mw...@spikus.com> on 2005/01/19 22:31:25 UTC
[PATCH] Made commit-email.pl work on both Windows and Unix systems.
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.
Let me know what you think.
-Mike
Re: [PATCH] Made commit-email.pl work on both Windows and Unix systems.
Posted by Max Bowsher <ma...@ukf.net>.
John Peacock wrote:
> Max Bowsher wrote:
>>>>> +# 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.
>>
>
> No, there is a definite point to the two-parameter form of bless().
> Though
> the canonical way to write that is usually:
>
> sub new
> {
> my $proto = shift;
> my $class = ref($proto) || $proto;
>
> return bless (), $class;
> }
>
> The contents of $proto are either going to be an object of the same type,
> if
> the caller was $obj->new(), or it is going to be a string containing the
> name
> of the class this sub is being called by (not necessarily the same class
> as
> the package, since this could be an inherited method). The second line
> determines
> which is which. If you don't provide a class to bless(), it will bless
> the
> object into the current package, which isn't the right thing with
> inherited
> classes.
> However, in this specific case, there is no concern about subclassing I
> think,
> so the single parameter form of bless() is sufficient.
>
> HTH
Indeed it does, thanks for the excellent clarification.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Made commit-email.pl work on both Windows and Unix systems.
Posted by John Peacock <jp...@rowman.com>.
Max Bowsher wrote:
>>>> +# 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.
>
No, there is a definite point to the two-parameter form of bless(). Though the
canonical way to write that is usually:
sub new
{
my $proto = shift;
my $class = ref($proto) || $proto;
return bless (), $class;
}
The contents of $proto are either going to be an object of the same type, if the
caller was $obj->new(), or it is going to be a string containing the name of the
class this sub is being called by (not necessarily the same class as the
package, since this could be an inherited method). The second line determines
which is which. If you don't provide a class to bless(), it will bless the
object into the current package, which isn't the right thing with inherited classes.
However, in this specific case, there is no concern about subclassing I think,
so the single parameter form of bless() is sufficient.
HTH
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4720 Boston Way
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5747
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Made commit-email.pl work on both Windows and Unix systems.
Posted by Max Bowsher <ma...@ukf.net>.
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
Re: [PATCH] Made commit-email.pl work on both Windows and Unix systems.
Posted by Michael Wallendahl <mw...@spikus.com>.
>> 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.
After waiting for additional comments, I will resubmit as smaller
separate patches.
Thanks,
-Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Made commit-email.pl work on both Windows and Unix systems.
Posted by Michael Wallendahl <mw...@spikus.com>.
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
Re: [PATCH] Made commit-email.pl work on both Windows and Unix systems.
Posted by Max Bowsher <ma...@ukf.net>.
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
Re: [PATCH] Made commit-email.pl work on both Windows and Unix systems.
Posted by Michael Wallendahl <mw...@spikus.com>.
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.
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.
Does anyone have any feedback on why this patch should or should not be
included?
Thanks for your time,
-Mike