git-credential support in git-send-email
v4 cover git-credential support in git-send-email
From: Michal Nazarewicz <[email protected]>
Besids git-credential support in git-send-email, there are some other minor improvements to Git.pm in this patchset. Patch 3/6 is new compared to the previous patchset.
Michal Nazarewicz (6):
Git.pm: allow command_close_bidi_pipe to be called as method Git.pm: fix example in command_close_bidi_pipe documentation Git.pm: refactor command_close_bidi_pipe to use _cmd_close 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 | 198 ++++++++++++++++++++++++++++++++++----- 3 files changed, 213 insertions(+), 48 deletions(-)
Signature · 1 line
1.8.1.3.572.g32bae1f
PATCH v4 1/6 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.3.572.g32bae1f
PATCH v4 2/6 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.572.g32bae1f
PATCH v4 3/6 Git.pm: refactor command_close_bidi_pipe to use _cmd_close
In reply to Michal NazarewiczFrom: Michal Nazarewicz <[email protected]>
The body of the loop in command_close_bidi_pipe function is identical to what _cmd_close function does so instead of duplicating, refactor change _cmd_close so that it accepts list of file handlers to be closed, which makes it usable with command_close_bidi_pipe.
--- perl/Git.pm | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-)
perl/Git.pm +11 -19
@@ -267,13 +267,13 @@ sub command { if (not defined wantarray) { # Nothing to pepper the possible exception with. - _cmd_close($fh, $ctx); + _cmd_close($ctx, $fh); } elsif (not wantarray) { local $/; my $text = <$fh>; try { - _cmd_close($fh, $ctx); + _cmd_close($ctx, $fh); } catch Git::Error::Command with { # Pepper with the output: my $E = shift;
@@ -286,7 +286,7 @@ sub command { my @lines = <$fh>; defined and chomp for @lines; try { - _cmd_close($fh, $ctx); + _cmd_close($ctx, $fh); } catch Git::Error::Command with { my $E = shift; $E->{'-outputref'} = \@lines;
@@ -313,7 +313,7 @@ sub command_oneline { my $line = <$fh>; defined $line and chomp $line; try { - _cmd_close($fh, $ctx); + _cmd_close($ctx, $fh); } catch Git::Error::Command with { # Pepper with the output: my $E = shift;
@@ -381,7 +381,7 @@ have more complicated structure. sub command_close_pipe { my ($self, $fh, $ctx) = _maybe_self(@_); $ctx ||= '<unknown>'; - _cmd_close($fh, $ctx); + _cmd_close($ctx, $fh); } =item command_bidi_pipe ( COMMAND [, ARGUMENTS... ] )
@@ -431,18 +431,8 @@ have more complicated structure. sub command_close_bidi_pipe { local $?; my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_); - foreach my $fh ($in, $out) { - unless (close $fh) { - if ($!) { - carp "error closing pipe: $!"; - } elsif ($? >> 8) { - throw Git::Error::Command($ctx, $? >>8); - } - } - } - + _cmd_close($ctx, $in, $out); waitpid $pid, 0; - if ($? >> 8) { throw Git::Error::Command($ctx, $? >>8); }
@@ -1355,9 +1345,11 @@ sub _execv_git_cmd { exec('git', @_); } # Close pipe to a subprocess. sub _cmd_close { - my ($fh, $ctx) = @_; - if (not close $fh) { - if ($!) { + my $ctx = shift @_; + foreach my $fh (@_) { + if (close $fh) { + # nop; + } elsif ($!) { # It's just close, no point in fatalities carp "error closing pipe: $!"; } elsif ($? >> 8) {
Signature · 1 line
1.8.1.3.572.g32bae1f
Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close
Quoted 5 lines · Michal Nazarewicz
From: Michal Nazarewicz <[email protected]>
The body of the loop in
command_close_bidi_pipefunction is identical to what_cmd_closefunction does so instead of duplicating, refactor change_cmd_closeso that it accepts list of file handlers to be closed, which
s/file handlers/file handles/, I think.
Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close
Quoted 9 lines · Junio C Hamano
Quoted 5 lines · Michal Nazarewicz
From: Michal Nazarewicz <[email protected]>
The body of the loop in
command_close_bidi_pipefunction is identical to what_cmd_closefunction does so instead of duplicating, refactor change_cmd_closeso that it accepts list of file handlers to be closed, whichs/file handlers/file handles/, I think.
And s/refactor change/refactor/.
Other than that, I think the series looks OK. I have one style micro-nit on patch 4 which I'll reply in-line. But it is either "fix while applying" or "ignore", I don't think it will be worth a re-roll.
-Peff
Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines · Michal Nazarewicz
The body of the loop in
command_close_bidi_pipefunction is identical to what_cmd_closefunction does so instead of duplicating, refactor change_cmd_closeso that it accepts list of file handlers to be closed, whichQuoted 1 line · Junio C Hamano
s/file handlers/file handles/, I think.
Quoted 5 lines · Jeff King
And s/refactor change/refactor/.
Other than that, I think the series looks OK. I have one style micro-nit on patch 4 which I'll reply in-line. But it is either "fix while applying" or "ignore", I don't think it will be worth a re-roll.
All fixed.
Junio, do you want me to resend or would you be fine with just pulling:
git://github.com/mina86/git.git 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--
Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close
Quoted 15 lines · Michal Nazarewicz
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines · Michal Nazarewicz
The body of the loop in
command_close_bidi_pipefunction is identical to what_cmd_closefunction does so instead of duplicating, refactor change_cmd_closeso that it accepts list of file handlers to be closed, whichQuoted 1 line · Junio C Hamano
s/file handlers/file handles/, I think.
Quoted 5 lines · Jeff King
And s/refactor change/refactor/.
Other than that, I think the series looks OK. I have one style micro-nit on patch 4 which I'll reply in-line. But it is either "fix while applying" or "ignore", I don't think it will be worth a re-roll.
Quoted 5 lines
All fixed.
Junio, do you want me to resend or would you be fine with just pulling:
git://github.com/mina86/git.gitmaster
Neither. I agree with Peff that these micronits are not enough reason for the trouble of rerolling the series, so I'll just amend them at my end. Please double-check what you see on the 'pu' branch when I push today's integration result out later.
Thanks.
PATCH v4 4/6 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,12 +426,25 @@ 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(@_); - _cmd_close($ctx, $in, $out); + _cmd_close($ctx, grep defined, $in, $out); waitpid $pid, 0; if ($? >> 8) { throw Git::Error::Command($ctx, $? >>8);
Signature · 1 line
1.8.1.3.572.g32bae1f
Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
perl/Git.pm Note that you should not rely on whatever actually is in C<CTX>; · line 447
Quoted 5 lines · Michal Nazarewicz
sub
command_close_bidi_pipe{perl/Git.pm +1
-1local $?; my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_); - _cmd_close($ctx, $in, $out); + _cmd_close($ctx, grep defined, $in, $out);
Maybe it is just me, but I find the "grep EXPR" form a little subtle inside an argument list. Either:
_cmd_close($ctx, grep { defined } $in, $out);or
_cmd_close($ctx, grep(defined, $in, $out));
is a little more obvious to me.
-Peff
Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
Quoted 11 lines · Jeff King
perl/Git.pm Note that you should not rely on whatever actually is in C<CTX>; · line 447
Quoted 5 lines · Michal Nazarewicz
sub
command_close_bidi_pipe{perl/Git.pm +1
-1local $?; my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_); - _cmd_close($ctx, $in, $out); + _cmd_close($ctx, grep defined, $in, $out);Maybe it is just me, but I find the "grep EXPR" form a little subtle inside an argument list. Either:
Quoted 7 lines
_cmd_close($ctx, grep { defined } $in, $out);or
_cmd_close($ctx, grep(defined, $in, $out));is a little more obvious to me.
I would actually vote for the most explicit:
_cmd_close($ctx, (grep { defined } ($in, $out)));Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
Quoted 17 lines · Junio C Hamano
Quoted 15 lines · Jeff King
perl/Git.pm Note that you should not rely on whatever actually is in C<CTX>; · line 447
Quoted 5 lines · Michal Nazarewicz
sub
command_close_bidi_pipe{perl/Git.pm +1
-1local $?; my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_); - _cmd_close($ctx, $in, $out); + _cmd_close($ctx, grep defined, $in, $out);Maybe it is just me, but I find the "grep EXPR" form a little subtle inside an argument list. Either:
_cmd_close($ctx, grep { defined } $in, $out);or
Quoted 7 lines
Quoted 3 lines
_cmd_close($ctx, grep(defined, $in, $out));is a little more obvious to me.
I would actually vote for the most explicit:
_cmd_close($ctx, (grep { defined } ($in, $out)));
Gross. My perl spider-sense tingles at seeing that many optional punctuation characters, but it should at least be obvious to a casual or new perl programmer what is going on. I'm fine with it.
-Peff
Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
In reply to Junio C HamanoQuoted 3 lines · Junio C Hamano
I would actually vote for the most explicit:
_cmd_close($ctx, (grep { defined } ($in, $out)));
To me that looks weird at best, but I don't have strong opinions on that matter.
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: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
In reply to Jeff KingQuoted 11 lines · Jeff King
perl/Git.pm Note that you should not rely on whatever actually is in C<CTX>; · line 447
Quoted 5 lines · Michal Nazarewicz
sub
command_close_bidi_pipe{perl/Git.pm +1
-1local $?; my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_); - _cmd_close($ctx, $in, $out); + _cmd_close($ctx, grep defined, $in, $out);Maybe it is just me, but I find the "grep EXPR" form a little subtle inside an argument list. Either:
Quoted 7 lines
_cmd_close($ctx, grep { defined } $in, $out);or
_cmd_close($ctx, grep(defined, $in, $out));is a little more obvious to me.
I personally avoid parens whenever possible in Perl, but Git.pm seem to favour them so I went with the second option.
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 v4 5/6 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 | 151 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 151 insertions(+)
perl/Git.pm +151 -0
@@ -59,6 +59,7 @@ require Exporter; command_bidi_pipe command_close_bidi_pipe version exec_path html_path hash_object git_cmd_try remote_refs prompt + credential credential_read credential_write temp_acquire temp_release temp_reset temp_path);
@@ -1003,6 +1004,156 @@ sub _close_cat_blob { } +=item credential_read( FILEHANDLE ) + +Reads credential key-value pairs from C<FILEHANDLE>. 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 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_write( FILEHANDLE, CREDENTIAL_HASHREF ) + +Writes credential key-value pairs from hash referenced by +C<CREDENTIAL_HASHREF> to C<FILEHANDLE>. Keys and values cannot contain +new-lines or NUL bytes characters, and key cannot contain equal signs 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; + } + if (<$reader>) { + throw Error::Simple("unexpected output from git credential $op response:\n$_\n"); + } + + command_close_bidi_pipe($pid, $reader, undef, $ctx); +} + +=item credential( CREDENTIAL_HASHREF [, OPERATION ] ) + +=item credential( CREDENTIAL_HASHREF, CODE ) + +Executes C<git credential> for a given set of credentials and specified +operation. In both forms C<CREDENTIAL_HASHREF> 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'>, C<'approve'> or C<'reject'>, +and function will execute corresponding C<git credential> sub-command. If +it's omitted C<'fill'> is assumed. In case of C<'fill'> the values stored in +C<CREDENTIAL_HASHREF> will be changed to the ones returned by the C<git +credential fill> 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 the provided credential +hash, then call C<CODE> with C<CREDENTIAL_HASHREF> 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> returns. 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); + if (defined $ret) { + _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.572.g32bae1f
Re: [PATCHv4 5/6] Git.pm: add interface for git credential command
perl/Git.pm sub _close_cat_blob { · line 1012
Quoted 6 lines · Michal Nazarewicz
+=item
credential_read( FILEHANDLE )perl/Git.pm +5
-0+ +Reads credential key-value pairs from C<FILEHANDLE>. 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 hash with all read values. Any white +space (other then new-line character) is preserved.
Typo: other then -> than.
perl/Git.pm sub _close_cat_blob { · line 1023
Quoted 8 lines
perl/Git.pm +8
-0+sub credential_read { + my ($self, $reader) = _maybe_self(@_); + my %credential; + while (<$reader>) { + chomp; + if ($_ eq '') { + last; + } elsif (!/^([^=]+)=(.*)$/) {
Good.
perl/Git.pm sub _close_cat_blob { · line 1058
Quoted 10 lines
perl/Git.pm +10
-0+ # 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"); + } + }
Good.
These checks seem to address all the points raised during discussion about when the API should reject values.
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
PATCH v4 6/6 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.572.g32bae1f
Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password
git-send-email.perl sub maildomain { · line 1068
Quoted 3 lines · Michal Nazarewicz
git-send-email.perl +3
-0+ $auth = Git::credential({ + 'protocol' => 'smtp', + 'host' => join(':', $smtp_server, $smtp_server_port),
At this point, $smtp_server_port is not always defined. I just tested and got
Use of uninitialized value $smtp_server_port in join or string at git-send-email line 1077.
Other than that, the whole series looks good.
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password
Quoted 9 lines · Matthieu Moy
Review
Quoted 3 lines · Michal Nazarewicz
(quoted diff) +3
-0+ $auth = Git::credential({ + 'protocol' => 'smtp', + 'host' => join(':', $smtp_server, $smtp_server_port),At this point, $smtp_server_port is not always defined. I just tested and got
Quoted 4 lines
Use of uninitialized value $smtp_server_port in join or string at git-send-email line 1077.
Other than that, the whole series looks good.
Given that there is another place that conditionally append ":$port" to the host string, I think we should follow suit here. Perhaps like the attached diff?
Thanks for a review.
git-send-email.perl | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
git-send-email.perl +10 -4
@@ -1045,6 +1045,14 @@ sub maildomain { return maildomain_net() || maildomain_mta() || 'localhost.localdomain'; } +sub smtp_host_string { + if (defined $smtp_server_port) { + return "$smtp_server:$smtp_server_port"; + } else { + return $smtp_server; + } +} + # Returns 1 if authentication succeeded or was not necessary # (smtp_user was not specified), and 0 otherwise.
@@ -1065,7 +1073,7 @@ sub smtp_auth_maybe { # reject credentials. $auth = Git::credential({ 'protocol' => 'smtp', - 'host' => join(':', $smtp_server, $smtp_server_port), + 'host' => smtp_host_string(), 'username' => $smtp_authuser, # if there's no password, "git credential fill" will # give us one, otherwise it'll just pass this one.
@@ -1188,9 +1196,7 @@ sub send_message { else { require Net::SMTP; $smtp_domain ||= maildomain(); - $smtp ||= Net::SMTP->new((defined $smtp_server_port) - ? "$smtp_server:$smtp_server_port" - : $smtp_server, + $smtp ||= Net::SMTP->new(smtp_host_string(), Hello => $smtp_domain, Debug => $debug_net_smtp); if ($smtp_encryption eq 'tls' && $smtp) {
Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password
Quoted 29 lines · Junio C Hamano
git-send-email.perl +9
-1@@ -1045,6 +1045,14 @@ sub maildomain { return maildomain_net() || maildomain_mta() || 'localhost.localdomain'; } +sub smtp_host_string { + if (defined $smtp_server_port) { + return "$smtp_server:$smtp_server_port"; + } else { + return $smtp_server; + } +} + # Returns 1 if authentication succeeded or was not necessary # (smtp_user was not specified), and 0 otherwise.@@ -1065,7 +1073,7 @@ sub smtp_auth_maybe { # reject credentials. $auth = Git::credential({ 'protocol' => 'smtp', - 'host' => join(':', $smtp_server, $smtp_server_port), + 'host' => smtp_host_string(), 'username' => $smtp_authuser, # if there's no password, "git credential fill" will # give us one, otherwise it'll just pass this one.
git-send-email.perl sub send_message { · line 1202
Quoted 10 lines
git-send-email.perl +1
-3@@ -1188,9 +1196,7 @@ sub send_message { else { require Net::SMTP; $smtp_domain ||= maildomain(); - $smtp ||= Net::SMTP->new((defined $smtp_server_port) - ? "$smtp_server:$smtp_server_port" - : $smtp_server, + $smtp ||= Net::SMTP->new(smtp_host_string(), Hello => $smtp_domain, Debug => $debug_net_smtp); if ($smtp_encryption eq 'tls' && $smtp) {
Seems obviously correct. I also did a basic test and it worked smoothly.
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password
Quoted 35 lines · Matthieu Moy
Quoted 33 lines · Junio C Hamano
git-send-email.perl +9
-2@@ -1045,6 +1045,14 @@ sub maildomain { return maildomain_net() || maildomain_mta() || 'localhost.localdomain'; } +sub smtp_host_string { + if (defined $smtp_server_port) { + return "$smtp_server:$smtp_server_port"; + } else { + return $smtp_server; + } +} + # Returns 1 if authentication succeeded or was not necessary # (smtp_user was not specified), and 0 otherwise.@@ -1065,7 +1073,7 @@ sub smtp_auth_maybe { # reject credentials. $auth = Git::credential({ 'protocol' => 'smtp', - 'host' => join(':', $smtp_server, $smtp_server_port), + 'host' => smtp_host_string(), 'username' => $smtp_authuser, # if there's no password, "git credential fill" will # give us one, otherwise it'll just pass this one.@@ -1188,9 +1196,7 @@ sub send_message { else { require Net::SMTP; $smtp_domain ||= maildomain(); - $smtp ||= Net::SMTP->new((defined $smtp_server_port)
Quoted 8 lines
git-send-email.perl sub send_message { · line 1202
Quoted 6 lines
git-send-email.perl +1
-2- ? "$smtp_server:$smtp_server_port" - : $smtp_server, + $smtp ||= Net::SMTP->new(smtp_host_string(), Hello => $smtp_domain, Debug => $debug_net_smtp); if ($smtp_encryption eq 'tls' && $smtp) {Seems obviously correct. I also did a basic test and it worked smoothly.
OK, I'll squash it in.
Thanks.
Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password
In reply to Junio C HamanoQuoted 11 lines · Junio C Hamano
Quoted 9 lines · Matthieu Moy
Review
Quoted 3 lines · Michal Nazarewicz
(quoted diff) +3
-0+ $auth = Git::credential({ + 'protocol' => 'smtp', + 'host' => join(':', $smtp_server, $smtp_server_port),At this point, $smtp_server_port is not always defined. I just tested and got
Quoted 8 lines
Quoted 4 lines
Use of uninitialized value $smtp_server_port in join or string at git-send-email line 1077.
Other than that, the whole series looks good.
Given that there is another place that conditionally append ":$port" to the host string, I think we should follow suit here. Perhaps like the attached diff?
Damn meetings, you beat me to it… I was just about to send a patch. ;)
Quoted 46 lines
Thanks for a review.
git-send-email.perl | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)git-send-email.perl +10
-4@@ -1045,6 +1045,14 @@ sub maildomain { return maildomain_net() || maildomain_mta() || 'localhost.localdomain'; } +sub smtp_host_string { + if (defined $smtp_server_port) { + return "$smtp_server:$smtp_server_port"; + } else { + return $smtp_server; + } +} + # Returns 1 if authentication succeeded or was not necessary # (smtp_user was not specified), and 0 otherwise.@@ -1065,7 +1073,7 @@ sub smtp_auth_maybe { # reject credentials. $auth = Git::credential({ 'protocol' => 'smtp', - 'host' => join(':', $smtp_server, $smtp_server_port), + 'host' => smtp_host_string(), 'username' => $smtp_authuser, # if there's no password, "git credential fill" will # give us one, otherwise it'll just pass this one.@@ -1188,9 +1196,7 @@ sub send_message { else { require Net::SMTP; $smtp_domain ||= maildomain(); - $smtp ||= Net::SMTP->new((defined $smtp_server_port) - ? "$smtp_server:$smtp_server_port" - : $smtp_server, + $smtp ||= Net::SMTP->new(smtp_host_string(), Hello => $smtp_domain, Debug => $debug_net_smtp); if ($smtp_encryption eq 'tls' && $smtp) {
Quoted 1 line
From reading of SMTP.pm, it seems that this could be changed to:
- $smtp ||= Net::SMTP->new((defined $smtp_server_port)
- ? "$smtp_server:$smtp_server_port"
- : $smtp_server,
+ $smtp ||= Net::SMTP->new($smtp_server,
+ Port => $smtp_server_port,
and than the other part would become:
@@ -1060,12 +1060,17 @@ sub smtp_auth_maybe {
Authen::SASL->import(qw(Perl));
};+ my $host = $smtp_server;
+ if (defined $smtp_server_port) {
+ $host .= ':' . $smtp_server_port;
+ }
+
# 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),
+ 'host' => $host,
'username' => $smtp_authuser,
# if there's no password, "git credential fill" will # give us one, otherwise it'll just pass this one.
Either way, looks good to me.
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--