Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2013, 22:56 UTC
Message-ID: <7vd2wf1yex.fsf@alter.siamese.dyndns.org>
In-Reply-To: <87mwvjsqjc.fsf_-_@lifelogs.com>

Ted Zlatanov <tzz@lifelogs.com> writes:

> Signed-off-by: Ted Zlatanov <tzz@lifelogs.com>

The space above your S-o-b: could be utilized a bit better ;-)

Show 36 quoted lines

> @@ -0,0 +1,236 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
> +
> +use Getopt::Long;
> +use File::Basename;
> +
> +my $VERSION = "0.1";
> +
> +my %options = (
> +               help => 0,
> +               debug => 0,
> +
> +               # identical token maps, e.g. host -> host, will be inserted later
> +               tmap => {
> +                        port => 'protocol',
> +                        machine => 'host',
> +                        path => 'path',
> +                        login => 'username',
> +                        user => 'username',
> +                        password => 'password',
> +                       }
> +              );
> +
> +# map each credential protocol token to itself on the netrc side
> +$options{tmap}->{$_} = $_ foreach values %{$options{tmap}};
> +
> +foreach my $suffix ('.gpg', '') {
> +	foreach my $base (qw/authinfo netrc/) {
> +		my $file = glob("~/.$base$suffix");
> +		next unless (defined $file && -f $file);
> +		$options{file} = $file ;
> +	}
> +}

This checks .gpg first and then unencrypted, and checks authinfo first and netrc second, both of which makes sense. It is good to encourage use of encrypted files, and it is good to use newer authinfo files over netrc files.

However, it is strange that you let the ones that are discovered later in the loop to override the ones that are discovered earlier. Perhaps you meant

	next unless (... exists there ...);
        $options{"file"} = $file;
        last;

instead?

> +Getopt::Long::Configure("bundling");

Hmm, OK.

Show 61 quoted lines

> +# TODO: maybe allow the token map $options{tmap} to be configurable.
> +GetOptions(\%options,
> +           "help|h",
> +           "debug|d",
> +           "file|f=s",
> +          );
> +
> +if ($options{help}) {
> +	my $shortname = basename($0);
> +	$shortname =~ s/git-credential-//;
> +
> +	print <<EOHIPPUS;
> +
> +$0 [-f AUTHFILE] [-d] get
> +
> +Version $VERSION by tzz\@lifelogs.com.  License: BSD.
> +
> +Options:
> +  -f AUTHFILE: specify a netrc-style file
> +  -d: turn on debugging
> +
> +To enable (note that Git will prepend "git-credential-" to the helper
> +name and look for it in the path):
> +
> +  git config credential.helper '$shortname -f AUTHFILE'
> +
> +And if you want lots of debugging info:
> +
> +  git config credential.helper '$shortname -f AUTHFILE -d'
> +
> +Only "get" mode is supported by this credential helper.  It opens
> +AUTHFILE and looks for entries that match the requested search
> +criteria:
> +
> + 'port|protocol':
> +   The protocol that will be used (e.g., https). (protocol=X)
> +
> + 'machine|host':
> +   The remote hostname for a network credential. (host=X)
> +
> + 'path':
> +   The path with which the credential will be used. (path=X)
> +
> + 'login|user|username':
> +   The credential’s username, if we already have one. (username=X)
> +
> +Thus, when we get this query on STDIN:
> +
> +protocol=https
> +username=tzz
> +
> +this credential helper will look for lines in AUTHFILE that match
> +
> +port https login tzz
> +
> +OR
> +
> +protocol https login tzz
> +
> +OR... etc. acceptable tokens as listed above.  Any unknown tokens are
> +simply ignored.

I recall that netrc/authinfo files are _not_ line oriented. Earlier you said "looks for entries that match" which is a lot more correct, but then we see "look for lines in authfile".

> +Then, the helper will print out whatever tokens it got from the line,
> +including "password" tokens, mapping e.g. "port" back to "protocol".

Again "line" is mentioned twice, above and below.

Show 9 quoted lines

> +The first matching line is used.  Tokens can be quoted as 'STRING' or
> +"STRING".
> +
> +No caching is performed by this credential helper.
> +
> +EOHIPPUS
> +
> +	exit;
> +}

Show 8 quoted lines

> +my $mode = shift @ARGV;
> +
> +# credentials may get 'get', 'store', or 'erase' as parameters but
> +# only acknowledge 'get'
> +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
> +
> +# only support 'get' mode
> +exit unless $mode eq 'get';

The above looks strange. Why does the invoker get the error message only when it runs this without arguments? Did you mean to say more like this?

	unless (defined $mode && $mode eq 'get') {
		die "...";
	}

By the way, I think statement modifiers tend to get overused and make the resulting program harder to read. die "..." at the beginning of line makes the reader go "Whoa, it already is done and existing on error", and then forces the eyes to scan the error message to find "unless" and the condition.

It may be a cute syntax and some may find it even cool, but cuteness or coolness is less valuable compared with the readability.

Show 12 quoted lines

> +my $debug = $options{debug};
> +my $file = $options{file};
> +
> +unless (defined $file) {
> +	print STDERR "Please specify an existing netrc file (with or without a .gpg extension) with -f AUTHFILE\n" if $debug;
> +	exit 0;
> +}
> +
> +unless (-f $file) {
> +	print STDERR "Sorry, the specified netrc $file is not accessible\n" if $debug;
> +	exit 0;
> +}

Perhaps "-r $file", if you say "is not accessible"?

Is it sensible to squelch the error message by default and force user to specify --debug? You could argue that the option is to debug the user's configuration, but the name of the option sounds more like it is for debugging this script itself.

I saw Peff already pointed out error conditions, but I am not sure why all of these exit with 0. If the user has configured

	git config credential.helper 'netrc -f $HOME/.netcr'

shouldn't it be diagnosed as an error? It is understandable to let this go silently

	git config credential.helper 'netrc'

and let other credential helpers take over when no $HOME/.{netrc,authinfo}{,.gpg} file exist, but in that case the user may still want to remove the config item that is not doing anything useful and erroring out with a message may be a way to help the user know about the situation.

Show 11 quoted lines

> +my @data;
> +if ($file =~ m/\.gpg$/) {
> +	@data = load('-|', qw(gpg --decrypt), $file)
> +}
> +else {
> +	@data = load('<', $file);
> +}
> +
> +chomp @data;
> +
> +unless (scalar @data) {

Shouldn't this error check come logically before chomping?

> +	print STDERR "Sorry, we could not load data from [$file]\n" if $debug;
> +	exit;
> +}

Is this really an error? The file perhaps was empty. Shouldn't that case treated the same way as the case where no entry that matches the criteria invoker gave you was found?