[PATCHv2 4/5] Git.pm: add interface for git credential command
From: Michal Nazarewicz min...@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 min...@mina86.com
---
perl/Git.pm | 110 +++-
1 file changed, 109 insertions(+), 1 deletion(-)
On Thu, Feb 07 2013, Jeff King p...@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@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 Cgit credential for a given set of credentials and
+specified operation. In both form CCREDENTIAL_HASH needs to be
+a reference to a hash which stores credentials. Under certain
+conditions the hash can change.
+
+In the first form, COPERATION can be C'fill' (or omitted),
+C'approve' or C'reject', and function will execute corresponding
+Cgit credential
Re: [PATCHv2 4/5] Git.pm: add interface for git credential command
Michal Nazarewicz m...@google.com writes: From: Michal Nazarewicz min...@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@imag.fr -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] Git.pm: add interface for git credential command
Matthieu Moy matthieu@grenoble-inp.fr writes: Michal Nazarewicz m...@google.com writes: From: Michal Nazarewicz min...@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@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] Git.pm: add interface for git credential command
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: m...@google.com--ooO--(_)--Ooo-- pgpkH3LocTxhw.pgp Description: PGP signature
Re: [PATCHv2 4/5] Git.pm: add interface for git credential command
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@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
