Ryan Anderson <[EMAIL PROTECTED]> writes:

>       All emails are sent as a reply to the previous email, making it easy to
>       skip a collection of emails that are uninteresting.

I understand why _some_ people consider this preferable, but
wonder if this should have a knob to be tweaked.

For example, I myself often find it very hard to read when
a cascading thread goes very deep like this:

     [PATCH 0/9] cover
       [PATCH 1/9] first one
         [PATCH 2/9] second one
          [PATCH 3/9] third one in the series

and prefer to see this instead (this assumes your MUA is
half-way decent and lets you sort by subject):

     [PATCH 0/9] cover
       [PATCH 1/9] first one
       [PATCH 2/9] second one
       [PATCH 3/9] third one in the series

> +# horrible hack of a script to send off a large number of email messages, 
> one after
> +# each other, all chained together.  This is useful for large numbers of 
> patches.
> +#
> +# Use at your own risk!!!!

Well, if it is "Use at your own risk" maybe it should stay
outside the official distribution for a while until it gets
safer ;-).

> +     my @fields = split /\s+/, $data;
> +     my $ident = join(" ", @fields[0...(@fields-3)]);

Wouldn't "s/>.*/>/" be easier than splitting and joining?

> +if (!defined $from) {
> +     $from = $author || $committer;
> +     1 while (!defined ($_ = $term->readline("Who should the emails appear 
> to be from? ", 
> +                             $from)));

Judging from your past patches, you seem to really like
statement modifiers[*].  While they _are_ valid Perl constructs,
it is extremely hard to read when used outside very simple
idiomatic use.  Please consider rewriting the above and the like
using compound statements[*] (I am using these terms according
to the definition in perlsyn.pod).  Remember, there are people
Perl is not their native language, but are intelligent enough to
be of great help fixing problems in programs you write in Perl.
To most of them, compound statements are more familiar, so try
to be gentle to them.

> +             opendir(DH,$f)
> +                     or die "Failed to opendir $f: $!";
> +             push @files, map { +$f . "/" . $_ } grep !/^\.{1,2}$/,
> +                     sort readdir(DH);

Maybe skip potential subdirs while you are at it, something like this?

    push @files, sort grep { -f $_ } map { "$f/$_" } readdir(DH)

> +     my $pseudo_rand = int (rand(4200));
> +     $message_id = "<[EMAIL PROTECTED]>";
> +     print "new message id = $message_id\n";

I doubt this hardcoded foobar.com is a good idea.  Did you mean
to print it, by the way?

> +     $to{lc(Email::Valid->address($_))}++ for (@to);
> +     my $to = join(",", keys %to);

Is this the culprit that produced this mechanical-looking line?

    To: [EMAIL PROTECTED],git@vger.kernel.org

Interestingly enough, you do not seem to do it for the From:

    From: Ryan Anderson <[EMAIL PROTECTED]>

Also you seem to be losing the ordering in @to and @cc by the
use of uniquefying "keys %to" and "keys %cc".  I can not offhand
tell if it matters, but you probably would care, at least for
the primary recipients listed in @to array.

> +     $mail{smtp} = 'localhost';

I suspect this probably need to be configurable.  I may be a
minority, but my outgoing messages are directly handed to my
local ISP smtp server from my MUA, and the smtp server running
on the locahost does not talk to the outside world.

> +     # set up for the next message
> +     $reply_to = $message_id;

Making chaining policy configurable would be just one liner
change here, I suppose.

Since there are always 47 different ways to do the same thing in
Perl, Perl style varies a lot more than Shell style which in
turn varies a lot more than C.  You should be prepared to be
nitpicked a lot when you post Perl ;-).  And I was in nitpicking
mood tonight.

Thanks for the patch.  Overall, very good intent.  Slightly
troublesome details.


To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to