Make git-send-email use git-credential
v2 cover Make git-send-email use git-credential
From: Michal Nazarewicz <[email protected]>
Minor fixes as suggested in emails.
Michal Nazarewicz (5):
Git.pm: allow command_close_bidi_pipe to be called as method Git.pm: fix example in command_close_bidi_pipe documentation Git.pm: allow pipes to be closed prior to calling
command_close_bidi_pipe
Git.pm: add interface for git credential command
git-send-email: use git credential to obtain password
Documentation/git-send-email.txt | 4 +- git-send-email.perl | 59 ++++++++++-------- perl/Git.pm | 129 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 161 insertions(+), 31 deletions(-)
Signature · 1 line
1.8.1.2.549.g1d13f9f
PATCH v2 1/5 Git.pm: allow command_close_bidi_pipe to be called as method
From: Michal Nazarewicz <[email protected]>
The documentation of command_close_bidi_pipe() claims that it can be called as a method, but it does not check whether the first argument is $self or not assuming the latter. Using _maybe_self() fixes this.
--- perl/Git.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
perl/Git.pm +1 -1
@@ -430,7 +430,7 @@ have more complicated structure. sub command_close_bidi_pipe { local $?; - my ($pid, $in, $out, $ctx) = @_; + my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_); foreach my $fh ($in, $out) { unless (close $fh) { if ($!) {
Signature · 1 line
1.8.1.2.549.g1d13f9f
PATCH v2 2/5 Git.pm: fix example in command_close_bidi_pipe documentation
In reply to Michal NazarewiczFrom: Michal Nazarewicz <[email protected]>
File handle goes as the first argument when calling print on it.
--- perl/Git.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
perl/Git.pm +1 -1
@@ -418,7 +418,7 @@ and it is the fourth value returned by C<command_bidi_pipe()>. The call idiom is: my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe('cat-file --batch-check'); - print "000000000\n" $out; + print $out "000000000\n"; while (<$in>) { ... } $r->command_close_bidi_pipe($pid, $in, $out, $ctx);
Signature · 1 line
1.8.1.2.549.g1d13f9f
PATCH v2 3/5 Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
In reply to Michal NazarewiczFrom: Michal Nazarewicz <[email protected]>
The command_close_bidi_pipe() function will insist on closing both input and output pipes returned by command_bidi_pipe(). With this change it is possible to close one of the pipes in advance and pass undef as an argument.
--- perl/Git.pm | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
Quoted 8 lines · Michal Nazarewicz
Quoted 7 lines
This allows for something like:
my ($pid, $in, $out, $ctx) = command_bidi_pipe(...); print $out "write data"; close $out; # ... do stuff with $in command_close_bidi_pipe($pid, $in, undef, $ctx);
Quoted 2 lines · Jeff King
Should this part go into the documentation for
command_close_bidi_pipein Git.pm?
Done.
perl/Git.pm +14 -1
@@ -426,13 +426,26 @@ Note that you should not rely on whatever actually is in C<CTX>; currently it is simply the command name but in future the context might have more complicated structure. +C<PIPE_IN> and C<PIPE_OUT> may be C<undef> if they have been closed prior to +calling this function. This may be useful in a query-response type of +commands where caller first writes a query and later reads response, eg: + + my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe('cat-file --batch-check'); + print $out "000000000\n"; + close $out; + while (<$in>) { ... } + $r->command_close_bidi_pipe($pid, $in, undef, $ctx); + +This idiom may prevent potential dead locks caused by data sent to the output +pipe not being flushed and thus not reaching the executed command. + =cut sub command_close_bidi_pipe { local $?; my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_); foreach my $fh ($in, $out) { - unless (close $fh) { + if (defined $fh && !close $fh) { if ($!) { carp "error closing pipe: $!"; } elsif ($? >> 8) {
Signature · 1 line
1.8.1.2.549.g1d13f9f
PATCH v2 4/5 Git.pm: add interface for git credential command
In reply to Michal NazarewiczFrom: Michal Nazarewicz <[email protected]>
Add a credential() function which is an interface to the git credential command. The code is heavily based on credential_* functions in <contrib/mw-to-git/git-remote-mediawiki>.
--- perl/Git.pm | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-)
Quoted 13 lines · Jeff King
Review
Quoted 6 lines · Michal Nazarewicz
(quoted diff) +6
-0+sub _credential_read { + my %credential; + my ($reader, $op) = (@_); + while (<$reader>) { + chomp; + my ($key, $value) = /([^=]*)=(.*)/;Empty keys are not valid. Can we make this:
/^([^=]+)=(.*)/
Quoted 7 lines
to fail the regex? Otherwise, I think this check:
Review
Quoted 3 lines
(quoted diff) +3
-0+ if (not defined $key) { + throw Error::Simple("unable to parse git credential $op response:\n$_\n"); + }would not pass because $key would be the empty string.
Right, fixed.
Quoted 9 lines
Quoted 9 lines
(quoted diff) +9
-0+sub _credential_write { + my ($credential, $writer) = @_; + + for my $key (sort { + # url overwrites other fields, so it must come first + return -1 if $a eq 'url'; + return 1 if $b eq 'url'; + return $a cmp $b; + } keys %$credential) {
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ if (defined $credential->{$key} && length $credential->{$key}) { + print $writer $key, '=', $credential->{$key}, "\n"; + } + }There are a few disallowed characters, like "\n" in key or value, and "=" in a key. They should never happen unless the caller is buggy, but should we check and catch them here?
I left it as is for now since it's not entairly clear to me what to do in all cases. In particular:
- when reading, what to do if the line is " foo = bar ",
- when reading, what to do if the line is "foo=" (ie. empty value),
- when writing, what to do if value is a single space,
- when writing, what to do if value ends with a new line,
- when writing, what to do if value is empty (currently not printed at all),
Quoted 4 lines · Matthieu Moy
I think you should credit git-remote-mediawiki for the code in the commit message. Perhaps have a first "copy/paste" commit, and then an "adaptation" commit to add sort, ^ anchor in regexp, doc and your callback mechanism, but I won't insist on that.
Good point. Creating additional commit is a bit too much for my licking, but added note in commit message.
perl/Git.pm +109 -1
@@ -59,7 +59,8 @@ require Exporter; command_bidi_pipe command_close_bidi_pipe version exec_path html_path hash_object git_cmd_try remote_refs prompt - temp_acquire temp_release temp_reset temp_path); + temp_acquire temp_release temp_reset temp_path + credential); =head1 DESCRIPTION
@@ -1013,6 +1014,113 @@ sub _close_cat_blob { } +sub _credential_read { + my %credential; + my ($reader, $op) = (@_); + while (<$reader>) { + if (!/^([^=\s]+)=(.*?)\s*$/) { + throw Error::Simple("unable to parse git credential $op response:\n$_"); + } + $credential{$1} = $2; + } + return %credential; +} + +sub _credential_write { + my ($credential, $writer) = @_; + + for my $key (sort { + # url overwrites other fields, so it must come first + return -1 if $a eq 'url'; + return 1 if $b eq 'url'; + return $a cmp $b; + } keys %$credential) { + if (defined $credential->{$key} && length $credential->{$key}) { + print $writer $key, '=', $credential->{$key}, "\n"; + } + } + print $writer "\n"; +} + +sub _credential_run { + my ($self, $credential, $op) = _maybe_self(@_); + + my ($pid, $reader, $writer, $ctx) = command_bidi_pipe('credential', $op); + + _credential_write $credential, $writer; + close $writer; + + if ($op eq "fill") { + %$credential = _credential_read $reader, $op; + } elsif (<$reader>) { + throw Error::Simple("unexpected output from git credential $op response:\n$_\n"); + } + + command_close_bidi_pipe($pid, $reader, undef, $ctx); +} + +=item credential( CREDENTIAL_HASH [, OPERATION ] ) + +=item credential( CREDENTIAL_HASH, CODE ) + +Executes C<git credential> for a given set of credentials and +specified operation. In both form C<CREDENTIAL_HASH> needs to be +a reference to a hash which stores credentials. Under certain +conditions the hash can change. + +In the first form, C<OPERATION> can be C<'fill'> (or omitted), +C<'approve'> or C<'reject'>, and function will execute corresponding +C<git credential> sub-command. In case of C<'fill'> the values stored +in C<CREDENTIAL_HASH> will be changed to the ones returned by the +C<git credential> command. The usual usage would look something like: + + my %cred = ( + 'protocol' => 'https', + 'host' => 'example.com', + 'username' => 'bob' + ); + Git::credential \%cred; + if (try_to_authenticate($cred{'username'}, $cred{'password'})) { + Git::credential \%cred, 'approve'; + ... do more stuff ... + } else { + Git::credential \%cred, 'reject'; + } + +In the second form, C<CODE> needs to be a reference to a subroutine. +The function will execute C<git credential fill> to fill provided +credential hash, than call C<CODE> with C<CREDENTIAL_HASH> as the sole +argument, and finally depending on C<CODE>'s return value execute +C<git credential approve> (if return value yields true) or C<git +credential reject> (otherwise). The return value is the same as what +C<CODE> returned. With this form, the usage might look as follows: + + if (Git::credential { + 'protocol' => 'https', + 'host' => 'example.com', + 'username' => 'bob' + }, sub { + my $cred = shift; + return try_to_authenticate($cred->{'username'}, $cred->{'password'}); + }) { + ... do more stuff ... + } + +=cut + +sub credential { + my ($self, $credential, $op_or_code) = (_maybe_self(@_), 'fill'); + + if ('CODE' eq ref $op_or_code) { + _credential_run $credential, 'fill'; + my $ret = $op_or_code->($credential); + _credential_run $credential, $ret ? 'approve' : 'reject'; + return $ret; + } else { + _credential_run $credential, $op_or_code; + } +} + { # %TEMP_* Lexical Context my (%TEMP_FILEMAP, %TEMP_FILES);
Signature · 1 line
1.8.1.2.549.g1d13f9f
Re: [PATCHv2 4/5] Git.pm: add interface for git credential command
Quoted 8 lines · Michal Nazarewicz
Quoted 3 lines
There are a few disallowed characters, like "\n" in key or value, and "=" in a key. They should never happen unless the caller is buggy, but should we check and catch them here?
I left it as is for now since it's not entairly clear to me what to do in all cases. In particular:
- when reading, what to do if the line is " foo = bar ",
According to the spec, whitespace (except for the final newline) is not significant, and that parses key=" foo ", value=" bar ". The spec could ignore whitespace on the key side, but I intentionally did not in an attempt to keep the protocol simple. Your original implementation did the right thing already.
Quoted 1 line
- when reading, what to do if the line is "foo=" (ie. empty value),
The empty string is a valid value.
Quoted 1 line
- when writing, what to do if value is a single space,
Then it's a single space. It's the caller's problem whether that is an issue or not.
Quoted 1 line
- when writing, what to do if value ends with a new line,
That's bogus. We cannot represent that value. I'd suggest to simply die, as it is a bug in the caller (we _could_ try to be nice and assume the caller accidentally forgot to chomp, but I'd rather be careful than nice).
Quoted 1 line
- when writing, what to do if value is empty (currently not printed at all),
I think you should still print it. It's unlikely to matter, but technically a helper response may override keys (or set them to blank), and the intermediate state gets sent on to the next helper, if there are multiple.
Quoted 8 lines · Matthieu Moy
Quoted 4 lines
I think you should credit git-remote-mediawiki for the code in the commit message. Perhaps have a first "copy/paste" commit, and then an "adaptation" commit to add sort, ^ anchor in regexp, doc and your callback mechanism, but I won't insist on that.
Good point. Creating additional commit is a bit too much for my licking, but added note in commit message.
I think that's fine.
perl/Git.pm sub _close_cat_blob { · line 1024
Quoted 8 lines
perl/Git.pm +8
-0+sub _credential_read { + my %credential; + my ($reader, $op) = (@_); + while (<$reader>) { + if (!/^([^=\s]+)=(.*?)\s*$/) { + throw Error::Simple("unable to parse git credential $op response:\n$_"); + } + $credential{$1} = $2;
I think this is worse than your previous version. The spec really is as simple as:
while (<$reader>) {
last if /^$/; # blank line is OK as end-of-credential
/^([^=]+)=(.*)/
or throw Error::Simple(...);
$credential{$1} = {$2};
}(actually, the spec as written does not explicitly forbid an empty key, but it is nonsensical, and it might be worth updating the docs).
perl/Git.pm sub _close_cat_blob { · line 1040
Quoted 12 lines
perl/Git.pm +12
-0+sub _credential_write { + my ($credential, $writer) = @_; + + for my $key (sort { + # url overwrites other fields, so it must come first + return -1 if $a eq 'url'; + return 1 if $b eq 'url'; + return $a cmp $b; + } keys %$credential) { + if (defined $credential->{$key} && length $credential->{$key}) { + print $writer $key, '=', $credential->{$key}, "\n"; + }
When I mentioned error-checking the format, I really just meant something like:
$key =~ /[=\n\0]/
and die "BUG: credential key contains invalid characters: $key";
if (defined $credential->{$key}) {
$credential->{$key} =~ /[\n\0]/
and die "BUG: credential value contains invalid characters: $credential->{key}";
print $writer $key, '=', $credential->{$key}, "\n";
}Those dies should never happen, and are indicative of a bug in the caller. We can't even represent them in the protocol, so we might as well alert the user and die rather than trying to guess what the caller intended.
-Peff
Re: [PATCHv2 4/5] Git.pm: add interface for git credential command
In reply to Michal NazarewiczQuoted 5 lines · Michal Nazarewicz
From: Michal Nazarewicz <[email protected]>
Add a
credential()function which is an interface to the git credential command. The code is heavily based on credential_* functions in <contrib/mw-to-git/git-remote-mediawiki>.
I'm no perl expert, so I cannot comment much on style (there are many small changes compared to the mediawiki code that look like improvement though), but:
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
Re: [PATCHv2 4/5] Git.pm: add interface for git credential command
Quoted 8 lines · Matthieu Moy
Quoted 5 lines · Michal Nazarewicz
From: Michal Nazarewicz <[email protected]>
Add a
credential()function which is an interface to the git credential command. The code is heavily based on credential_* functions in <contrib/mw-to-git/git-remote-mediawiki>.
Quoted 5 lines
I'm no perl expert, so I cannot comment much on style (there are many small changes compared to the mediawiki code that look like improvement though), but:
Reviewed-by Matthieu Moy <[email protected]>
Thanks. I'd actually be more worried about the error checking issue Peff raised during his review. I have a feeling that "when in doubt, do not cause harm" is a more prudent way to go than "I do not know, so I'll let anything pass".
Re: [PATCHv2 4/5] Git.pm: add interface for git credential command
Quoted 4 lines · Junio C Hamano
I'd actually be more worried about the error checking issue Peff raised during his review. I have a feeling that "when in doubt, do not cause harm" is a more prudent way to go than "I do not know, so I'll let anything pass".
I can implement whatever checking you wish, just tell me what to do in corner cases I've listed. ;)
Signature · 4 lines
Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +----<email/xmpp: [email protected]>--------------ooO--(_)--Ooo--
PATCH v2 5/5 git-send-email: use git credential to obtain password
In reply to Michal NazarewiczFrom: Michal Nazarewicz <[email protected]>
If smtp_user is provided but smtp_pass is not, instead of prompting for password, make git-send-email use git credential command instead.
--- Documentation/git-send-email.txt | 4 +-- git-send-email.perl | 59 +++++++++++++++++++++++----------------- 2 files changed, 36 insertions(+), 27 deletions(-)
Documentation/git-send-email.txt +2 -2
@@ -164,8 +164,8 @@ Sending Furthermore, passwords need not be specified in configuration files or on the command line. If a username has been specified (with '--smtp-user' or a 'sendemail.smtpuser'), but no password has been -specified (with '--smtp-pass' or 'sendemail.smtppass'), then the -user is prompted for a password while the input is masked for privacy. +specified (with '--smtp-pass' or 'sendemail.smtppass'), then +a password is obtained using 'git-credential'. --smtp-server=<host>:: If set, specifies the outgoing SMTP server to use (e.g.
git-send-email.perl +34 -25
71 lines not on this page · raw message
Signature · 1 line
1.8.1.2.549.g1d13f9f
Re: [PATCHv2 5/5] git-send-email: use git credential to obtain password
Quoted 8 lines · Michal Nazarewicz
From: Michal Nazarewicz <[email protected]>
If
smtp_useris provided butsmtp_passis not, instead of prompting for password, make git-send-email use git credential command instead.Signed-off-by Michal Nazarewicz <[email protected]>---
Nice ;-)
I'd expect reviews on 4/5 from Peff and Matthiew which may result in either Reviewed-by:'s or another round, but everything else looks in good order.
Thanks to all three of you for working on this.
Quoted 96 lines
Documentation/git-send-email.txt | 4 +-- git-send-email.perl | 59 +++++++++++++++++++++++----------------- 2 files changed, 36 insertions(+), 27 deletions(-)Documentation/git-send-email.txt +2
-2@@ -164,8 +164,8 @@ Sending Furthermore, passwords need not be specified in configuration files or on the command line. If a username has been specified (with '--smtp-user' or a 'sendemail.smtpuser'), but no password has been -specified (with '--smtp-pass' or 'sendemail.smtppass'), then the -user is prompted for a password while the input is masked for privacy. +specified (with '--smtp-pass' or 'sendemail.smtppass'), then +a password is obtained using 'git-credential'. --smtp-server=<host>:: If set, specifies the outgoing SMTP server to use (e.g.git-send-email.perl +34
-25@@ -1045,6 +1045,39 @@ sub maildomain { return maildomain_net() || maildomain_mta() || 'localhost.localdomain'; } +# Returns 1 if authentication succeeded or was not necessary +# (smtp_user was not specified), and 0 otherwise. + +sub smtp_auth_maybe { + if (!defined $smtp_authuser || $auth) { + return 1; + } + + # Workaround AUTH PLAIN/LOGIN interaction defect + # with Authen::SASL::Cyrus + eval { + require Authen::SASL; + Authen::SASL->import(qw(Perl)); + }; + + # TODO: Authentication may fail not because credentials were + # invalid but due to other reasons, in which we should not + # reject credentials. + $auth = Git::credential({ + 'protocol' => 'smtp', + 'host' => join(':', $smtp_server, $smtp_server_port), + 'username' => $smtp_authuser, + # if there's no password, "git credential fill" will + # give us one, otherwise it'll just pass this one. + 'password' => $smtp_authpass + }, sub { + my $cred = shift; + return !!$smtp->auth($cred->{'username'}, $cred->{'password'}); + }); + + return $auth; +} + # Returns 1 if the message was sent, and 0 otherwise. # In actuality, the whole program dies when there # is an error sending a message.@@ -1185,31 +1218,7 @@ X-Mailer: git-send-email $gitversion defined $smtp_server_port ? " port=$smtp_server_port" : ""; } - if (defined $smtp_authuser) { - # Workaround AUTH PLAIN/LOGIN interaction defect - # with Authen::SASL::Cyrus - eval { - require Authen::SASL; - Authen::SASL->import(qw(Perl)); - }; - - if (!defined $smtp_authpass) { - - system "stty -echo"; - - do { - print "Password: "; - $_ = <STDIN>; - print "\n"; - } while (!defined $_); - - chomp($smtp_authpass = $_); - - system "stty echo"; - } - - $auth ||= $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message; - } + smtp_auth_maybe or die $smtp->message; $smtp->mail( $raw_from ) or die $smtp->message; $smtp->to( @recipients ) or die $smtp->message;