[PATCHv2 0/5] Make git-send-email use git-credential

[PATCHv2 0/5] Make git-send-email use git-credential

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

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

-- 
1.8.1.2.549.g1d13f9f

[PATCHv2 1/5] 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.2.549.g1d13f9f

[PATCHv2 2/5] Git.pm: fix example in command_close_bidi_pipe documentation

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

File handle goes as the first argument when calling print on it.

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

[PATCHv2 3/5] 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(-)

 > On Wed, Feb 06, 2013 at 09:47:04PM +0100, Michal Nazarewicz wrote:
 >> 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);

 On Thu, Feb 07 2013, Jeff King <peff@peff.net> wrote:
 > Should this part go into the documentation for command_close_bidi_pipe
 > in Git.pm?

 Done.

diff --git a/perl/Git.pm b/perl/Git.pm
index 11f310a..9dded54 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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) {
-- 
1.8.1.2.549.g1d13f9f

[PATCHv2 4/5] 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 | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

 On Thu, Feb 07 2013, Jeff King <peff@peff.net> wrote:
 > On Wed, Feb 06, 2013 at 09:47:05PM +0100, Michal Nazarewicz wrote:
 >
 >> +sub _credential_read {
 >> +	my %credential;
 >> +	my ($reader, $op) = (@_);
 >> +	while (<$reader>) {
 >> +		chomp;
 >> +		my ($key, $value) = /([^=]*)=(.*)/;
 >
 > Empty keys are not valid. Can we make this:
 >
 >   /^([^=]+)=(.*)/
 >
 > to fail the regex? Otherwise, I think this check:
 >
 >> +		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.  

 >> +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";
 >> +		}
 >> +	}
 >
 > 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),

 On Thu, Feb 07 2013, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
 > 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.

diff --git a/perl/Git.pm b/perl/Git.pm
index 9dded54..b4adead 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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);
-- 
1.8.1.2.549.g1d13f9f

[PATCHv2 5/5] 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.2.549.g1d13f9f

Re: [PATCHv2 5/5] git-send-email: use git credential to obtain password

[Junio C Hamano]

Michal Nazarewicz <mpn@google.com> writes:

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

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.

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

Re: [PATCHv2 4/5] Git.pm: add interface for git credential command

[Matthieu Moy]

Michal Nazarewicz <mpn@google.com> writes:

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

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 <Matthieu.Moy@imag.fr>

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

Re: [PATCHv2 4/5] Git.pm: add interface for git credential command

[Junio C Hamano]

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

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

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

[Michal Nazarewicz]

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

On Fri, Feb 08 2013, Junio C Hamano wrote:
> 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. ;)

-- 
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: [PATCHv2 4/5] Git.pm: add interface for git credential command

[Jeff King]

On Thu, Feb 07, 2013 at 03:01:20PM +0100, Michal Nazarewicz wrote:

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

>  - when reading, what to do if the line is "foo=" (ie. empty value),

The empty string is a valid value.

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

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

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

>  On Thu, Feb 07 2013, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>  > 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.

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

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