Re: [PATCH RFC3.5 06/12] send-email: Cleanup and streamline the SMTP code in send_message

From: Michael Witten <mfwitten@gmail.com>
Date: Apr 20, 2009, 05:38 UTC
Message-ID: <b4087cc50904192238j744f353dtf5f6a616cada8cd8@mail.gmail.com>
In-Reply-To: <7vskk4nlrg.fsf@gitster.siamese.dyndns.org>

On Sun, Apr 19, 2009 at 20:42, Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

> Michael Witten <mfwitten@gmail.com> writes:
>
>> +                             die "Server does not support STARTTLS: " . $smtp->message . "\n"
>> +                                     unless $smtp->code == 220;
>
> ...
> But the following, which is equivalent to what you did, is inexcuable.
>
>        do this;
>        do that;
>        do something unusual
>                if some condition that rarely holds true;
>        do some other thing;
>
> When your eyes and brain are coasting over this segment of code, your
> thought process needs to stumble and hiccup at the statment that does
> something unusual, and then need to realize that it is qualified with a
> statement modifier that says "this is only for rare case".

I mostly agree, and I frequently consider[ed] exactly those points. However, there are 2 things that played a role in my decision:

    * For most conditional cases, I personally
      loathe curly braces around one statement.

    * The flow is actually:

            do this;
            do that;

            DIE "whisper some curses with the last breath"
                UNLESS some condition that holds mostly true;

            do some other thing;

      The "die" and thoughtful spacing should be pretty good clues.
      However, the "unless" can be strange to think with (at first);
      I figured Perlers would be happy with it.

In any case, I also like:

    condition and/or (do something);

or:

    condition and/or do something;

The only thing keeping me from using that more often is that I assume other people would be less comfortable with it and that it may introduce an unnecessary comparison of the return value of "do something"; also, it might make the line a little long, which some people get really angry about.

Show 8 quoted lines

> Written without statement modifier:
>
>        do this;
>        do that;
>        if (some consition that rarely holds true) {
>                do something unusual
>        }
>        do some other thing;

I just have a hard time stomaching those curly braces. I really wish perl didn't enforce them when there's only one statement. Also, I would use some whitespace:

    do this;
    do that;

    if (some consition that rarely holds true) {
        do something unusual
    }

    do some other thing;

Show 9 quoted lines

>> +             $smtp->mail($raw_from)               and
>> +             $smtp->to(@recipients)               and
>> +             $smtp->data                          and
>> +             $smtp->datasend("$header\n$message") and
>> +             $smtp->dataend                       or
>> +
>> +             die "Failed to send '$subject': " . $smtp->message . "\n";
>
> These do make things more pleasant to read.

Thanks!

P.S.

Sorry if the formatting of this email is bad; I'm in the middle of a large move between systems, and currently I'm stuck with gmail's webmail, which insists on reformatting my text and refusing to render in fixed-width font (though I bet I could hack firefox's css to get that one working.... hmmm.....), and firefox doesn't make it easy to input tabs.

So, I've actually been writing and sending some emails with a combination of:

    * vim
    * date +'%a, %e %b %Y %T %z'
    * uuidgen (though I've found gmail makes a Message-ID for me)
    * cat path/to/email.txt | perl -pe 's/\n/\r\n/; END {print
"\r\n"}' | msmtp -t

This email was written in the webmail in firefox; I actually counted spaces for indentation in the hope that things line up. ;-)