Add git-credential support to git-send-email
v3 cover Add git-credential support to git-send-email
From: Michal Nazarewicz <[email protected]>
The third version of the patch with changes suggested by Jeff in the 4/5 patch. Also credential_read and credential_write are now public functions in case someone wants to write a helper in perl.
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 | 166 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 198 insertions(+), 31 deletions(-)
Signature · 1 line
1.8.1.3.571.g3f8bed7
Quoted 5 lines · Michal Nazarewicz
From: Michal Nazarewicz <[email protected]>
The third version of the patch with changes suggested by Jeff in the 4/5 patch. Also
credential_readandcredential_writeare now public functions in case someone wants to write a helper in perl.
Thanks, the changes you made look good. And I think it's a good idea to make the read/write functions public.
I have two minor comments, which I'll reply inline with. But even with those comments, I think this would be OK to merge.
-Peff
Quoted 2 lines · Jeff King
I have two minor comments, which I'll reply inline with. But even with those comments, I think this would be OK to merge.
I'll send a new patchset tomorrow with.
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--
Quoted 5 lines · Michal Nazarewicz
Quoted 2 lines · Jeff King
I have two minor comments, which I'll reply inline with. But even with those comments, I think this would be OK to merge.
I'll send a new patchset tomorrow with.
Based on our discussion, I think it would just need the patch below squashed into your 4/5 (this handles the "undef" thing, and I also fixed a few typos in the API documentation):
---
perl/Git.pm +13 -8
@@ -1038,7 +1038,7 @@ sub credential_read { return %credential; } -=item credential_read( FILE_HANDLE, CREDENTIAL_HASH ) +=item credential_write( FILE_HANDLE, CREDENTIAL_HASH ) Writes credential key-value pairs from hash referenced by C<CREDENTIAL_HASH> to C<FILE_HANDLE>. Keys and values cannot contain new-line or NUL byte
@@ -1102,7 +1102,7 @@ sub _credential_run { =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 +specified operation. In both forms C<CREDENTIAL_HASH> needs to be a reference to a hash which stores credentials. Under certain conditions the hash can change.
@@ -1126,11 +1126,14 @@ sub _credential_run { } 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> 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 +The function will execute C<git credential fill> to fill the provided +credential hash, then call C<CODE> with C<CREDENTIAL> as the sole +argument. If C<CODE>'s return value is defined, the function will +execute C<git credential approve> (if return value yields true) or +C<git credential reject> (if return value is false). If the return +value is undef, nothing at all is executed; this is useful, for +example, if the credential could neither be verified nor rejected due +to an unrelated network error. The return value is the same as what C<CODE> returned. With this form, the usage might look as follows: if (Git::credential {
@@ -1152,7 +1155,9 @@ sub credential { if ('CODE' eq ref $op_or_code) { _credential_run $credential, 'fill'; my $ret = $op_or_code->($credential); - _credential_run $credential, $ret ? 'approve' : 'reject'; + if (defined $ret) { + _credential_run $credential, $ret ? 'approve' : 'reject'; + } return $ret; } else { _credential_run $credential, $op_or_code;
Quoted 5 lines · Jeff King
Based on our discussion, I think it would just need the patch below squashed into your 4/5 (this handles the "undef" thing, and I also fixed a few typos in the API documentation):
perl/Git.pm sub credential { · line 1163
Quoted 10 lines
perl/Git.pm +3
-1@@ -1152,7 +1155,9 @@ sub credential { if ('CODE' eq ref $op_or_code) { _credential_run $credential, 'fill'; my $ret = $op_or_code->($credential); - _credential_run $credential, $ret ? 'approve' : 'reject'; + if (defined $ret) { + _credential_run $credential, $ret ? 'approve' : 'reject'; + } return $ret; } else { _credential_run $credential, $op_or_code;
Yep, that's what I did as well. Thanks for spotting the typos, I actually changed some other wording as well (most notably CREDENTIAL_HASH -> CREDENTIAL_HASHREF), and also added some unrelated patch in the middle: <https://github.com/mina86/git/commits/master>.
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 v3 1/5 Git.pm: allow command_close_bidi_pipe to be called as method
In reply to Michal NazarewiczFrom: 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.3.571.g3f8bed7.dirty
PATCH v3 2/5 Git.pm: fix example in command_close_bidi_pipe documentation
In reply to Michal NazarewiczFrom: Michal Nazarewicz <[email protected]>
--- 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.3.571.g3f8bed7.dirty
PATCH v3 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(-)
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.3.571.g3f8bed7.dirty
PATCH v3 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 | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 147 insertions(+), 1 deletion(-)
perl/Git.pm +147 -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 credential_read credential_write); =head1 DESCRIPTION
@@ -1013,6 +1014,151 @@ sub _close_cat_blob { } +=item credential_read( FILE_HANDLE ) + +Reads credential key-value pairs from C<FILE_HANDLE>. Reading stops at EOF or +when an empty line is encountered. Each line must be of the form C<key=value> +with a non-empty key. Function returns a hash with all read values. Any +white space (other then new-line character) is preserved. + +=cut + +sub credential_read { + my ($self, $reader) = _maybe_self(@_); + my %credential; + while (<$reader>) { + chomp; + if ($_ eq '') { + last; + } elsif (!/^([^=]+)=(.*)$/) { + throw Error::Simple("unable to parse git credential data:\n$_"); + } + $credential{$1} = $2; + } + return %credential; +} + +=item credential_read( FILE_HANDLE, CREDENTIAL_HASH ) + +Writes credential key-value pairs from hash referenced by C<CREDENTIAL_HASH> +to C<FILE_HANDLE>. Keys and values cannot contain new-line or NUL byte +characters, and key cannot contain equal sign nor be empty (if they do +Error::Simple is thrown). Any white space is preserved. If value for a key +is C<undef>, it will be skipped. + +If C<'url'> key exists it will be written first. (All the other key-value +pairs are written in sorted order but you should not depend on that). Once +all lines are written, an empty line is printed. + +=cut + +sub credential_write { + my ($self, $writer, $credential) = _maybe_self(@_); + my ($key, $value); + + # Check if $credential is valid prior to writing anything + while (($key, $value) = each %$credential) { + if (!defined $key || !length $key) { + throw Error::Simple("credential key empty or undefined"); + } elsif ($key =~ /[=\n\0]/) { + throw Error::Simple("credential key contains invalid characters: $key"); + } elsif (defined $value && $value =~ /[\n\0]/) { + throw Error::Simple("credential value for key=$key contains invalid characters: $value"); + } + } + + for $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}) { + 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 $writer, $credential; + close $writer; + + if ($op eq "fill") { + %$credential = credential_read $reader; + } 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> 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.3.571.g3f8bed7.dirty
Re: [PATCHv3 4/5] Git.pm: add interface for git credential command
Quoted 13 lines · Michal Nazarewicz
+=item
credential_read( FILE_HANDLE )perl/Git.pm +12
-0+ +Reads credential key-value pairs from C<FILE_HANDLE>. Reading stops at EOF or +when an empty line is encountered. Each line must be of the form C<key=value> +with a non-empty key. Function returns a hash with all read values. Any +white space (other then new-line character) is preserved. + +=cut + +sub credential_read { + my ($self, $reader) = _maybe_self(@_); + my %credential; + while (<$reader>) {
perl/Git.pm sub _close_cat_blob { · line 1039
Quoted 10 lines
perl/Git.pm +10
-0+ chomp; + if ($_ eq '') { + last; + } elsif (!/^([^=]+)=(.*)$/) { + throw Error::Simple("unable to parse git credential data:\n$_"); + } + $credential{$1} = $2; + } + return %credential; +}
Should this return a hash reference? It seems like that is how we end up using and passing it elsewhere (since we have to anyway when passing it as a parameter).
I don't have a strong preference, and it's somewhat a matter of taste. And maybe returning the actual hash matches the rest of the module better. I admit I don't really use Git.pm much.
-Peff
Re: [PATCHv3 4/5] Git.pm: add interface for git credential command
Quoted 21 lines · Jeff King
Quoted 19 lines · Michal Nazarewicz
+=item
credential_read( FILE_HANDLE )perl/Git.pm +18
-0+ +Reads credential key-value pairs from C<FILE_HANDLE>. Reading stops at EOF or +when an empty line is encountered. Each line must be of the form C<key=value> +with a non-empty key. Function returns a hash with all read values. Any +white space (other then new-line character) is preserved. + +=cut + +sub credential_read { + my ($self, $reader) = _maybe_self(@_); + my %credential; + while (<$reader>) { + chomp; + if ($_ eq '') { + last; + } elsif (!/^([^=]+)=(.*)$/) { + throw Error::Simple("unable to parse git credential data:\n$_"); + }
Quoted 8 lines
perl/Git.pm sub _close_cat_blob { · line 1039
Quoted 4 lines
perl/Git.pm +4
-0+ $credential{$1} = $2; + } + return %credential; +}Should this return a hash reference? It seems like that is how we end up using and passing it elsewhere (since we have to anyway when passing it as a parameter).
Admittedly I mostly just copied what git-remote-mediawiki did here and don't really have any preference either way, even though with this function returning a reference the call site would have to become:
%$credential = %{ credential_read $reader };Another alternative would be for it to take a reference as an argument, possibly an optional one:
+sub credential_read {
+ my ($self, $reader, $ret) = (_maybe_self(@_), {});
+ my %credential;
+ while (<$reader>) {
+ # ...
+ }
+ %$ret = %credential;
+ $ret;
+}
I'd avoid modifying the hash while reading though since I think it's best if it's left intact in case of an error.
And of course, if we want to get even more crazy, credential_write could accept either reference or a hash, like so:
+sub credential_write {
+ my ($self, $writer, @rest) = _maybe_self(@_);
+ my $credential = @rest == 1 ? $rest[0] : { @rest };
+ my ($key, $value);
+ # ...
+}
Bottom line is, anything can be coded, but a question is whether it makes sense to do so. ;)
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--
Re: [PATCHv3 4/5] Git.pm: add interface for git credential command
Quoted 9 lines · Michal Nazarewicz
Quoted 3 lines
Should this return a hash reference? It seems like that is how we end up using and passing it elsewhere (since we have to anyway when passing it as a parameter).
Admittedly I mostly just copied what git-remote-mediawiki did here and don't really have any preference either way, even though with this function returning a reference the call site would have to become:
%$credential = %{ credential_read $reader };
Oh, right, because Git::credential takes the credential as an in-out parameter rather than just returning it. Which is a bit unusual in perl, but keeps the interface reasonably simple. The alternative would be:
$cred = Git::credential $cred, sub {
...
}which is a little less nice.
Quoted 2 lines
Another alternative would be for it to take a reference as an argument, possibly an optional one:
I think that is making things more ugly.
Quoted 2 lines
I'd avoid modifying the hash while reading though since I think it's best if it's left intact in case of an error.
Agreed.
perl/Git.pm sub _close_cat_blob { · line 1067
Quoted 9 lines
And of course, if we want to get even more crazy,
credential_writecould accept either reference or a hash, like so:perl/Git.pm +6
-0+sub credential_write { + my ($self, $writer, @rest) = _maybe_self(@_); + my $credential = @rest == 1 ? $rest[0] : { @rest }; + my ($key, $value); + # ... +}
Ugh.
Quoted 2 lines
Bottom line is, anything can be coded, but a question is whether it makes sense to do so. ;)
Yes, it is probably OK to leave it as-is, then. It is largely a matter of taste, and I will defer to your judgement on that. :)
-Peff
PATCH v3 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.3.571.g3f8bed7.dirty
Re: [PATCHv3 5/5] git-send-email: use git credential to obtain password
Quoted 4 lines · Michal Nazarewicz
git-send-email.perl +4
-0+ # TODO: Authentication may fail not because credentials were + # invalid but due to other reasons, in which we should not + # reject credentials. + $auth = Git::credential({
git-send-email.perl sub maildomain { · line 1076
Quoted 10 lines
git-send-email.perl +10
-0+ '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'}); + });
What do we want to do about this TODO?
I am happy to put it off until it becomes a problem, but I wonder if the Git::credential() interface is sufficient to express what we would want. It only allows two return values: true for approve, false for reject. But we would want a tri-state: approve, reject, indeterminate.
Reading the Net::SMTP code, it doesn't look like the information is even available to us (it really just passes out success or failure), so I don't think we can even make it work now. But it may be better to prepare the public Git::credential interface for it now, so we do not have to deal with breaking compatibility later.
-Peff
Re: [PATCHv3 5/5] git-send-email: use git credential to obtain password
Quoted 15 lines · Michal Nazarewicz
Quoted 14 lines
git-send-email.perl +14
-0+ # 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'}); + });
Quoted 6 lines · Jeff King
What do we want to do about this TODO?
I am happy to put it off until it becomes a problem, but I wonder if the
Git::credential()interface is sufficient to express what we would want. It only allows two return values: true for approve, false for reject. But we would want a tri-state: approve, reject, indeterminate.
Being it tri-state is not a problem. The last can be easily represented by undef.
Quoted 5 lines
Reading the
Net::SMTPcode, it doesn't look like the information is even available to us (it really just passes out success or failure), so I don't think we can even make it work now. But it may be better to prepare the publicGit::credentialinterface for it now, so we do not have to deal with breaking compatibility later.
I guess. I left it as is since git-send-email won't make use of the indeterminate values, but I can add it in this patchset as well.
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--
Re: [PATCHv3 5/5] git-send-email: use git credential to obtain password
Quoted 7 lines · Michal Nazarewicz
Quoted 4 lines
I am happy to put it off until it becomes a problem, but I wonder if the
Git::credential()interface is sufficient to express what we would want. It only allows two return values: true for approve, false for reject. But we would want a tri-state: approve, reject, indeterminate.Being it tri-state is not a problem. The last can be easily represented by undef.
Yeah, I think undef makes sense there.
Quoted 8 lines
Quoted 5 lines
Reading the
Net::SMTPcode, it doesn't look like the information is even available to us (it really just passes out success or failure), so I don't think we can even make it work now. But it may be better to prepare the publicGit::credentialinterface for it now, so we do not have to deal with breaking compatibility later.I guess. I left it as is since git-send-email won't make use of the indeterminate values, but I can add it in this patchset as well.
Yeah, I am more worried about third-party uses outside of the Git tree, which we may then break if we change the meaning of "undef" later. Thanks.
-Peff