Re: [PATCH 3/4] Git.pm: Add interface for git credential command.
- From
Jeff King <peff@peff.net>
- Date
- Feb 6, 2013, 23:16 UTC
- Message-ID
- <20130206231625.GL27507@sigill.intra.peff.net>
- In-Reply-To
- <f4b5413b0a55474346daa7b0866c7a4fed55778d.1360183427.git.mina86@mina86.com>
On Wed, Feb 06, 2013 at 09:47:05PM +0100, Michal Nazarewicz wrote:
Show 6 quoted lines
> +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.
Show 13 quoted lines
> +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?
Show 7 quoted lines
> +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