Re: [PATCHv3 4/5] Git.pm: add interface for git credential command
On Mon, Feb 11, 2013 at 05:23:38PM +0100, Michal Nazarewicz wrote:
+=item credential_read( FILE_HANDLE )
+
+Reads credential key-value pairs from CFILE_HANDLE. Reading stops at EOF
or
+when an empty line is encountered. Each line must be of the form
Ckey=value
+with a non-empty key. Function returns a 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;
+}
Should this return a hash reference? It seems like that is how we end up
using and passing it elsewhere (since we have to anyway when passing it
as a parameter).
I don't have a strong preference, and it's somewhat a matter of taste.
And maybe returning the actual hash matches the rest of the module
better. I admit I don't really use Git.pm much.
-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
Re: [PATCHv3 4/5] Git.pm: add interface for git credential command
On Mon, Feb 11 2013, Jeff King wrote:
On Mon, Feb 11, 2013 at 05:23:38PM +0100, Michal Nazarewicz wrote:
+=item credential_read( FILE_HANDLE )
+
+Reads credential key-value pairs from CFILE_HANDLE. Reading stops at EOF
or
+when an empty line is encountered. Each line must be of the form
Ckey=value
+with a non-empty key. Function returns a 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;
+}
Should this return a hash reference? It seems like that is how we end up
using and passing it elsewhere (since we have to anyway when passing it
as a parameter).
Admittedly I mostly just copied what git-remote-mediawiki did here and
don't really have any preference either way, even though with this
function returning a reference the call site would have to become:
%$credential = %{ credential_read $reader };
Another alternative would be for it to take a reference as an argument,
possibly an optional one:
+sub credential_read {
+ my ($self, $reader, $ret) = (_maybe_self(@_), {});
+ my %credential;
+ while ($reader) {
+ # ...
+ }
+ %$ret = %credential;
+ $ret;
+}
I'd avoid modifying the hash while reading though since I think it's
best if it's left intact in case of an error.
And of course, if we want to get even more crazy, credential_write could
accept either reference or a hash, like so:
+sub credential_write {
+ my ($self, $writer, @rest) = _maybe_self(@_);
+ my $credential = @rest == 1 ? $rest[0] : { @rest };
+ my ($key, $value);
+ # ...
+}
Bottom line is, anything can be coded, but a question is whether it
makes sense to do so. ;)
--
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--
pgpzSk9PC_DIu.pgp
Description: PGP signature
Re: [PATCHv3 4/5] Git.pm: add interface for git credential command
On Mon, Feb 11, 2013 at 06:14:24PM +0100, Michal Nazarewicz wrote:
Should this return a hash reference? It seems like that is how we end up
using and passing it elsewhere (since we have to anyway when passing it
as a parameter).
Admittedly I mostly just copied what git-remote-mediawiki did here and
don't really have any preference either way, even though with this
function returning a reference the call site would have to become:
%$credential = %{ credential_read $reader };
Oh, right, because Git::credential takes the credential as an in-out
parameter rather than just returning it. Which is a bit unusual in perl,
but keeps the interface reasonably simple. The alternative would be:
$cred = Git::credential $cred, sub {
...
}
which is a little less nice.
Another alternative would be for it to take a reference as an argument,
possibly an optional one:
I think that is making things more ugly.
I'd avoid modifying the hash while reading though since I think it's
best if it's left intact in case of an error.
Agreed.
And of course, if we want to get even more crazy, credential_write could
accept either reference or a hash, like so:
+sub credential_write {
+ my ($self, $writer, @rest) = _maybe_self(@_);
+ my $credential = @rest == 1 ? $rest[0] : { @rest };
+ my ($key, $value);
+ # ...
+}
Ugh.
Bottom line is, anything can be coded, but a question is whether it
makes sense to do so. ;)
Yes, it is probably OK to leave it as-is, then. It is largely a matter
of taste, and I will defer to your judgement on that. :)
-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
