On Tue, Feb 05, 2013 at 07:38:58PM -0500, Ted Zlatanov wrote:
> Add Git credential helper that can parse netrc/authinfo files.
>
> This credential helper supports multiple files, returning the first one
> that matches. It checks file permissions and owner. For *.gpg files,
> it will run GPG to decrypt the file.
>
> Signed-off-by: Ted Zlatanov <tzz@xxxxxxxxxxxx>
> + # the following check is copied from Net::Netrc, for non-GPG files
> + # OS/2 and Win32 do not handle stat in a way compatable with this check :-(
s/compatable/compatible/
You mention os/2 and Win32 here, but the check has more:
> + unless ($gpgmode || $options{insecure} ||
> + $^O eq 'os2'
> + || $^O eq 'MSWin32'
> + || $^O eq 'MacOS'
> + || $^O =~ /^cygwin/) {
Does MacOS really not handle stat? Or is this old MacOS, not OS X?
> +sub load_netrc {
> [...]
> + foreach my $nentry (@netrc_entries) {
> + my %entry;
> + my $num_port;
> +
> + if (!defined $nentry->{machine}) {
> + next;
> + }
> + if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
> + $num_port = $nentry->{port};
> + delete $nentry->{port};
> + }
> +
> + # create the new entry for the credential helper protocol
> + $entry{$options{tmap}->{$_}} = $nentry->{$_} foreach keys %$nentry;
> +
> + # for "host X port Y" where Y is an integer (captured by
> + # $num_port above), set the host to "X:Y"
> + if (defined $entry{host} && defined $num_port) {
> + $entry{host} = join(':', $entry{host}, $num_port);
> + }
So this will convert:
machine foo port smtp
in the netrc into (protocol => "smtp", host => "foo"), but:
machine foo port 25
into (protocol => undef, host => "foo:25"), right? That makes sense to
me.
> +sub net_netrc_loader {
> [...]
I won't comment here, as I know very little about netrc (I always
thought it was line-oriented, too!) and Junio has covered it.
> +# takes the search tokens and then a list of entries
> +# each entry is a hash reference
> +sub find_netrc_entry {
> + my $query = shift @_;
> +
> + ENTRY:
> + foreach my $entry (@_)
> + {
> + my $entry_text = join ', ', map { "$_=$entry->{$_}" } keys %$entry;
> + foreach my $check (sort keys %$query) {
> + if (defined $query->{$check}) {
> + log_debug("compare %s [%s] to [%s] (entry: %s)",
> + $check,
> + $entry->{$check},
> + $query->{$check},
> + $entry_text);
> + unless ($query->{$check} eq $entry->{$check}) {
> + next ENTRY;
> + }
> + } else {
> + log_debug("OK: any value satisfies check $check");
> + }
This looks right to me.
> +sub print_credential_data {
I don't know if you want to take the hit of relying on Git.pm (it is
nice for the helper to be totally standalone and copy-able), but one
obvious possible refactor would be to use the credential read/write
functions recently added there. I'm OK with not doing that, though.
> + my $entry = shift @_;
> + my $query = shift @_;
> +
> + log_debug("entry has passed all the search checks");
> + TOKEN:
> + foreach my $git_token (sort keys %$entry) {
> + log_debug("looking for useful token $git_token");
> + # don't print unknown (to the credential helper protocol) tokens
> + next TOKEN unless exists $query->{$git_token};
> +
> + # don't print things asked in the query (the entry matches them)
> + next TOKEN if defined $query->{$git_token};
> +
> + log_debug("FOUND: $git_token=$entry->{$git_token}");
> + printf "%s=%s\n", $git_token, $entry->{$git_token};
> + }
Printf? Bleh, isn't this supposed to be perl? :P
I don't see anything wrong from the credential-handling side of things.
As I said, I didn't look closely at the netrc parsing bits. From my
reading of "perldoc macos", the answer to my question above is "yes,
stat doesn't work on MacOS Classic". So I think the script itself is
fine.
In your tests:
> +++ b/contrib/credential/netrc/Makefile
> @@ -0,0 +1,12 @@
> +test_netrc:
> + @(echo "bad data" | ./git-credential-netrc -f A -d -v) || echo "Bad invocation test, ignoring
> failure"
> + @echo "=> Silent invocation... nothing should show up here with a missing file"
> + @echo "bad data" | ./git-credential-netrc -f A get
> + @echo "=> Back to noisy: -v and -d used below, missing file"
> + echo "bad data" | ./git-credential-netrc -f A -d -v get
> + @echo "=> Look for any entry in the default file set"
> + echo "" | ./git-credential-netrc -d -v get
> + @echo "=> Look for github.com in the default file set"
> + echo "host=google.com" | ./git-credential-netrc -d -v get
> + @echo "=> Look for a nonexistent machine in the default file set"
> + echo "host=korovamilkbar" | ./git-credential-netrc -d -v get
You are depending on whatever the user has in their ~/.netrc, no?
Wouldn't it make more sense to ship a sample netrc and run all of the
tests with "-f netrc.example"?
It may also be worth building on top of the regular git test harness.
It's more work, but the resulting code (and the output) will be much
more readable.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html