[PATCHv4 0/6] git-credential support in git-send-email

[PATCHv4 0/6] git-credential support in git-send-email

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

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(-)

-- 
1.8.1.3.572.g32bae1f

[PATCHv4 1/6] Git.pm: allow command_close_bidi_pipe to be called as method

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

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.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 perl/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 931047c..bbb753a 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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 ($!) {
-- 
1.8.1.3.572.g32bae1f

[PATCHv4 2/6] Git.pm: fix example in command_close_bidi_pipe documentation

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>


Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 perl/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index bbb753a..11f310a 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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);
 
-- 
1.8.1.3.572.g32bae1f

[PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

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.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 perl/Git.pm | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 11f310a..6bc9a3c 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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) {
-- 
1.8.1.3.572.g32bae1f

[PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

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.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 perl/Git.pm | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 6bc9a3c..d6e6c9e 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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);
-- 
1.8.1.3.572.g32bae1f

[PATCHv4 5/6] Git.pm: add interface for git credential command

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

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>.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 perl/Git.pm | 151 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 151 insertions(+)

diff --git a/perl/Git.pm b/perl/Git.pm
index d6e6c9e..a24458c 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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);
-- 
1.8.1.3.572.g32bae1f

[PATCHv4 6/6] git-send-email: use git credential to obtain password

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

If smtp_user is provided but smtp_pass is not, instead of
prompting for password, make git-send-email use git
credential command instead.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 Documentation/git-send-email.txt |  4 +--
 git-send-email.perl              | 59 +++++++++++++++++++++++-----------------
 2 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 44a1f7c..0cffef8 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -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.
diff --git a/git-send-email.perl b/git-send-email.perl
index be809e5..76bbfc3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -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;
-- 
1.8.1.3.572.g32bae1f

Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close

[Junio C Hamano]

Michal Nazarewicz <mpn@google.com> writes:

> From: Michal Nazarewicz <mina86@mina86.com>
>
> 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

s/file handlers/file handles/, I think.

Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close

[Jeff King]

On Tue, Feb 12, 2013 at 10:55:05AM -0800, Junio C Hamano wrote:

> Michal Nazarewicz <mpn@google.com> writes:
> 
> > From: Michal Nazarewicz <mina86@mina86.com>
> >
> > 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
> 
> s/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 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe

[Jeff King]

On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:

>  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);

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 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]

>> Michal Nazarewicz <mpn@google.com> writes:
>> > 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

> On Tue, Feb 12, 2013 at 10:55:05AM -0800, Junio C Hamano wrote:
>> s/file handlers/file handles/, I think.

On Tue, Feb 12 2013, Jeff King wrote:
> 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

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

On Tue, Feb 12 2013, Jeff King wrote:
> On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:
>
>>  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);
>
> 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.

I personally avoid parens whenever possible in Perl, but Git.pm seem to
favour them so I went with the second option.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:
>
>>  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);
>
> 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.

I would actually vote for the most explicit:

	_cmd_close($ctx, (grep { defined } ($in, $out)));

Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close

[Junio C Hamano]

Michal Nazarewicz <mina86@mina86.com> writes:

>>> Michal Nazarewicz <mpn@google.com> writes:
>>> > 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
>
>> On Tue, Feb 12, 2013 at 10:55:05AM -0800, Junio C Hamano wrote:
>>> s/file handlers/file handles/, I think.
>
> On Tue, Feb 12 2013, Jeff King wrote:
>> 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

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.

Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe

[Jeff King]

On Tue, Feb 12, 2013 at 01:14:57PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:
> >
> >>  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);
> >
> > 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.
> 
> 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

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

On Tue, Feb 12 2013, Junio C Hamano wrote:
> 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.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCHv4 5/6] Git.pm: add interface for git credential command

[Matthieu Moy]

Michal Nazarewicz <mpn@google.com> writes:

> +=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.

Typo: other then -> than.

> +sub credential_read {
> +	my ($self, $reader) = _maybe_self(@_);
> +	my %credential;
> +	while (<$reader>) {
> +		chomp;
> +		if ($_ eq '') {
> +			last;
> +		} elsif (!/^([^=]+)=(.*)$/) {

Good.

> +	# 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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password

[Matthieu Moy]

Michal Nazarewicz <mpn@google.com> writes:

> +	$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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Michal Nazarewicz <mpn@google.com> writes:
>
>> +	$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.

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(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 76bbfc3..c3501d9 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -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

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 3715 bytes --]

On Wed, Feb 27 2013, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Michal Nazarewicz <mpn@google.com> writes:
>>
>>> +	$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.
>
> 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. ;)

> Thanks for a review.
>
>
>  git-send-email.perl | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 76bbfc3..c3501d9 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -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) {

>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.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 76bbfc3..c3501d9 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -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) {

Seems obviously correct. I also did a basic test and it worked smoothly.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 76bbfc3..c3501d9 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -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) {
>
> Seems obviously correct. I also did a basic test and it worked smoothly.

OK, I'll squash it in.
Thanks.