{"thread":{"id":"32877","subject":"[PATCHv3 0/5] Add git-credential support to git-send-email","startedAt":"2013-02-11T16:23:34Z","lastAt":"2013-02-11T18:40:07Z","messageCount":16,"participants":["Michal Nazarewicz","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"209255","messageId":"cover.1360599057.git.mina86@mina86.com","threadId":"32877","inReplyTo":null,"subject":"[PATCHv3 0/5] Add git-credential support to git-send-email","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-11T16:23:34Z","receivedAt":"2013-02-11T16:23:34Z","isPatch":false,"sender":{"key":"mpn@google.com","avatar":null},"body":"From: Michal Nazarewicz \n\nThe third version of the patch with changes suggested by Jeff in the\n4/5 patch. Also credential_read and credential_write are now public\nfunctions in case someone wants to write a helper in perl.\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 | 166 ++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 198 insertions(+), 31 deletions(-)\n\n-- \n1.8.1.3.571.g3f8bed7\n"},{"id":"209256","messageId":"df0bb01e70629e8170b022867c6e70a8d1b88768.1360599712.git.mina86@mina86.com","threadId":"32877","inReplyTo":"cover.1360599057.git.mina86@mina86.com","subject":"[PATCHv3 1/5] Git.pm: allow command_close_bidi_pipe to be called as method","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-11T16:23:35Z","receivedAt":"2013-02-11T16:23:35Z","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.571.g3f8bed7.dirty\n"},{"id":"209257","messageId":"d8be058cc4c9a3baa68c166fd9f3333e93e3583e.1360599712.git.mina86@mina86.com","threadId":"32877","inReplyTo":"cover.1360599057.git.mina86@mina86.com","subject":"[PATCHv3 2/5] Git.pm: fix example in command_close_bidi_pipe documentation","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-11T16:23:36Z","receivedAt":"2013-02-11T16:23:36Z","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.571.g3f8bed7.dirty\n"},{"id":"209258","messageId":"b35b9286463c47f95d4c5ee91ecd4ccf4d945cba.1360599712.git.mina86@mina86.com","threadId":"32877","inReplyTo":"cover.1360599057.git.mina86@mina86.com","subject":"[PATCHv3 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-11T16:23:37Z","receivedAt":"2013-02-11T16:23:37Z","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 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.3.571.g3f8bed7.dirty\n"},{"id":"209260","messageId":"2ec5dd694878055e9ce9d650889ee85369073568.1360599712.git.mina86@mina86.com","threadId":"32877","inReplyTo":"cover.1360599057.git.mina86@mina86.com","subject":"[PATCHv3 4/5] Git.pm: add interface for git credential command","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-11T16:23:38Z","receivedAt":"2013-02-11T16:23:38Z","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 | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 147 insertions(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 9dded54..0e6fcf9 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 credential_read credential_write);\n \n \n =head1 DESCRIPTION\n@@ -1013,6 +1014,151 @@ sub _close_cat_blob {\n }\n \n \n+=item credential_read( FILE_HANDLE )\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 a hash with all read values. Any\n+white 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_read( FILE_HANDLE, CREDENTIAL_HASH )\n+\n+Writes credential key-value pairs from hash referenced by C\n+to C. Keys and values cannot contain new-line or NUL byte\n+characters, and key cannot contain equal sign nor be empty (if they do\n+Error::Simple is thrown). Any white space is preserved. If value for a key\n+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+\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} 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.3.571.g3f8bed7.dirty\n"},{"id":"209259","messageId":"fd7997960cad569d57f5330f2416f702db414169.1360599712.git.mina86@mina86.com","threadId":"32877","inReplyTo":"cover.1360599057.git.mina86@mina86.com","subject":"[PATCHv3 5/5] git-send-email: use git credential to obtain password","fromName":"Michal Nazarewicz","fromEmail":"mpn@google.com","sentAt":"2013-02-11T16:23:39Z","receivedAt":"2013-02-11T16:23:39Z","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.571.g3f8bed7.dirty\n"},{"id":"209266","messageId":"20130211165136.GC16402@sigill.intra.peff.net","threadId":"32877","inReplyTo":"cover.1360599057.git.mina86@mina86.com","subject":"Re: [PATCHv3 0/5] Add git-credential support to git-send-email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-11T16:51:36Z","receivedAt":"2013-02-11T16:51:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 11, 2013 at 05:23:34PM +0100, Michal Nazarewicz wrote:\n\n> From: Michal Nazarewicz \n> \n> The third version of the patch with changes suggested by Jeff in the\n> 4/5 patch. Also credential_read and credential_write are now public\n> functions in case someone wants to write a helper in perl.\n\nThanks, the changes you made look good. And I think it's a good idea to\nmake the read/write functions public.\n\nI have two minor comments, which I'll reply inline with. But even with\nthose comments, I think this would be OK to merge.\n\n-Peff\n"},{"id":"209267","messageId":"20130211165331.GD16402@sigill.intra.peff.net","threadId":"32877","inReplyTo":"2ec5dd694878055e9ce9d650889ee85369073568.1360599712.git.mina86@mina86.com","subject":"Re: [PATCHv3 4/5] Git.pm: add interface for git credential command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-11T16:53:31Z","receivedAt":"2013-02-11T16:53:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 11, 2013 at 05:23:38PM +0100, Michal Nazarewicz wrote:\n\n> +=item credential_read( FILE_HANDLE )\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 a hash with all read values. Any\n> +white 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\nShould this return a hash reference? It seems like that is how we end up\nusing and passing it elsewhere (since we have to anyway when passing it\nas a parameter).\n\nI don't have a strong preference, and it's somewhat a matter of taste.\nAnd maybe returning the actual hash matches the rest of the module\nbetter. I admit I don't really use Git.pm much.\n\n-Peff\n"},{"id":"209268","messageId":"20130211170134.GE16402@sigill.intra.peff.net","threadId":"32877","inReplyTo":"fd7997960cad569d57f5330f2416f702db414169.1360599712.git.mina86@mina86.com","subject":"Re: [PATCHv3 5/5] git-send-email: use git credential to obtain password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-11T17:01:34Z","receivedAt":"2013-02-11T17:01:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 11, 2013 at 05:23:39PM +0100, Michal Nazarewicz wrote:\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\nWhat do we want to do about this TODO?\n\nI am happy to put it off until it becomes a problem, but I wonder if the\nGit::credential() interface is sufficient to express what we would want.\nIt only allows two return values: true for approve, false for reject.\nBut we would want a tri-state: approve, reject, indeterminate.\n\nReading the Net::SMTP code, it doesn't look like the information is even\navailable to us (it really just passes out success or failure), so I\ndon't think we can even make it work now. But it may be better to\nprepare the public Git::credential interface for it now, so we do not\nhave to deal with breaking compatibility later.\n\n-Peff\n"},{"id":"209272","messageId":"xa1tr4kmg4cv.fsf@mina86.com","threadId":"32877","inReplyTo":"20130211165331.GD16402@sigill.intra.peff.net","subject":"Re: [PATCHv3 4/5] Git.pm: add interface for git credential command","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2013-02-11T17:14:24Z","receivedAt":"2013-02-11T17:14:24Z","isPatch":false,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"On Mon, Feb 11 2013, Jeff King wrote:\n> On Mon, Feb 11, 2013 at 05:23:38PM +0100, Michal Nazarewicz wrote:\n>\n>> +=item credential_read( FILE_HANDLE )\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 a hash with all read values. Any\n>> +white 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> Should this return a hash reference? It seems like that is how we end up\n> using and passing it elsewhere (since we have to anyway when passing it\n> as a parameter).\n\nAdmittedly I mostly just copied what git-remote-mediawiki did here and\ndon't really have any preference either way, even though with this\nfunction returning a reference the call site would have to become:\n\n %$credential = %{ credential_read $reader };\n\nAnother alternative would be for it to take a reference as an argument,\npossibly an optional one:\n\n+sub credential_read {\n+\tmy ($self, $reader, $ret) = (_maybe_self(@_), {});\n+\tmy %credential;\n+\twhile (<$reader>) {\n+\t\t# ...\n+\t}\n+\t%$ret = %credential;\n+\t$ret;\n+}\n\nI'd avoid modifying the hash while reading though since I think it's\nbest if it's left intact in case of an error.\n\nAnd of course, if we want to get even more crazy, credential_write could\naccept either reference or a hash, like so:\n\n+sub credential_write {\n+\tmy ($self, $writer, @rest) = _maybe_self(@_);\n+\tmy $credential = @rest == 1 ? $rest[0] : { @rest };\n+\tmy ($key, $value);\n+\t# ...\n+}\n\nBottom line is, anything can be coded, but a question is whether it\nmakes sense to do so. ;)\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":"209274","messageId":"xa1tmwvag47s.fsf@mina86.com","threadId":"32877","inReplyTo":"20130211170134.GE16402@sigill.intra.peff.net","subject":"Re: [PATCHv3 5/5] git-send-email: use git credential to obtain password","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2013-02-11T17:17:27Z","receivedAt":"2013-02-11T17:17:27Z","isPatch":false,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"> On Mon, Feb 11, 2013 at 05:23:39PM +0100, Michal Nazarewicz wrote:\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\nOn Mon, Feb 11 2013, Jeff King wrote:\n> What do we want to do about this TODO?\n>\n> I am happy to put it off until it becomes a problem, but I wonder if the\n> Git::credential() interface is sufficient to express what we would want.\n> It only allows two return values: true for approve, false for reject.\n> But we would want a tri-state: approve, reject, indeterminate.\n\nBeing it tri-state is not a problem. The last can be easily represented\nby undef.\n\n> Reading the Net::SMTP code, it doesn't look like the information is even\n> available to us (it really just passes out success or failure), so I\n> don't think we can even make it work now. But it may be better to\n> prepare the public Git::credential interface for it now, so we do not\n> have to deal with breaking compatibility later.\n\nI guess. I left it as is since git-send-email won't make use of the\nindeterminate values, but I can add it in this patchset as well.\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":"209275","messageId":"xa1tk3qeg46r.fsf@mina86.com","threadId":"32877","inReplyTo":"20130211165136.GC16402@sigill.intra.peff.net","subject":"Re: [PATCHv3 0/5] Add git-credential support to git-send-email","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2013-02-11T17:18:04Z","receivedAt":"2013-02-11T17:18:04Z","isPatch":false,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"On Mon, Feb 11 2013, Jeff King wrote:\n> I have two minor comments, which I'll reply inline with. But even with\n> those comments, I think this would be OK to merge.\n\nI'll send a new patchset tomorrow with.\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":"209281","messageId":"20130211173135.GI16402@sigill.intra.peff.net","threadId":"32877","inReplyTo":"xa1tmwvag47s.fsf@mina86.com","subject":"Re: [PATCHv3 5/5] git-send-email: use git credential to obtain password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-11T17:31:35Z","receivedAt":"2013-02-11T17:31:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 11, 2013 at 06:17:27PM +0100, Michal Nazarewicz wrote:\n\n> > I am happy to put it off until it becomes a problem, but I wonder if the\n> > Git::credential() interface is sufficient to express what we would want.\n> > It only allows two return values: true for approve, false for reject.\n> > But we would want a tri-state: approve, reject, indeterminate.\n> \n> Being it tri-state is not a problem. The last can be easily represented\n> by undef.\n\nYeah, I think undef makes sense there.\n\n> > Reading the Net::SMTP code, it doesn't look like the information is even\n> > available to us (it really just passes out success or failure), so I\n> > don't think we can even make it work now. But it may be better to\n> > prepare the public Git::credential interface for it now, so we do not\n> > have to deal with breaking compatibility later.\n> \n> I guess. I left it as is since git-send-email won't make use of the\n> indeterminate values, but I can add it in this patchset as well.\n\nYeah, I am more worried about third-party uses outside of the Git tree,\nwhich we may then break if we change the meaning of \"undef\" later.\nThanks.\n\n-Peff\n"},{"id":"209282","messageId":"20130211173632.GJ16402@sigill.intra.peff.net","threadId":"32877","inReplyTo":"xa1tr4kmg4cv.fsf@mina86.com","subject":"Re: [PATCHv3 4/5] Git.pm: add interface for git credential command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-11T17:36:32Z","receivedAt":"2013-02-11T17:36:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 11, 2013 at 06:14:24PM +0100, Michal Nazarewicz wrote:\n\n> > Should this return a hash reference? It seems like that is how we end up\n> > using and passing it elsewhere (since we have to anyway when passing it\n> > as a parameter).\n> \n> Admittedly I mostly just copied what git-remote-mediawiki did here and\n> don't really have any preference either way, even though with this\n> function returning a reference the call site would have to become:\n> \n> %$credential = %{ credential_read $reader };\n\nOh, right, because Git::credential takes the credential as an in-out\nparameter rather than just returning it. Which is a bit unusual in perl,\nbut keeps the interface reasonably simple. The alternative would be:\n\n $cred = Git::credential $cred, sub {\n ...\n }\n\nwhich is a little less nice.\n\n> Another alternative would be for it to take a reference as an argument,\n> possibly an optional one:\n\nI think that is making things more ugly.\n\n> I'd avoid modifying the hash while reading though since I think it's\n> best if it's left intact in case of an error.\n\nAgreed.\n\n> And of course, if we want to get even more crazy, credential_write could\n> accept either reference or a hash, like so:\n> \n> +sub credential_write {\n> +\tmy ($self, $writer, @rest) = _maybe_self(@_);\n> +\tmy $credential = @rest == 1 ? $rest[0] : { @rest };\n> +\tmy ($key, $value);\n> +\t# ...\n> +}\n\nUgh.\n\n> Bottom line is, anything can be coded, but a question is whether it\n> makes sense to do so. ;)\n\nYes, it is probably OK to leave it as-is, then. It is largely a matter\nof taste, and I will defer to your judgement on that. :)\n\n-Peff\n"},{"id":"209283","messageId":"20130211174811.GK16402@sigill.intra.peff.net","threadId":"32877","inReplyTo":"xa1tk3qeg46r.fsf@mina86.com","subject":"Re: [PATCHv3 0/5] Add git-credential support to git-send-email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-11T17:48:11Z","receivedAt":"2013-02-11T17:48:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 11, 2013 at 06:18:04PM +0100, Michal Nazarewicz wrote:\n\n> On Mon, Feb 11 2013, Jeff King wrote:\n> > I have two minor comments, which I'll reply inline with. But even with\n> > those comments, I think this would be OK to merge.\n> \n> I'll send a new patchset tomorrow with.\n\nBased on our discussion, I think it would just need the patch below\nsquashed into your 4/5 (this handles the \"undef\" thing, and I also fixed\na few typos in the API documentation):\n\n---\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 0e6fcf9..35893e6 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1038,7 +1038,7 @@ sub credential_read {\n \treturn %credential;\n }\n \n-=item credential_read( FILE_HANDLE, CREDENTIAL_HASH )\n+=item credential_write( FILE_HANDLE, CREDENTIAL_HASH )\n \n Writes credential key-value pairs from hash referenced by C\n to C. Keys and values cannot contain new-line or NUL byte\n@@ -1102,7 +1102,7 @@ sub _credential_run {\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+specified operation. In both forms C needs to be\n a reference to a hash which stores credentials. Under certain\n conditions the hash can change.\n \n@@ -1126,11 +1126,14 @@ sub _credential_run {\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+The function will execute C to fill the provided\n+credential hash, then call C with C as the sole\n+argument. If C's return value is defined, the function will\n+execute C (if return value yields true) or\n+C (if return value is false). If the return\n+value is undef, nothing at all is executed; this is useful, for\n+example, if the credential could neither be verified nor rejected due\n+to an unrelated network error. 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@@ -1152,7 +1155,9 @@ sub credential {\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\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"},{"id":"209288","messageId":"xa1td2w6g0e0.fsf@mina86.com","threadId":"32877","inReplyTo":"20130211174811.GK16402@sigill.intra.peff.net","subject":"Re: [PATCHv3 0/5] Add git-credential support to git-send-email","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2013-02-11T18:40:07Z","receivedAt":"2013-02-11T18:40:07Z","isPatch":false,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"On Mon, Feb 11 2013, Jeff King wrote:\n> Based on our discussion, I think it would just need the patch below\n> squashed into your 4/5 (this handles the \"undef\" thing, and I also fixed\n> a few typos in the API documentation):\n\n> @@ -1152,7 +1155,9 @@ sub credential {\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\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\nYep, that's what I did as well. Thanks for spotting the typos,\nI actually changed some other wording as well (most notably\nCREDENTIAL_HASH -> CREDENTIAL_HASHREF), and also added some unrelated\npatch in the middle: .\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"}]}