Re: [PATCH 3/4] Git.pm: Add interface for git credential command.

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.

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

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

This is a nice touch. It makes the normal calling code a lot simpler.

-Peff