{"thread":{"id":"32889","subject":"[PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe","startedAt":"2013-02-12T14:02:27Z","lastAt":"2013-02-27T16:29:26Z","messageCount":22,"participants":["Michal Nazarewicz","Junio C Hamano","Jeff King","Matthieu Moy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"209367","messageId":"cover.1360677646.git.mina86@mina86.com","threadId":"32889","inReplyTo":null,"subject":"[PATCHv4 0/6] git-credential support in git-send-email","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-12T14:02:27Z","receivedAt":"2013-02-12T14:02:27Z","isPatch":false,"sender":{"key":"mpn@google.com","avatar":null},"body":"From: Michal Nazarewicz \n\nBesids git-credential support in git-send-email, there are some other\nminor improvements to Git.pm in this patchset. Patch 3/6 is new\ncompared to the previous patchset.\n\nMichal Nazarewicz (6):\n Git.pm: allow command_close_bidi_pipe to be called as method\n Git.pm: fix example in command_close_bidi_pipe documentation\n Git.pm: refactor command_close_bidi_pipe to use _cmd_close\n Git.pm: allow pipes to be closed prior to calling\n command_close_bidi_pipe\n Git.pm: add interface for git credential command\n git-send-email: use git credential to obtain password\n\n Documentation/git-send-email.txt | 4 +-\n git-send-email.perl | 59 +++++++-----\n perl/Git.pm | 198 ++++++++++++++++++++++++++++++++++-----\n 3 files changed, 213 insertions(+), 48 deletions(-)\n\n-- \n1.8.1.3.572.g32bae1f\n"},{"id":"209370","messageId":"df0bb01e70629e8170b022867c6e70a8d1b88768.1360677646.git.mina86@mina86.com","threadId":"32889","inReplyTo":"cover.1360677646.git.mina86@mina86.com","subject":"[PATCHv4 1/6] Git.pm: allow command_close_bidi_pipe to be called as method","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-12T14:02:28Z","receivedAt":"2013-02-12T14:02:28Z","isPatch":false,"sender":{"key":"mpn@google.com","avatar":null},"body":"From: Michal Nazarewicz \n\nThe documentation of command_close_bidi_pipe() claims that it can\nbe called as a method, but it does not check whether the first\nargument is $self or not assuming the latter. Using _maybe_self()\nfixes this.\n\nSigned-off-by: Michal Nazarewicz \n---\n perl/Git.pm | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 931047c..bbb753a 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -430,7 +430,7 @@ have more complicated structure.\n \n sub command_close_bidi_pipe {\n \tlocal $?;\n-\tmy ($pid, $in, $out, $ctx) = @_;\n+\tmy ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);\n \tforeach my $fh ($in, $out) {\n \t\tunless (close $fh) {\n \t\t\tif ($!) {\n-- \n1.8.1.3.572.g32bae1f\n"},{"id":"209368","messageId":"d8be058cc4c9a3baa68c166fd9f3333e93e3583e.1360677646.git.mina86@mina86.com","threadId":"32889","inReplyTo":"cover.1360677646.git.mina86@mina86.com","subject":"[PATCHv4 2/6] Git.pm: fix example in command_close_bidi_pipe documentation","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-12T14:02:29Z","receivedAt":"2013-02-12T14:02:29Z","isPatch":false,"sender":{"key":"mpn@google.com","avatar":null},"body":"From: Michal Nazarewicz \n\n\nSigned-off-by: Michal Nazarewicz \n---\n perl/Git.pm | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex bbb753a..11f310a 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -418,7 +418,7 @@ and it is the fourth value returned by C. The call idiom\n is:\n \n \tmy ($pid, $in, $out, $ctx) = $r->command_bidi_pipe('cat-file --batch-check');\n-\tprint \"000000000\\n\" $out;\n+\tprint $out \"000000000\\n\";\n \twhile (<$in>) { ... }\n \t$r->command_close_bidi_pipe($pid, $in, $out, $ctx);\n \n-- \n1.8.1.3.572.g32bae1f\n"},{"id":"209369","messageId":"fc760829f74f31d23f94b61a9e087eda2a66956e.1360677646.git.mina86@mina86.com","threadId":"32889","inReplyTo":"cover.1360677646.git.mina86@mina86.com","subject":"[PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-12T14:02:30Z","receivedAt":"2013-02-12T14:02:30Z","isPatch":false,"sender":{"key":"mpn@google.com","avatar":null},"body":"From: Michal Nazarewicz \n\nThe body of the loop in command_close_bidi_pipe function is identical to\nwhat _cmd_close function does so instead of duplicating, refactor change\n_cmd_close so that it accepts list of file handlers to be closed, which\nmakes it usable with command_close_bidi_pipe.\n\nSigned-off-by: Michal Nazarewicz \n---\n perl/Git.pm | 30 +++++++++++-------------------\n 1 file changed, 11 insertions(+), 19 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 11f310a..6bc9a3c 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -267,13 +267,13 @@ sub command {\n \n \tif (not defined wantarray) {\n \t\t# Nothing to pepper the possible exception with.\n-\t\t_cmd_close($fh, $ctx);\n+\t\t_cmd_close($ctx, $fh);\n \n \t} elsif (not wantarray) {\n \t\tlocal $/;\n \t\tmy $text = <$fh>;\n \t\ttry {\n-\t\t\t_cmd_close($fh, $ctx);\n+\t\t\t_cmd_close($ctx, $fh);\n \t\t} catch Git::Error::Command with {\n \t\t\t# Pepper with the output:\n \t\t\tmy $E = shift;\n@@ -286,7 +286,7 @@ sub command {\n \t\tmy @lines = <$fh>;\n \t\tdefined and chomp for @lines;\n \t\ttry {\n-\t\t\t_cmd_close($fh, $ctx);\n+\t\t\t_cmd_close($ctx, $fh);\n \t\t} catch Git::Error::Command with {\n \t\t\tmy $E = shift;\n \t\t\t$E->{'-outputref'} = \\@lines;\n@@ -313,7 +313,7 @@ sub command_oneline {\n \tmy $line = <$fh>;\n \tdefined $line and chomp $line;\n \ttry {\n-\t\t_cmd_close($fh, $ctx);\n+\t\t_cmd_close($ctx, $fh);\n \t} catch Git::Error::Command with {\n \t\t# Pepper with the output:\n \t\tmy $E = shift;\n@@ -381,7 +381,7 @@ have more complicated structure.\n sub command_close_pipe {\n \tmy ($self, $fh, $ctx) = _maybe_self(@_);\n \t$ctx ||= '';\n-\t_cmd_close($fh, $ctx);\n+\t_cmd_close($ctx, $fh);\n }\n \n =item command_bidi_pipe ( COMMAND [, ARGUMENTS... ] )\n@@ -431,18 +431,8 @@ have more complicated structure.\n sub command_close_bidi_pipe {\n \tlocal $?;\n \tmy ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);\n-\tforeach my $fh ($in, $out) {\n-\t\tunless (close $fh) {\n-\t\t\tif ($!) {\n-\t\t\t\tcarp \"error closing pipe: $!\";\n-\t\t\t} elsif ($? >> 8) {\n-\t\t\t\tthrow Git::Error::Command($ctx, $? >>8);\n-\t\t\t}\n-\t\t}\n-\t}\n-\n+\t_cmd_close($ctx, $in, $out);\n \twaitpid $pid, 0;\n-\n \tif ($? >> 8) {\n \t\tthrow Git::Error::Command($ctx, $? >>8);\n \t}\n@@ -1355,9 +1345,11 @@ sub _execv_git_cmd { exec('git', @_); }\n \n # Close pipe to a subprocess.\n sub _cmd_close {\n-\tmy ($fh, $ctx) = @_;\n-\tif (not close $fh) {\n-\t\tif ($!) {\n+\tmy $ctx = shift @_;\n+\tforeach my $fh (@_) {\n+\t\tif (close $fh) {\n+\t\t\t# nop;\n+\t\t} elsif ($!) {\n \t\t\t# It's just close, no point in fatalities\n \t\t\tcarp \"error closing pipe: $!\";\n \t\t} elsif ($? >> 8) {\n-- \n1.8.1.3.572.g32bae1f\n"},{"id":"209365","messageId":"3bb6b7736eb4b0a958469be13d8c646faec1208a.1360677646.git.mina86@mina86.com","threadId":"32889","inReplyTo":"cover.1360677646.git.mina86@mina86.com","subject":"[PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-12T14:02:31Z","receivedAt":"2013-02-12T14:02:31Z","isPatch":false,"sender":{"key":"mpn@google.com","avatar":null},"body":"From: Michal Nazarewicz \n\nThe command_close_bidi_pipe() function will insist on closing both\ninput and output pipes returned by command_bidi_pipe(). With this\nchange it is possible to close one of the pipes in advance and\npass undef as an argument.\n\nSigned-off-by: Michal Nazarewicz \n---\n perl/Git.pm | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 6bc9a3c..d6e6c9e 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -426,12 +426,25 @@ Note that you should not rely on whatever actually is in C;\n currently it is simply the command name but in future the context might\n have more complicated structure.\n \n+C and C may be C if they have been closed prior to\n+calling this function. This may be useful in a query-response type of\n+commands where caller first writes a query and later reads response, eg:\n+\n+\tmy ($pid, $in, $out, $ctx) = $r->command_bidi_pipe('cat-file --batch-check');\n+\tprint $out \"000000000\\n\";\n+\tclose $out;\n+\twhile (<$in>) { ... }\n+\t$r->command_close_bidi_pipe($pid, $in, undef, $ctx);\n+\n+This idiom may prevent potential dead locks caused by data sent to the output\n+pipe not being flushed and thus not reaching the executed command.\n+\n =cut\n \n sub command_close_bidi_pipe {\n \tlocal $?;\n \tmy ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);\n-\t_cmd_close($ctx, $in, $out);\n+\t_cmd_close($ctx, grep defined, $in, $out);\n \twaitpid $pid, 0;\n \tif ($? >> 8) {\n \t\tthrow Git::Error::Command($ctx, $? >>8);\n-- \n1.8.1.3.572.g32bae1f\n"},{"id":"209371","messageId":"e5834c9b5ccb66a88b64b3d07982ad41205fb97e.1360677646.git.mina86@mina86.com","threadId":"32889","inReplyTo":"cover.1360677646.git.mina86@mina86.com","subject":"[PATCHv4 5/6] Git.pm: add interface for git credential command","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-12T14:02:32Z","receivedAt":"2013-02-12T14:02:32Z","isPatch":false,"sender":{"key":"mpn@google.com","avatar":null},"body":"From: Michal Nazarewicz \n\nAdd a credential() function which is an interface to the git\ncredential command. The code is heavily based on credential_*\nfunctions in .\n\nSigned-off-by: Michal Nazarewicz \n---\n perl/Git.pm | 151 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 151 insertions(+)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex d6e6c9e..a24458c 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -59,6 +59,7 @@ require Exporter;\n command_bidi_pipe command_close_bidi_pipe\n version exec_path html_path hash_object git_cmd_try\n remote_refs prompt\n+ credential credential_read credential_write\n temp_acquire temp_release temp_reset temp_path);\n \n \n@@ -1003,6 +1004,156 @@ sub _close_cat_blob {\n }\n \n \n+=item credential_read( FILEHANDLE )\n+\n+Reads credential key-value pairs from C. Reading stops at EOF or\n+when an empty line is encountered. Each line must be of the form C\n+with a non-empty key. Function returns hash with all read values. Any white\n+space (other then new-line character) is preserved.\n+\n+=cut\n+\n+sub credential_read {\n+\tmy ($self, $reader) = _maybe_self(@_);\n+\tmy %credential;\n+\twhile (<$reader>) {\n+\t\tchomp;\n+\t\tif ($_ eq '') {\n+\t\t\tlast;\n+\t\t} elsif (!/^([^=]+)=(.*)$/) {\n+\t\t\tthrow Error::Simple(\"unable to parse git credential data:\\n$_\");\n+\t\t}\n+\t\t$credential{$1} = $2;\n+\t}\n+\treturn %credential;\n+}\n+\n+=item credential_write( FILEHANDLE, CREDENTIAL_HASHREF )\n+\n+Writes credential key-value pairs from hash referenced by\n+C to C. Keys and values cannot contain\n+new-lines or NUL bytes characters, and key cannot contain equal signs nor be\n+empty (if they do Error::Simple is thrown). Any white space is preserved. If\n+value for a key is C, it will be skipped.\n+\n+If C<'url'> key exists it will be written first. (All the other key-value\n+pairs are written in sorted order but you should not depend on that). Once\n+all lines are written, an empty line is printed.\n+\n+=cut\n+\n+sub credential_write {\n+\tmy ($self, $writer, $credential) = _maybe_self(@_);\n+\tmy ($key, $value);\n+\n+\t# Check if $credential is valid prior to writing anything\n+\twhile (($key, $value) = each %$credential) {\n+\t\tif (!defined $key || !length $key) {\n+\t\t\tthrow Error::Simple(\"credential key empty or undefined\");\n+\t\t} elsif ($key =~ /[=\\n\\0]/) {\n+\t\t\tthrow Error::Simple(\"credential key contains invalid characters: $key\");\n+\t\t} elsif (defined $value && $value =~ /[\\n\\0]/) {\n+\t\t\tthrow Error::Simple(\"credential value for key=$key contains invalid characters: $value\");\n+\t\t}\n+\t}\n+\n+\tfor $key (sort {\n+\t\t# url overwrites other fields, so it must come first\n+\t\treturn -1 if $a eq 'url';\n+\t\treturn 1 if $b eq 'url';\n+\t\treturn $a cmp $b;\n+\t} keys %$credential) {\n+\t\tif (defined $credential->{$key}) {\n+\t\t\tprint $writer $key, '=', $credential->{$key}, \"\\n\";\n+\t\t}\n+\t}\n+\tprint $writer \"\\n\";\n+}\n+\n+sub _credential_run {\n+\tmy ($self, $credential, $op) = _maybe_self(@_);\n+\tmy ($pid, $reader, $writer, $ctx) = command_bidi_pipe('credential', $op);\n+\n+\tcredential_write $writer, $credential;\n+\tclose $writer;\n+\n+\tif ($op eq \"fill\") {\n+\t\t%$credential = credential_read $reader;\n+\t}\n+\tif (<$reader>) {\n+\t\tthrow Error::Simple(\"unexpected output from git credential $op response:\\n$_\\n\");\n+\t}\n+\n+\tcommand_close_bidi_pipe($pid, $reader, undef, $ctx);\n+}\n+\n+=item credential( CREDENTIAL_HASHREF [, OPERATION ] )\n+\n+=item credential( CREDENTIAL_HASHREF, CODE )\n+\n+Executes C for a given set of credentials and specified\n+operation. In both forms C needs to be a reference to\n+a hash which stores credentials. Under certain conditions the hash can\n+change.\n+\n+In the first form, C can be C<'fill'>, C<'approve'> or C<'reject'>,\n+and function will execute corresponding C sub-command. If\n+it's omitted C<'fill'> is assumed. In case of C<'fill'> the values stored in\n+C will be changed to the ones returned by the C command. The usual usage would look something like:\n+\n+\tmy %cred = (\n+\t\t'protocol' => 'https',\n+\t\t'host' => 'example.com',\n+\t\t'username' => 'bob'\n+\t);\n+\tGit::credential \\%cred;\n+\tif (try_to_authenticate($cred{'username'}, $cred{'password'})) {\n+\t\tGit::credential \\%cred, 'approve';\n+\t\t... do more stuff ...\n+\t} else {\n+\t\tGit::credential \\%cred, 'reject';\n+\t}\n+\n+In the second form, C needs to be a reference to a subroutine. The\n+function will execute C to fill the provided credential\n+hash, then call C with C as the sole argument. If\n+C's return value is defined, the function will execute C (if return value yields true) or C (if return\n+value is false). If the return value is undef, nothing at all is executed;\n+this is useful, for example, if the credential could neither be verified nor\n+rejected due to an unrelated network error. The return value is the same as\n+what C returns. With this form, the usage might look as follows:\n+\n+\tif (Git::credential {\n+\t\t'protocol' => 'https',\n+\t\t'host' => 'example.com',\n+\t\t'username' => 'bob'\n+\t}, sub {\n+\t\tmy $cred = shift;\n+\t\treturn !!try_to_authenticate($cred->{'username'},\n+\t\t $cred->{'password'});\n+\t}) {\n+\t\t... do more stuff ...\n+\t}\n+\n+=cut\n+\n+sub credential {\n+\tmy ($self, $credential, $op_or_code) = (_maybe_self(@_), 'fill');\n+\n+\tif ('CODE' eq ref $op_or_code) {\n+\t\t_credential_run $credential, 'fill';\n+\t\tmy $ret = $op_or_code->($credential);\n+\t\tif (defined $ret) {\n+\t\t\t_credential_run $credential, $ret ? 'approve' : 'reject';\n+\t\t}\n+\t\treturn $ret;\n+\t} else {\n+\t\t_credential_run $credential, $op_or_code;\n+\t}\n+}\n+\n { # %TEMP_* Lexical Context\n \n my (%TEMP_FILEMAP, %TEMP_FILES);\n-- \n1.8.1.3.572.g32bae1f\n"},{"id":"209366","messageId":"32bae1f3c7159035ea3fb5f61ab622cbff30293a.1360677646.git.mina86@mina86.com","threadId":"32889","inReplyTo":"cover.1360677646.git.mina86@mina86.com","subject":"[PATCHv4 6/6] git-send-email: use git credential to obtain password","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-12T14:02:33Z","receivedAt":"2013-02-12T14:02:33Z","isPatch":false,"sender":{"key":"mpn@google.com","avatar":null},"body":"From: Michal Nazarewicz \n\nIf smtp_user is provided but smtp_pass is not, instead of\nprompting for password, make git-send-email use git\ncredential command instead.\n\nSigned-off-by: Michal Nazarewicz \n---\n Documentation/git-send-email.txt | 4 +--\n git-send-email.perl | 59 +++++++++++++++++++++++-----------------\n 2 files changed, 36 insertions(+), 27 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 44a1f7c..0cffef8 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -164,8 +164,8 @@ Sending\n Furthermore, passwords need not be specified in configuration files\n or on the command line. If a username has been specified (with\n '--smtp-user' or a 'sendemail.smtpuser'), but no password has been\n-specified (with '--smtp-pass' or 'sendemail.smtppass'), then the\n-user is prompted for a password while the input is masked for privacy.\n+specified (with '--smtp-pass' or 'sendemail.smtppass'), then\n+a password is obtained using 'git-credential'.\n \n --smtp-server=::\n \tIf set, specifies the outgoing SMTP server to use (e.g.\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex be809e5..76bbfc3 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1045,6 +1045,39 @@ sub maildomain {\n \treturn maildomain_net() || maildomain_mta() || 'localhost.localdomain';\n }\n \n+# Returns 1 if authentication succeeded or was not necessary\n+# (smtp_user was not specified), and 0 otherwise.\n+\n+sub smtp_auth_maybe {\n+\tif (!defined $smtp_authuser || $auth) {\n+\t\treturn 1;\n+\t}\n+\n+\t# Workaround AUTH PLAIN/LOGIN interaction defect\n+\t# with Authen::SASL::Cyrus\n+\teval {\n+\t\trequire Authen::SASL;\n+\t\tAuthen::SASL->import(qw(Perl));\n+\t};\n+\n+\t# TODO: Authentication may fail not because credentials were\n+\t# invalid but due to other reasons, in which we should not\n+\t# reject credentials.\n+\t$auth = Git::credential({\n+\t\t'protocol' => 'smtp',\n+\t\t'host' => join(':', $smtp_server, $smtp_server_port),\n+\t\t'username' => $smtp_authuser,\n+\t\t# if there's no password, \"git credential fill\" will\n+\t\t# give us one, otherwise it'll just pass this one.\n+\t\t'password' => $smtp_authpass\n+\t}, sub {\n+\t\tmy $cred = shift;\n+\t\treturn !!$smtp->auth($cred->{'username'}, $cred->{'password'});\n+\t});\n+\n+\treturn $auth;\n+}\n+\n # Returns 1 if the message was sent, and 0 otherwise.\n # In actuality, the whole program dies when there\n # is an error sending a message.\n@@ -1185,31 +1218,7 @@ X-Mailer: git-send-email $gitversion\n \t\t\t defined $smtp_server_port ? \" port=$smtp_server_port\" : \"\";\n \t\t}\n \n-\t\tif (defined $smtp_authuser) {\n-\t\t\t# Workaround AUTH PLAIN/LOGIN interaction defect\n-\t\t\t# with Authen::SASL::Cyrus\n-\t\t\teval {\n-\t\t\t\trequire Authen::SASL;\n-\t\t\t\tAuthen::SASL->import(qw(Perl));\n-\t\t\t};\n-\n-\t\t\tif (!defined $smtp_authpass) {\n-\n-\t\t\t\tsystem \"stty -echo\";\n-\n-\t\t\t\tdo {\n-\t\t\t\t\tprint \"Password: \";\n-\t\t\t\t\t$_ = ;\n-\t\t\t\t\tprint \"\\n\";\n-\t\t\t\t} while (!defined $_);\n-\n-\t\t\t\tchomp($smtp_authpass = $_);\n-\n-\t\t\t\tsystem \"stty echo\";\n-\t\t\t}\n-\n-\t\t\t$auth ||= $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message;\n-\t\t}\n+\t\tsmtp_auth_maybe or die $smtp->message;\n \n \t\t$smtp->mail( $raw_from ) or die $smtp->message;\n \t\t$smtp->to( @recipients ) or die $smtp->message;\n-- \n1.8.1.3.572.g32bae1f\n"},{"id":"209388","messageId":"7va9r9gy5y.fsf@alter.siamese.dyndns.org","threadId":"32889","inReplyTo":"fc760829f74f31d23f94b61a9e087eda2a66956e.1360677646.git.mina86@mina86.com","subject":"Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-12T18:55:05Z","receivedAt":"2013-02-12T18:55:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michal Nazarewicz writes:\n\n> From: Michal Nazarewicz \n>\n> The body of the loop in command_close_bidi_pipe function is identical to\n> what _cmd_close function does so instead of duplicating, refactor change\n> _cmd_close so that it accepts list of file handlers to be closed, which\n\ns/file handlers/file handles/, I think.\n"},{"id":"209408","messageId":"20130212204807.GB25330@sigill.intra.peff.net","threadId":"32889","inReplyTo":"7va9r9gy5y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-12T20:48:07Z","receivedAt":"2013-02-12T20:48:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 12, 2013 at 10:55:05AM -0800, Junio C Hamano wrote:\n\n> Michal Nazarewicz writes:\n> \n> > From: Michal Nazarewicz \n> >\n> > The body of the loop in command_close_bidi_pipe function is identical to\n> > what _cmd_close function does so instead of duplicating, refactor change\n> > _cmd_close so that it accepts list of file handlers to be closed, which\n> \n> s/file handlers/file handles/, I think.\n\nAnd s/refactor change/refactor/.\n\nOther than that, I think the series looks OK. I have one style micro-nit\non patch 4 which I'll reply in-line. But it is either \"fix while\napplying\" or \"ignore\", I don't think it will be worth a re-roll.\n\n-Peff\n"},{"id":"209410","messageId":"20130212205141.GC25330@sigill.intra.peff.net","threadId":"32889","inReplyTo":"3bb6b7736eb4b0a958469be13d8c646faec1208a.1360677646.git.mina86@mina86.com","subject":"Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-12T20:51:41Z","receivedAt":"2013-02-12T20:51:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:\n\n> sub command_close_bidi_pipe {\n> \tlocal $?;\n> \tmy ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);\n> -\t_cmd_close($ctx, $in, $out);\n> +\t_cmd_close($ctx, grep defined, $in, $out);\n\nMaybe it is just me, but I find the \"grep EXPR\" form a little subtle\ninside an argument list. Either:\n\n _cmd_close($ctx, grep { defined } $in, $out);\n\nor\n\n _cmd_close($ctx, grep(defined, $in, $out));\n\nis a little more obvious to me.\n\n-Peff\n"},{"id":"209414","messageId":"xa1tk3qd9qza.fsf@mina86.com","threadId":"32889","inReplyTo":"20130212204807.GB25330@sigill.intra.peff.net","subject":"Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2013-02-12T21:12:09Z","receivedAt":"2013-02-12T21:12:09Z","isPatch":false,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":">> Michal Nazarewicz writes:\n>> > The body of the loop in command_close_bidi_pipe function is identical to\n>> > what _cmd_close function does so instead of duplicating, refactor change\n>> > _cmd_close so that it accepts list of file handlers to be closed, which\n\n> On Tue, Feb 12, 2013 at 10:55:05AM -0800, Junio C Hamano wrote:\n>> s/file handlers/file handles/, I think.\n\nOn Tue, Feb 12 2013, Jeff King wrote:\n> And s/refactor change/refactor/.\n>\n> Other than that, I think the series looks OK. I have one style micro-nit\n> on patch 4 which I'll reply in-line. But it is either \"fix while\n> applying\" or \"ignore\", I don't think it will be worth a re-roll.\n\nAll fixed.\n\nJunio, do you want me to resend or would you be fine with just pulling:\n\n\tgit://github.com/mina86/git.git master\n\n-- \nBest regards, _ _\n.o. | Liege of Serenely Enlightened Majesty of o' \\,=./ `o\n..o | Computer Science, Michał “mina86” Nazarewicz (o o)\nooo +------------------ooO--(_)--Ooo--\n\n"},{"id":"209415","messageId":"xa1thalh9qxl.fsf@mina86.com","threadId":"32889","inReplyTo":"20130212205141.GC25330@sigill.intra.peff.net","subject":"Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2013-02-12T21:13:10Z","receivedAt":"2013-02-12T21:13:10Z","isPatch":false,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"On Tue, Feb 12 2013, Jeff King wrote:\n> On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:\n>\n>> sub command_close_bidi_pipe {\n>> \tlocal $?;\n>> \tmy ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);\n>> -\t_cmd_close($ctx, $in, $out);\n>> +\t_cmd_close($ctx, grep defined, $in, $out);\n>\n> Maybe it is just me, but I find the \"grep EXPR\" form a little subtle\n> inside an argument list. Either:\n>\n> _cmd_close($ctx, grep { defined } $in, $out);\n>\n> or\n>\n> _cmd_close($ctx, grep(defined, $in, $out));\n>\n> is a little more obvious to me.\n\nI personally avoid parens whenever possible in Perl, but Git.pm seem to\nfavour them so I went with the second option.\n\n-- \nBest regards, _ _\n.o. | Liege of Serenely Enlightened Majesty of o' \\,=./ `o\n..o | Computer Science, Michał “mina86” Nazarewicz (o o)\nooo +------------------ooO--(_)--Ooo--\n\n"},{"id":"209416","messageId":"7va9r9fd4e.fsf@alter.siamese.dyndns.org","threadId":"32889","inReplyTo":"20130212205141.GC25330@sigill.intra.peff.net","subject":"Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-12T21:14:57Z","receivedAt":"2013-02-12T21:14:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:\n>\n>> sub command_close_bidi_pipe {\n>> \tlocal $?;\n>> \tmy ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);\n>> -\t_cmd_close($ctx, $in, $out);\n>> +\t_cmd_close($ctx, grep defined, $in, $out);\n>\n> Maybe it is just me, but I find the \"grep EXPR\" form a little subtle\n> inside an argument list. Either:\n>\n> _cmd_close($ctx, grep { defined } $in, $out);\n>\n> or\n>\n> _cmd_close($ctx, grep(defined, $in, $out));\n>\n> is a little more obvious to me.\n\nI would actually vote for the most explicit:\n\n\t_cmd_close($ctx, (grep { defined } ($in, $out)));\n"},{"id":"209417","messageId":"7v621xfd0f.fsf@alter.siamese.dyndns.org","threadId":"32889","inReplyTo":"xa1tk3qd9qza.fsf@mina86.com","subject":"Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-12T21:17:20Z","receivedAt":"2013-02-12T21:17:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michal Nazarewicz writes:\n\n>>> Michal Nazarewicz writes:\n>>> > The body of the loop in command_close_bidi_pipe function is identical to\n>>> > what _cmd_close function does so instead of duplicating, refactor change\n>>> > _cmd_close so that it accepts list of file handlers to be closed, which\n>\n>> On Tue, Feb 12, 2013 at 10:55:05AM -0800, Junio C Hamano wrote:\n>>> s/file handlers/file handles/, I think.\n>\n> On Tue, Feb 12 2013, Jeff King wrote:\n>> And s/refactor change/refactor/.\n>>\n>> Other than that, I think the series looks OK. I have one style micro-nit\n>> on patch 4 which I'll reply in-line. But it is either \"fix while\n>> applying\" or \"ignore\", I don't think it will be worth a re-roll.\n>\n> All fixed.\n>\n> Junio, do you want me to resend or would you be fine with just pulling:\n>\n> \tgit://github.com/mina86/git.git master\n\nNeither. I agree with Peff that these micronits are not enough\nreason for the trouble of rerolling the series, so I'll just amend\nthem at my end. Please double-check what you see on the 'pu' branch\nwhen I push today's integration result out later.\n\nThanks.\n"},{"id":"209419","messageId":"20130212211759.GA30329@sigill.intra.peff.net","threadId":"32889","inReplyTo":"7va9r9fd4e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-12T21:17:59Z","receivedAt":"2013-02-12T21:17:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 12, 2013 at 01:14:57PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:\n> >\n> >> sub command_close_bidi_pipe {\n> >> \tlocal $?;\n> >> \tmy ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);\n> >> -\t_cmd_close($ctx, $in, $out);\n> >> +\t_cmd_close($ctx, grep defined, $in, $out);\n> >\n> > Maybe it is just me, but I find the \"grep EXPR\" form a little subtle\n> > inside an argument list. Either:\n> >\n> > _cmd_close($ctx, grep { defined } $in, $out);\n> >\n> > or\n> >\n> > _cmd_close($ctx, grep(defined, $in, $out));\n> >\n> > is a little more obvious to me.\n> \n> I would actually vote for the most explicit:\n> \n> \t_cmd_close($ctx, (grep { defined } ($in, $out)));\n\nGross. My perl spider-sense tingles at seeing that many optional\npunctuation characters, but it should at least be obvious to a casual or\nnew perl programmer what is going on. I'm fine with it.\n\n-Peff\n"},{"id":"209427","messageId":"xa1tehgl9mfg.fsf@mina86.com","threadId":"32889","inReplyTo":"7va9r9fd4e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2013-02-12T22:50:27Z","receivedAt":"2013-02-12T22:50:27Z","isPatch":false,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"On Tue, Feb 12 2013, Junio C Hamano wrote:\n> I would actually vote for the most explicit:\n>\n> \t_cmd_close($ctx, (grep { defined } ($in, $out)));\n\nTo me that looks weird at best, but I don't have strong opinions on that\nmatter.\n\n-- \nBest regards, _ _\n.o. | Liege of Serenely Enlightened Majesty of o' \\,=./ `o\n..o | Computer Science, Michał “mina86” Nazarewicz (o o)\nooo +------------------ooO--(_)--Ooo--\n\n"},{"id":"210392","messageId":"vpqppzl4z7d.fsf@grenoble-inp.fr","threadId":"32889","inReplyTo":"e5834c9b5ccb66a88b64b3d07982ad41205fb97e.1360677646.git.mina86@mina86.com","subject":"Re: [PATCHv4 5/6] Git.pm: add interface for git credential command","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-02-27T14:18:46Z","receivedAt":"2013-02-27T14:18:46Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Michal Nazarewicz writes:\n\n> +=item credential_read( FILEHANDLE )\n> +\n> +Reads credential key-value pairs from C. Reading stops at EOF or\n> +when an empty line is encountered. Each line must be of the form C\n> +with a non-empty key. Function returns hash with all read values. Any white\n> +space (other then new-line character) is preserved.\n\nTypo: other then -> than.\n\n> +sub credential_read {\n> +\tmy ($self, $reader) = _maybe_self(@_);\n> +\tmy %credential;\n> +\twhile (<$reader>) {\n> +\t\tchomp;\n> +\t\tif ($_ eq '') {\n> +\t\t\tlast;\n> +\t\t} elsif (!/^([^=]+)=(.*)$/) {\n\nGood.\n\n> +\t# Check if $credential is valid prior to writing anything\n> +\twhile (($key, $value) = each %$credential) {\n> +\t\tif (!defined $key || !length $key) {\n> +\t\t\tthrow Error::Simple(\"credential key empty or undefined\");\n> +\t\t} elsif ($key =~ /[=\\n\\0]/) {\n> +\t\t\tthrow Error::Simple(\"credential key contains invalid characters: $key\");\n> +\t\t} elsif (defined $value && $value =~ /[\\n\\0]/) {\n> +\t\t\tthrow Error::Simple(\"credential value for key=$key contains invalid characters: $value\");\n> +\t\t}\n> +\t}\n\nGood.\n\nThese checks seem to address all the points raised during discussion\nabout when the API should reject values.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"210393","messageId":"vpqhakx4z4c.fsf@grenoble-inp.fr","threadId":"32889","inReplyTo":"32bae1f3c7159035ea3fb5f61ab622cbff30293a.1360677646.git.mina86@mina86.com","subject":"Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-02-27T14:20:35Z","receivedAt":"2013-02-27T14:20:35Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Michal Nazarewicz writes:\n\n> +\t$auth = Git::credential({\n> +\t\t'protocol' => 'smtp',\n> +\t\t'host' => join(':', $smtp_server, $smtp_server_port),\n\nAt this point, $smtp_server_port is not always defined. I just tested\nand got\n\nUse of uninitialized value $smtp_server_port in join or string at\ngit-send-email line 1077.\n\nOther than that, the whole series looks good.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"210401","messageId":"7vehg1kb09.fsf@alter.siamese.dyndns.org","threadId":"32889","inReplyTo":"vpqhakx4z4c.fsf@grenoble-inp.fr","subject":"Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-27T15:54:46Z","receivedAt":"2013-02-27T15:54:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Michal Nazarewicz writes:\n>\n>> +\t$auth = Git::credential({\n>> +\t\t'protocol' => 'smtp',\n>> +\t\t'host' => join(':', $smtp_server, $smtp_server_port),\n>\n> At this point, $smtp_server_port is not always defined. I just tested\n> and got\n>\n> Use of uninitialized value $smtp_server_port in join or string at\n> git-send-email line 1077.\n>\n> Other than that, the whole series looks good.\n\nGiven that there is another place that conditionally append \":$port\"\nto the host string, I think we should follow suit here. Perhaps\nlike the attached diff?\n\nThanks for a review.\n\n\n git-send-email.perl | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 76bbfc3..c3501d9 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1045,6 +1045,14 @@ sub maildomain {\n \treturn maildomain_net() || maildomain_mta() || 'localhost.localdomain';\n }\n \n+sub smtp_host_string {\n+\tif (defined $smtp_server_port) {\n+\t\treturn \"$smtp_server:$smtp_server_port\";\n+\t} else {\n+\t\treturn $smtp_server;\n+\t}\n+}\n+\n # Returns 1 if authentication succeeded or was not necessary\n # (smtp_user was not specified), and 0 otherwise.\n \n@@ -1065,7 +1073,7 @@ sub smtp_auth_maybe {\n \t# reject credentials.\n \t$auth = Git::credential({\n \t\t'protocol' => 'smtp',\n-\t\t'host' => join(':', $smtp_server, $smtp_server_port),\n+\t\t'host' => smtp_host_string(),\n \t\t'username' => $smtp_authuser,\n \t\t# if there's no password, \"git credential fill\" will\n \t\t# give us one, otherwise it'll just pass this one.\n@@ -1188,9 +1196,7 @@ sub send_message {\n \t\telse {\n \t\t\trequire Net::SMTP;\n \t\t\t$smtp_domain ||= maildomain();\n-\t\t\t$smtp ||= Net::SMTP->new((defined $smtp_server_port)\n-\t\t\t\t\t\t ? \"$smtp_server:$smtp_server_port\"\n-\t\t\t\t\t\t : $smtp_server,\n+\t\t\t$smtp ||= Net::SMTP->new(smtp_host_string(),\n \t\t\t\t\t\t Hello => $smtp_domain,\n \t\t\t\t\t\t Debug => $debug_net_smtp);\n \t\t\tif ($smtp_encryption eq 'tls' && $smtp) {\n"},{"id":"210404","messageId":"xa1tobf5viuk.fsf@mina86.com","threadId":"32889","inReplyTo":"7vehg1kb09.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2013-02-27T16:09:55Z","receivedAt":"2013-02-27T16:09:55Z","isPatch":false,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"On Wed, Feb 27 2013, Junio C Hamano wrote:\n> Matthieu Moy writes:\n>\n>> Michal Nazarewicz writes:\n>>\n>>> +\t$auth = Git::credential({\n>>> +\t\t'protocol' => 'smtp',\n>>> +\t\t'host' => join(':', $smtp_server, $smtp_server_port),\n>>\n>> At this point, $smtp_server_port is not always defined. I just tested\n>> and got\n>>\n>> Use of uninitialized value $smtp_server_port in join or string at\n>> git-send-email line 1077.\n>>\n>> Other than that, the whole series looks good.\n>\n> Given that there is another place that conditionally append \":$port\"\n> to the host string, I think we should follow suit here. Perhaps\n> like the attached diff?\n\nDamn meetings, you beat me to it… I was just about to send a patch. ;)\n\n> Thanks for a review.\n>\n>\n> git-send-email.perl | 14 ++++++++++----\n> 1 file changed, 10 insertions(+), 4 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 76bbfc3..c3501d9 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1045,6 +1045,14 @@ sub maildomain {\n> \treturn maildomain_net() || maildomain_mta() || 'localhost.localdomain';\n> }\n> \n> +sub smtp_host_string {\n> +\tif (defined $smtp_server_port) {\n> +\t\treturn \"$smtp_server:$smtp_server_port\";\n> +\t} else {\n> +\t\treturn $smtp_server;\n> +\t}\n> +}\n> +\n> # Returns 1 if authentication succeeded or was not necessary\n> # (smtp_user was not specified), and 0 otherwise.\n> \n> @@ -1065,7 +1073,7 @@ sub smtp_auth_maybe {\n> \t# reject credentials.\n> \t$auth = Git::credential({\n> \t\t'protocol' => 'smtp',\n> -\t\t'host' => join(':', $smtp_server, $smtp_server_port),\n> +\t\t'host' => smtp_host_string(),\n> \t\t'username' => $smtp_authuser,\n> \t\t# if there's no password, \"git credential fill\" will\n> \t\t# give us one, otherwise it'll just pass this one.\n> @@ -1188,9 +1196,7 @@ sub send_message {\n> \t\telse {\n> \t\t\trequire Net::SMTP;\n> \t\t\t$smtp_domain ||= maildomain();\n> -\t\t\t$smtp ||= Net::SMTP->new((defined $smtp_server_port)\n> -\t\t\t\t\t\t ? \"$smtp_server:$smtp_server_port\"\n> -\t\t\t\t\t\t : $smtp_server,\n> +\t\t\t$smtp ||= Net::SMTP->new(smtp_host_string(),\n> \t\t\t\t\t\t Hello => $smtp_domain,\n> \t\t\t\t\t\t Debug => $debug_net_smtp);\n> \t\t\tif ($smtp_encryption eq 'tls' && $smtp) {\n\n>From reading of SMTP.pm, it seems that this could be changed to:\n\n-\t\t\t$smtp ||= Net::SMTP->new((defined $smtp_server_port)\n-\t\t\t\t\t\t ? \"$smtp_server:$smtp_server_port\"\n-\t\t\t\t\t\t : $smtp_server,\n+\t\t\t$smtp ||= Net::SMTP->new($smtp_server,\n+\t\t\t\t\t\t Port => $smtp_server_port,\n\nand than the other part would become:\n\n@@ -1060,12 +1060,17 @@ sub smtp_auth_maybe {\n Authen::SASL->import(qw(Perl));\n };\n \n+ my $host = $smtp_server;\n+ if (defined $smtp_server_port) {\n+ $host .= ':' . $smtp_server_port;\n+ }\n+\n # TODO: Authentication may fail not because credentials were\n # invalid but due to other reasons, in which we should not\n # reject credentials.\n $auth = Git::credential({\n 'protocol' => 'smtp',\n- 'host' => join(':', $smtp_server, $smtp_server_port),\n+ 'host' => $host,\n 'username' => $smtp_authuser,\n # if there's no password, \"git credential fill\" will\n # give us one, otherwise it'll just pass this one.\n\nEither way, looks good to me.\n\n-- \nBest regards, _ _\n.o. | Liege of Serenely Enlightened Majesty of o' \\,=./ `o\n..o | Computer Science, Michał “mina86” Nazarewicz (o o)\nooo +------------------ooO--(_)--Ooo--\n\n"},{"id":"210405","messageId":"vpqy5e9zqe9.fsf@grenoble-inp.fr","threadId":"32889","inReplyTo":"7vehg1kb09.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-02-27T16:13:18Z","receivedAt":"2013-02-27T16:13:18Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 76bbfc3..c3501d9 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1045,6 +1045,14 @@ sub maildomain {\n> \treturn maildomain_net() || maildomain_mta() || 'localhost.localdomain';\n> }\n> \n> +sub smtp_host_string {\n> +\tif (defined $smtp_server_port) {\n> +\t\treturn \"$smtp_server:$smtp_server_port\";\n> +\t} else {\n> +\t\treturn $smtp_server;\n> +\t}\n> +}\n> +\n> # Returns 1 if authentication succeeded or was not necessary\n> # (smtp_user was not specified), and 0 otherwise.\n> \n> @@ -1065,7 +1073,7 @@ sub smtp_auth_maybe {\n> \t# reject credentials.\n> \t$auth = Git::credential({\n> \t\t'protocol' => 'smtp',\n> -\t\t'host' => join(':', $smtp_server, $smtp_server_port),\n> +\t\t'host' => smtp_host_string(),\n> \t\t'username' => $smtp_authuser,\n> \t\t# if there's no password, \"git credential fill\" will\n> \t\t# give us one, otherwise it'll just pass this one.\n> @@ -1188,9 +1196,7 @@ sub send_message {\n> \t\telse {\n> \t\t\trequire Net::SMTP;\n> \t\t\t$smtp_domain ||= maildomain();\n> -\t\t\t$smtp ||= Net::SMTP->new((defined $smtp_server_port)\n> -\t\t\t\t\t\t ? \"$smtp_server:$smtp_server_port\"\n> -\t\t\t\t\t\t : $smtp_server,\n> +\t\t\t$smtp ||= Net::SMTP->new(smtp_host_string(),\n> \t\t\t\t\t\t Hello => $smtp_domain,\n> \t\t\t\t\t\t Debug => $debug_net_smtp);\n> \t\t\tif ($smtp_encryption eq 'tls' && $smtp) {\n\nSeems obviously correct. I also did a basic test and it worked smoothly.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"210407","messageId":"7va9qpk9eh.fsf@alter.siamese.dyndns.org","threadId":"32889","inReplyTo":"vpqy5e9zqe9.fsf@grenoble-inp.fr","subject":"Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-27T16:29:26Z","receivedAt":"2013-02-27T16:29:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Junio C Hamano writes:\n>\n>> diff --git a/git-send-email.perl b/git-send-email.perl\n>> index 76bbfc3..c3501d9 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -1045,6 +1045,14 @@ sub maildomain {\n>> \treturn maildomain_net() || maildomain_mta() || 'localhost.localdomain';\n>> }\n>> \n>> +sub smtp_host_string {\n>> +\tif (defined $smtp_server_port) {\n>> +\t\treturn \"$smtp_server:$smtp_server_port\";\n>> +\t} else {\n>> +\t\treturn $smtp_server;\n>> +\t}\n>> +}\n>> +\n>> # Returns 1 if authentication succeeded or was not necessary\n>> # (smtp_user was not specified), and 0 otherwise.\n>> \n>> @@ -1065,7 +1073,7 @@ sub smtp_auth_maybe {\n>> \t# reject credentials.\n>> \t$auth = Git::credential({\n>> \t\t'protocol' => 'smtp',\n>> -\t\t'host' => join(':', $smtp_server, $smtp_server_port),\n>> +\t\t'host' => smtp_host_string(),\n>> \t\t'username' => $smtp_authuser,\n>> \t\t# if there's no password, \"git credential fill\" will\n>> \t\t# give us one, otherwise it'll just pass this one.\n>> @@ -1188,9 +1196,7 @@ sub send_message {\n>> \t\telse {\n>> \t\t\trequire Net::SMTP;\n>> \t\t\t$smtp_domain ||= maildomain();\n>> -\t\t\t$smtp ||= Net::SMTP->new((defined $smtp_server_port)\n>> -\t\t\t\t\t\t ? \"$smtp_server:$smtp_server_port\"\n>> -\t\t\t\t\t\t : $smtp_server,\n>> +\t\t\t$smtp ||= Net::SMTP->new(smtp_host_string(),\n>> \t\t\t\t\t\t Hello => $smtp_domain,\n>> \t\t\t\t\t\t Debug => $debug_net_smtp);\n>> \t\t\tif ($smtp_encryption eq 'tls' && $smtp) {\n>\n> Seems obviously correct. I also did a basic test and it worked smoothly.\n\nOK, I'll squash it in.\nThanks.\n"}]}