Re: [PATCH v2] send-email: Much readable error output

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Dec 27, 2014 at 6:09 AM, Alexander Kuleshov
<kuleshovmail@gmail.com> wrote:
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>

This patch has two distinct goals. First, it's adding "\n" to 'die'
messages to suppress the file+line# information which Perl appends
automatically to 'die' output. Second, it's trying to format the "SMTP
initialization" error message in a more human-readable way. The two
goals should be covered by two patches, rather than just one.

When you split your change into multiple patches, reviewers don't need
to divide their concentration between unrelated changes. Also, if the
project maintainer (Junio) decides that some of the changes are
worthwhile, but others are not, having multiple patches makes it
easier for him to pick and choose the ones he wants.

It's also worthwhile to write a suitable commit message explaining the
change if it's not obvious. For instance, the casual reader might not
realize that adding "\n" to 'die' messages suppresses the file+line#
information, and that you want to suppress that information because it
does not particularly add value to these user-facing message.

More below.

> ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 82c6fea..eb02ef9 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -38,7 +38,7 @@ sub new {
>  }
>  sub readline {
>         my $self = shift;
> -       die "Cannot use readline on FakeTerm: $$self";
> +       die "Cannot use readline on FakeTerm: $$self\n";
>  }
>  package main;
>
> @@ -1274,11 +1274,11 @@ X-Mailer: git-send-email $gitversion
>                 }
>
>                 if (!$smtp) {
> -                       die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
> -                           "VALUES: server=$smtp_server ",
> -                           "encryption=$smtp_encryption ",
> -                           "hello=$smtp_domain",
> -                           defined $smtp_server_port ? " port=$smtp_server_port" : "";
> +                       die "Unable to initialize SMTP properly. Check config and use --smtp-debug.\n",
> +                           "VALUES: server=$smtp_server\n\t",
> +                           "encryption=$smtp_encryption\n\t",
> +                           "hello=$smtp_domain\n\t",
> +                           defined $smtp_server_port ? "port=$smtp_server_port\n" : "\n";

Thomas made a couple points in his review[1] which are very much worth
considering:

1. Move the "\t" to the beginning of the lines you want to indent;
don't leave it at the end of the preceding line following the "\n".

2. Don't emit an unnecessary blank line if $smtp_server_port is not defined.

A third point he didn't mention:

3. If you're indenting "encryption", "hello", and "port", for
consistency, it also makes sense to indent "server". (That is, insert
a newline after "VALUES:", before "server=".)

[1]: http://article.gmane.org/gmane.comp.version-control.git/261833

>                 }
>
>                 smtp_auth_maybe or die $smtp->message;