{"thread":{"id":"32848","subject":"[PATCHv2 0/5] Make git-send-email use git-credential","startedAt":"2013-02-07T14:01:16Z","lastAt":"2013-02-08T05:11:20Z","messageCount":11,"participants":["Michal Nazarewicz","Junio C Hamano","Matthieu Moy","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"208913","messageId":"cover.1360242782.git.mina86@mina86.com","threadId":"32848","inReplyTo":null,"subject":"[PATCHv2 0/5] Make git-send-email use git-credential","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-07T14:01:16Z","receivedAt":"2013-02-07T14:01:16Z","isPatch":false,"sender":{"key":"mpn@google.com","avatar":null},"body":"From: Michal Nazarewicz \n\nMinor fixes as suggested in emails.\n\nMichal Nazarewicz (5):\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: 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 | 129 +++++++++++++++++++++++++++++++++++++--\n 3 files changed, 161 insertions(+), 31 deletions(-)\n\n-- \n1.8.1.2.549.g1d13f9f\n"},{"id":"208916","messageId":"80ccd09ea28fe5282ec97f4d20896a9c55720913.1360242782.git.mina86@mina86.com","threadId":"32848","inReplyTo":"cover.1360242782.git.mina86@mina86.com","subject":"[PATCHv2 1/5] Git.pm: allow command_close_bidi_pipe to be called as method","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-07T14:01:17Z","receivedAt":"2013-02-07T14:01:17Z","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.2.549.g1d13f9f\n"},{"id":"208915","messageId":"21a7bae678adc80768193b62ec87742feaf97c44.1360242782.git.mina86@mina86.com","threadId":"32848","inReplyTo":"cover.1360242782.git.mina86@mina86.com","subject":"[PATCHv2 2/5] Git.pm: fix example in command_close_bidi_pipe documentation","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-07T14:01:18Z","receivedAt":"2013-02-07T14:01:18Z","isPatch":false,"sender":{"key":"mpn@google.com","avatar":null},"body":"From: Michal Nazarewicz \n\nFile handle goes as the first argument when calling print on it.\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.2.549.g1d13f9f\n"},{"id":"208917","messageId":"afa54fb5dd2d08759317099d10090b81adfb593f.1360242782.git.mina86@mina86.com","threadId":"32848","inReplyTo":"cover.1360242782.git.mina86@mina86.com","subject":"[PATCHv2 3/5] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-07T14:01:19Z","receivedAt":"2013-02-07T14:01:19Z","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\n > On Wed, Feb 06, 2013 at 09:47:04PM +0100, Michal Nazarewicz wrote:\n >> This allows for something like:\n >> \n >> my ($pid, $in, $out, $ctx) = command_bidi_pipe(...);\n >> print $out \"write data\";\n >> close $out;\n >> # ... do stuff with $in\n >> command_close_bidi_pipe($pid, $in, undef, $ctx);\n\n On Thu, Feb 07 2013, Jeff King wrote:\n > Should this part go into the documentation for command_close_bidi_pipe\n > in Git.pm?\n\n Done.\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 11f310a..9dded54 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -426,13 +426,26 @@ 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 \tforeach my $fh ($in, $out) {\n-\t\tunless (close $fh) {\n+\t\tif (defined $fh && !close $fh) {\n \t\t\tif ($!) {\n \t\t\t\tcarp \"error closing pipe: $!\";\n \t\t\t} elsif ($? >> 8) {\n-- \n1.8.1.2.549.g1d13f9f\n"},{"id":"208914","messageId":"78516627e893e54d5aafe0694d1face9a37893de.1360242782.git.mina86@mina86.com","threadId":"32848","inReplyTo":"cover.1360242782.git.mina86@mina86.com","subject":"[PATCHv2 4/5] Git.pm: add interface for git credential command","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-07T14:01:20Z","receivedAt":"2013-02-07T14:01:20Z","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 | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 109 insertions(+), 1 deletion(-)\n\n On Thu, Feb 07 2013, Jeff King wrote:\n > On Wed, Feb 06, 2013 at 09:47:05PM +0100, Michal Nazarewicz wrote:\n >\n >> +sub _credential_read {\n >> +\tmy %credential;\n >> +\tmy ($reader, $op) = (@_);\n >> +\twhile (<$reader>) {\n >> +\t\tchomp;\n >> +\t\tmy ($key, $value) = /([^=]*)=(.*)/;\n >\n > Empty keys are not valid. Can we make this:\n >\n > /^([^=]+)=(.*)/\n >\n > to fail the regex? Otherwise, I think this check:\n >\n >> +\t\tif (not defined $key) {\n >> +\t\t\tthrow Error::Simple(\"unable to parse git credential $op response:\\n$_\\n\");\n >> +\t\t}\n >\n > would not pass because $key would be the empty string.\n\n Right, fixed. \n\n >> +sub _credential_write {\n >> +\tmy ($credential, $writer) = @_;\n >> +\n >> +\tfor my $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} && length $credential->{$key}) {\n >> +\t\t\tprint $writer $key, '=', $credential->{$key}, \"\\n\";\n >> +\t\t}\n >> +\t}\n >\n > There are a few disallowed characters, like \"\\n\" in key or value, and\n > \"=\" in a key. They should never happen unless the caller is buggy, but\n > should we check and catch them here?\n\n I left it as is for now since it's not entairly clear to me what to\n do in all cases. In particular:\n \n - when reading, what to do if the line is \" foo = bar \",\n - when reading, what to do if the line is \"foo=\" (ie. empty value),\n - when writing, what to do if value is a single space,\n - when writing, what to do if value ends with a new line,\n - when writing, what to do if value is empty (currently not printed at all),\n\n On Thu, Feb 07 2013, Matthieu Moy wrote:\n > I think you should credit git-remote-mediawiki for the code in the\n > commit message. Perhaps have a first \"copy/paste\" commit, and then an\n > \"adaptation\" commit to add sort, ^ anchor in regexp, doc and your\n > callback mechanism, but I won't insist on that.\n\n Good point. Creating additional commit is a bit too much for my\n licking, but added note in commit message.\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 9dded54..b4adead 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -59,7 +59,8 @@ 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- temp_acquire temp_release temp_reset temp_path);\n+ temp_acquire temp_release temp_reset temp_path\n+ credential);\n \n \n =head1 DESCRIPTION\n@@ -1013,6 +1014,113 @@ sub _close_cat_blob {\n }\n \n \n+sub _credential_read {\n+\tmy %credential;\n+\tmy ($reader, $op) = (@_);\n+\twhile (<$reader>) {\n+\t\tif (!/^([^=\\s]+)=(.*?)\\s*$/) {\n+\t\t\tthrow Error::Simple(\"unable to parse git credential $op response:\\n$_\");\n+\t\t}\n+\t\t$credential{$1} = $2;\n+\t}\n+\treturn %credential;\n+}\n+\n+sub _credential_write {\n+\tmy ($credential, $writer) = @_;\n+\n+\tfor my $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} && length $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+\n+\tmy ($pid, $reader, $writer, $ctx) = command_bidi_pipe('credential', $op);\n+\n+\t_credential_write $credential, $writer;\n+\tclose $writer;\n+\n+\tif ($op eq \"fill\") {\n+\t\t%$credential = _credential_read $reader, $op;\n+\t} elsif (<$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_HASH [, OPERATION ] )\n+\n+=item credential( CREDENTIAL_HASH, CODE )\n+\n+Executes C for a given set of credentials and\n+specified operation. In both form C needs to be\n+a reference to a hash which stores credentials. Under certain\n+conditions the hash can change.\n+\n+In the first form, C can be C<'fill'> (or omitted),\n+C<'approve'> or C<'reject'>, and function will execute corresponding\n+C sub-command. In case of C<'fill'> the values stored\n+in C will be changed to the ones returned by the\n+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.\n+The function will execute C to fill provided\n+credential hash, than call C with C as the sole\n+argument, and finally depending on C's return value execute\n+C (if return value yields true) or C (otherwise). The return value is the same as what\n+C returned. 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'}, $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\t_credential_run $credential, $ret ? 'approve' : 'reject';\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.2.549.g1d13f9f\n"},{"id":"208918","messageId":"0b3c9b66ccb6c8343dafd210b82c7765891d3785.1360242782.git.mina86@mina86.com","threadId":"32848","inReplyTo":"cover.1360242782.git.mina86@mina86.com","subject":"[PATCHv2 5/5] git-send-email: use git credential to obtain password","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-07T14:01:21Z","receivedAt":"2013-02-07T14:01:21Z","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.2.549.g1d13f9f\n"},{"id":"208931","messageId":"7vd2wc7ypb.fsf@alter.siamese.dyndns.org","threadId":"32848","inReplyTo":"0b3c9b66ccb6c8343dafd210b82c7765891d3785.1360242782.git.mina86@mina86.com","subject":"Re: [PATCHv2 5/5] git-send-email: use git credential to obtain password","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-07T18:42:24Z","receivedAt":"2013-02-07T18:42:24Z","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> If smtp_user is provided but smtp_pass is not, instead of\n> prompting for password, make git-send-email use git\n> credential command instead.\n>\n> Signed-off-by: Michal Nazarewicz \n> ---\n\nNice ;-)\n\nI'd expect reviews on 4/5 from Peff and Matthiew which may result in\neither Reviewed-by:'s or another round, but everything else looks in\ngood order.\n\nThanks to all three of you for working on this.\n\n> Documentation/git-send-email.txt | 4 +--\n> git-send-email.perl | 59 +++++++++++++++++++++++-----------------\n> 2 files changed, 36 insertions(+), 27 deletions(-)\n>\n> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n> index 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.\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 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"},{"id":"208932","messageId":"vpq38x8m06f.fsf@grenoble-inp.fr","threadId":"32848","inReplyTo":"78516627e893e54d5aafe0694d1face9a37893de.1360242782.git.mina86@mina86.com","subject":"Re: [PATCHv2 4/5] Git.pm: add interface for git credential command","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-02-07T18:46:48Z","receivedAt":"2013-02-07T18:46:48Z","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> From: Michal Nazarewicz \n>\n> Add a credential() function which is an interface to the git\n> credential command. The code is heavily based on credential_*\n> functions in .\n\nI'm no perl expert, so I cannot comment much on style (there are many\nsmall changes compared to the mediawiki code that look like improvement\nthough), but:\n\nReviewed-by: Matthieu Moy \n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"208956","messageId":"7v7gmj66fq.fsf@alter.siamese.dyndns.org","threadId":"32848","inReplyTo":"vpq38x8m06f.fsf@grenoble-inp.fr","subject":"Re: [PATCHv2 4/5] Git.pm: add interface for git credential command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-07T23:38:17Z","receivedAt":"2013-02-07T23:38:17Z","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>> From: Michal Nazarewicz \n>>\n>> Add a credential() function which is an interface to the git\n>> credential command. The code is heavily based on credential_*\n>> functions in .\n>\n> I'm no perl expert, so I cannot comment much on style (there are many\n> small changes compared to the mediawiki code that look like improvement\n> though), but:\n>\n> Reviewed-by: Matthieu Moy \n\nThanks. I'd actually be more worried about the error checking issue\nPeff raised during his review. I have a feeling that \"when in doubt,\ndo not cause harm\" is a more prudent way to go than \"I do not know,\nso I'll let anything pass\".\n"},{"id":"208962","messageId":"xa1t6223poum.fsf@mina86.com","threadId":"32848","inReplyTo":"7v7gmj66fq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 4/5] Git.pm: add interface for git credential command","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2013-02-08T01:37:53Z","receivedAt":"2013-02-08T01:37:53Z","isPatch":false,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"On Fri, Feb 08 2013, Junio C Hamano wrote:\n> I'd actually be more worried about the error checking issue\n> Peff raised during his review. I have a feeling that \"when in doubt,\n> do not cause harm\" is a more prudent way to go than \"I do not know,\n> so I'll let anything pass\".\n\nI can implement whatever checking you wish, just tell me what to do in\ncorner cases I've listed. ;)\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":"208973","messageId":"20130208051120.GD4157@sigill.intra.peff.net","threadId":"32848","inReplyTo":"78516627e893e54d5aafe0694d1face9a37893de.1360242782.git.mina86@mina86.com","subject":"Re: [PATCHv2 4/5] Git.pm: add interface for git credential command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-08T05:11:20Z","receivedAt":"2013-02-08T05:11:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 07, 2013 at 03:01:20PM +0100, Michal Nazarewicz wrote:\n\n> > There are a few disallowed characters, like \"\\n\" in key or value, and\n> > \"=\" in a key. They should never happen unless the caller is buggy, but\n> > should we check and catch them here?\n> \n> I left it as is for now since it's not entairly clear to me what to\n> do in all cases. In particular:\n> \n> - when reading, what to do if the line is \" foo = bar \",\n\nAccording to the spec, whitespace (except for the final newline) is not\nsignificant, and that parses key=\" foo \", value=\" bar \". The spec could\nignore whitespace on the key side, but I intentionally did not in an\nattempt to keep the protocol simple. Your original implementation did\nthe right thing already.\n\n> - when reading, what to do if the line is \"foo=\" (ie. empty value),\n\nThe empty string is a valid value.\n\n> - when writing, what to do if value is a single space,\n\nThen it's a single space. It's the caller's problem whether that is an\nissue or not.\n\n> - when writing, what to do if value ends with a new line,\n\nThat's bogus. We cannot represent that value. I'd suggest to simply die,\nas it is a bug in the caller (we _could_ try to be nice and assume the\ncaller accidentally forgot to chomp, but I'd rather be careful than\nnice).\n\n> - when writing, what to do if value is empty (currently not printed at all),\n\nI think you should still print it. It's unlikely to matter, but\ntechnically a helper response may override keys (or set them to blank),\nand the intermediate state gets sent on to the next helper, if there are\nmultiple.\n\n> On Thu, Feb 07 2013, Matthieu Moy wrote:\n> > I think you should credit git-remote-mediawiki for the code in the\n> > commit message. Perhaps have a first \"copy/paste\" commit, and then an\n> > \"adaptation\" commit to add sort, ^ anchor in regexp, doc and your\n> > callback mechanism, but I won't insist on that.\n> \n> Good point. Creating additional commit is a bit too much for my\n> licking, but added note in commit message.\n\nI think that's fine.\n\n> +sub _credential_read {\n> +\tmy %credential;\n> +\tmy ($reader, $op) = (@_);\n> +\twhile (<$reader>) {\n> +\t\tif (!/^([^=\\s]+)=(.*?)\\s*$/) {\n> +\t\t\tthrow Error::Simple(\"unable to parse git credential $op response:\\n$_\");\n> +\t\t}\n> +\t\t$credential{$1} = $2;\n\nI think this is worse than your previous version. The spec really is as\nsimple as:\n\n while (<$reader>) {\n last if /^$/; # blank line is OK as end-of-credential\n /^([^=]+)=(.*)/\n or throw Error::Simple(...);\n $credential{$1} = {$2};\n }\n\n(actually, the spec as written does not explicitly forbid an empty key,\nbut it is nonsensical, and it might be worth updating the docs).\n\n> +sub _credential_write {\n> +\tmy ($credential, $writer) = @_;\n> +\n> +\tfor my $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} && length $credential->{$key}) {\n> +\t\t\tprint $writer $key, '=', $credential->{$key}, \"\\n\";\n> +\t\t}\n\nWhen I mentioned error-checking the format, I really just meant\nsomething like:\n\n $key =~ /[=\\n\\0]/\n and die \"BUG: credential key contains invalid characters: $key\";\n if (defined $credential->{$key}) {\n $credential->{$key} =~ /[\\n\\0]/\n and die \"BUG: credential value contains invalid characters: $credential->{key}\";\n print $writer $key, '=', $credential->{$key}, \"\\n\";\n }\n\nThose dies should never happen, and are indicative of a bug in the\ncaller. We can't even represent them in the protocol, so we might as\nwell alert the user and die rather than trying to guess what the caller\nintended.\n\n-Peff\n"}]}