{"thread":{"id":"38252","subject":"[PATCH] send-email: Much readable error output","startedAt":"2014-12-26T12:06:39Z","lastAt":"2014-12-28T08:56:37Z","messageCount":4,"participants":["Alexander Kuleshov","Thomas Gummerer","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"254102","messageId":"1419595599-19974-1-git-send-email-kuleshovmail@gmail.com","threadId":"38252","inReplyTo":null,"subject":"[PATCH] send-email: Much readable error output","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-12-26T12:06:39Z","receivedAt":"2014-12-26T12:06:39Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Signed-off-by: 0xAX <kuleshovmail@gmail.com>\n---\n git-send-email.perl | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 82c6fea..6a995c4 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1274,11 +1274,11 @@ X-Mailer: git-send-email $gitversion\n \t\t}\n \n \t\tif (!$smtp) {\n-\t\t\tdie \"Unable to initialize SMTP properly. Check config and use --smtp-debug. \",\n-\t\t\t    \"VALUES: server=$smtp_server \",\n-\t\t\t    \"encryption=$smtp_encryption \",\n-\t\t\t    \"hello=$smtp_domain\",\n-\t\t\t    defined $smtp_server_port ? \" port=$smtp_server_port\" : \"\";\n+\t\t\tdie \"Unable to initialize SMTP properly. Check config and use --smtp-debug.\\n\",\n+\t\t\t    \"VALUES: server=$smtp_server\\n\\t\",\n+\t\t\t    \"encryption=$smtp_encryption\\n\\t\",\n+\t\t\t    \"hello=$smtp_domain\\n\\t\",\n+\t\t\t    defined $smtp_server_port ? \"port=$smtp_server_port\\n\" : \"\\n\";\n \t\t}\n \n \t\tsmtp_auth_maybe or die $smtp->message;\n-- \n2.2.1.202.g98acd41\n"},{"id":"254108","messageId":"87k31e56dy.fsf@gmail.com","threadId":"38252","inReplyTo":"1419595599-19974-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH] send-email: Much readable error output","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2014-12-26T23:38:17Z","receivedAt":"2014-12-26T23:38:17Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Hi,\n\nAlexander Kuleshov <kuleshovmail@gmail.com> writes:\n\n> Signed-off-by: 0xAX <kuleshovmail@gmail.com>\n\nPlease use your full name for the sign-off.\n\n> ---\n>  git-send-email.perl | 10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 82c6fea..6a995c4 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1274,11 +1274,11 @@ X-Mailer: git-send-email $gitversion\n>  \t\t}\n>\n>  \t\tif (!$smtp) {\n> -\t\t\tdie \"Unable to initialize SMTP properly. Check config and use --smtp-debug. \",\n> -\t\t\t    \"VALUES: server=$smtp_server \",\n> -\t\t\t    \"encryption=$smtp_encryption \",\n> -\t\t\t    \"hello=$smtp_domain\",\n> -\t\t\t    defined $smtp_server_port ? \" port=$smtp_server_port\" : \"\";\n> +\t\t\tdie \"Unable to initialize SMTP properly. Check config and use --smtp-debug.\\n\",\n> +\t\t\t    \"VALUES: server=$smtp_server\\n\\t\",\n> +\t\t\t    \"encryption=$smtp_encryption\\n\\t\",\n> +\t\t\t    \"hello=$smtp_domain\\n\\t\",\n> +\t\t\t    defined $smtp_server_port ? \"port=$smtp_server_port\\n\" : \"\\n\";\n\nMinor nits:\n  - I think this would be easier to read if the \\t was on the line which\n    should be indented.\n  - If $smtp_server_port is not defined we produce an unnecessary empty line\n\n>  \t\t}\n>\n>  \t\tsmtp_auth_maybe or die $smtp->message;\n> --\n> 2.2.1.202.g98acd41\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"254110","messageId":"1419678577-19340-1-git-send-email-kuleshovmail@gmail.com","threadId":"38252","inReplyTo":"1419595599-19974-1-git-send-email-kuleshovmail@gmail.com","subject":"[PATCH v2] send-email: Much readable error output","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-12-27T11:09:37Z","receivedAt":"2014-12-27T11:09:37Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>\n---\n git-send-email.perl | 36 ++++++++++++++++++------------------\n 1 file changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 82c6fea..eb02ef9 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -38,7 +38,7 @@ sub new {\n }\n sub readline {\n \tmy $self = shift;\n-\tdie \"Cannot use readline on FakeTerm: $$self\";\n+\tdie \"Cannot use readline on FakeTerm: $$self\\n\";\n }\n package main;\n \n@@ -185,13 +185,13 @@ sub do_edit {\n \t\tmap {\n \t\t\tsystem('sh', '-c', $editor.' \"$@\"', $editor, $_);\n \t\t\tif (($? & 127) || ($? >> 8)) {\n-\t\t\t\tdie(\"the editor exited uncleanly, aborting everything\");\n+\t\t\t\tdie(\"the editor exited uncleanly, aborting everything\\n\");\n \t\t\t}\n \t\t} @_;\n \t} else {\n \t\tsystem('sh', '-c', $editor.' \"$@\"', $editor, @_);\n \t\tif (($? & 127) || ($? >> 8)) {\n-\t\t\tdie(\"the editor exited uncleanly, aborting everything\");\n+\t\t\tdie(\"the editor exited uncleanly, aborting everything\\n\");\n \t\t}\n \t}\n }\n@@ -546,7 +546,7 @@ while (defined(my $f = shift @ARGV)) {\n \t\t@ARGV = ();\n \t} elsif (-d $f and !is_format_patch_arg($f)) {\n \t\topendir my $dh, $f\n-\t\t\tor die \"Failed to opendir $f: $!\";\n+\t\t\tor die \"Failed to opendir $f: $!\\n\";\n \n \t\tpush @files, grep { -f $_ } map { catfile($f, $_) }\n \t\t\t\tsort readdir $dh;\n@@ -591,7 +591,7 @@ sub get_patch_subject {\n \t\treturn \"GIT: $1\\n\";\n \t}\n \tclose $fh;\n-\tdie \"No subject line in $fn ?\";\n+\tdie \"No subject line in $fn ?\\n\";\n }\n \n if ($compose) {\n@@ -601,7 +601,7 @@ if ($compose) {\n \t\ttempfile(\".gitsendemail.msg.XXXXXX\", DIR => $repo->repo_path()) :\n \t\ttempfile(\".gitsendemail.msg.XXXXXX\", DIR => \".\"))[1];\n \topen my $c, \">\", $compose_filename\n-\t\tor die \"Failed to open for writing $compose_filename: $!\";\n+\t\tor die \"Failed to open for writing $compose_filename: $!\\n\";\n \n \n \tmy $tpl_sender = $sender || $repoauthor || $repocommitter || '';\n@@ -1205,7 +1205,7 @@ X-Mailer: git-send-email $gitversion\n \t\t$_ = ask(\"Send this email? ([y]es|[n]o|[q]uit|[a]ll): \",\n \t\t         valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,\n \t\t         default => $ask_default);\n-\t\tdie \"Send this email reply required\" unless defined $_;\n+\t\tdie \"Send this email reply required\\n\" unless defined $_;\n \t\tif (/^n/i) {\n \t\t\treturn 0;\n \t\t} elsif (/^q/i) {\n@@ -1231,7 +1231,7 @@ X-Mailer: git-send-email $gitversion\n \t} else {\n \n \t\tif (!defined $smtp_server) {\n-\t\t\tdie \"The required SMTP server is not properly defined.\"\n+\t\t\tdie \"The required SMTP server is not properly defined.\\n\"\n \t\t}\n \n \t\tif ($smtp_encryption eq 'ssl') {\n@@ -1274,11 +1274,11 @@ X-Mailer: git-send-email $gitversion\n \t\t}\n \n \t\tif (!$smtp) {\n-\t\t\tdie \"Unable to initialize SMTP properly. Check config and use --smtp-debug. \",\n-\t\t\t    \"VALUES: server=$smtp_server \",\n-\t\t\t    \"encryption=$smtp_encryption \",\n-\t\t\t    \"hello=$smtp_domain\",\n-\t\t\t    defined $smtp_server_port ? \" port=$smtp_server_port\" : \"\";\n+\t\t\tdie \"Unable to initialize SMTP properly. Check config and use --smtp-debug.\\n\",\n+\t\t\t    \"VALUES: server=$smtp_server\\n\\t\",\n+\t\t\t    \"encryption=$smtp_encryption\\n\\t\",\n+\t\t\t    \"hello=$smtp_domain\\n\\t\",\n+\t\t\t    defined $smtp_server_port ? \"port=$smtp_server_port\\n\" : \"\\n\";\n \t\t}\n \n \t\tsmtp_auth_maybe or die $smtp->message;\n@@ -1321,7 +1321,7 @@ $subject = $initial_subject;\n $message_num = 0;\n \n foreach my $t (@files) {\n-\topen my $fh, \"<\", $t or die \"can't open file $t\";\n+\topen my $fh, \"<\", $t or die \"can't open file $t\\n\";\n \n \tmy $author = undef;\n \tmy $sauthor = undef;\n@@ -1543,7 +1543,7 @@ sub recipients_cmd {\n \n \tmy @addresses = ();\n \topen my $fh, \"-|\", \"$cmd \\Q$file\\E\"\n-\t    or die \"($prefix) Could not execute '$cmd'\";\n+\t    or die \"($prefix) Could not execute '$cmd'\\n\";\n \twhile (my $address = <$fh>) {\n \t\t$address =~ s/^\\s*//g;\n \t\t$address =~ s/\\s*$//g;\n@@ -1554,7 +1554,7 @@ sub recipients_cmd {\n \t\t       $what, $address, $cmd) unless $quiet;\n \t\t}\n \tclose $fh\n-\t    or die \"($prefix) failed to close pipe to '$cmd'\";\n+\t    or die \"($prefix) failed to close pipe to '$cmd'\\n\";\n \treturn @addresses;\n }\n \n@@ -1581,7 +1581,7 @@ sub apply_transfer_encoding {\n \t$message = MIME::Base64::decode($message)\n \t\tif ($from eq 'base64');\n \n-\tdie \"cannot send message as 7bit\"\n+\tdie \"cannot send message as 7bit\\n\"\n \t\tif ($to eq '7bit' and $message =~ /[^[:ascii:]]/);\n \treturn $message\n \t\tif ($to eq '7bit' or $to eq '8bit');\n@@ -1589,7 +1589,7 @@ sub apply_transfer_encoding {\n \t\tif ($to eq 'quoted-printable');\n \treturn MIME::Base64::encode($message, \"\\n\")\n \t\tif ($to eq 'base64');\n-\tdie \"invalid transfer encoding\";\n+\tdie \"invalid transfer encoding\\n\";\n }\n \n sub unique_email_list {\n-- \n2.2.1.202.g98acd41.dirty\n"},{"id":"254129","messageId":"CAPig+cSPOwf2Db2_LJC27T2t+FMdJ-_YtccMFn_0Oybi9-1HJw@mail.gmail.com","threadId":"38252","inReplyTo":"1419678577-19340-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH v2] send-email: Much readable error output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-28T08:56:37Z","receivedAt":"2014-12-28T08:56:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Dec 27, 2014 at 6:09 AM, Alexander Kuleshov\n<kuleshovmail@gmail.com> wrote:\n> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>\n\nThis patch has two distinct goals. First, it's adding \"\\n\" to 'die'\nmessages to suppress the file+line# information which Perl appends\nautomatically to 'die' output. Second, it's trying to format the \"SMTP\ninitialization\" error message in a more human-readable way. The two\ngoals should be covered by two patches, rather than just one.\n\nWhen you split your change into multiple patches, reviewers don't need\nto divide their concentration between unrelated changes. Also, if the\nproject maintainer (Junio) decides that some of the changes are\nworthwhile, but others are not, having multiple patches makes it\neasier for him to pick and choose the ones he wants.\n\nIt's also worthwhile to write a suitable commit message explaining the\nchange if it's not obvious. For instance, the casual reader might not\nrealize that adding \"\\n\" to 'die' messages suppresses the file+line#\ninformation, and that you want to suppress that information because it\ndoes not particularly add value to these user-facing message.\n\nMore below.\n\n> ---\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 82c6fea..eb02ef9 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -38,7 +38,7 @@ sub new {\n>  }\n>  sub readline {\n>         my $self = shift;\n> -       die \"Cannot use readline on FakeTerm: $$self\";\n> +       die \"Cannot use readline on FakeTerm: $$self\\n\";\n>  }\n>  package main;\n>\n> @@ -1274,11 +1274,11 @@ X-Mailer: git-send-email $gitversion\n>                 }\n>\n>                 if (!$smtp) {\n> -                       die \"Unable to initialize SMTP properly. Check config and use --smtp-debug. \",\n> -                           \"VALUES: server=$smtp_server \",\n> -                           \"encryption=$smtp_encryption \",\n> -                           \"hello=$smtp_domain\",\n> -                           defined $smtp_server_port ? \" port=$smtp_server_port\" : \"\";\n> +                       die \"Unable to initialize SMTP properly. Check config and use --smtp-debug.\\n\",\n> +                           \"VALUES: server=$smtp_server\\n\\t\",\n> +                           \"encryption=$smtp_encryption\\n\\t\",\n> +                           \"hello=$smtp_domain\\n\\t\",\n> +                           defined $smtp_server_port ? \"port=$smtp_server_port\\n\" : \"\\n\";\n\nThomas made a couple points in his review[1] which are very much worth\nconsidering:\n\n1. Move the \"\\t\" to the beginning of the lines you want to indent;\ndon't leave it at the end of the preceding line following the \"\\n\".\n\n2. Don't emit an unnecessary blank line if $smtp_server_port is not defined.\n\nA third point he didn't mention:\n\n3. If you're indenting \"encryption\", \"hello\", and \"port\", for\nconsistency, it also makes sense to indent \"server\". (That is, insert\na newline after \"VALUES:\", before \"server=\".)\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/261833\n\n>                 }\n>\n>                 smtp_auth_maybe or die $smtp->message;\n"}]}