From: Jeff King <peff@peff.net>
Date: Mon, 04 Feb 2013 21:17:26 GMT
Subject: Re: [PATCH] Add contrib/credentials/netrc with GPG support
Message-ID: <20130204211726.GB13186@sigill.intra.peff.net>
In-Reply-To: <87ehgvua6h.fsf@lifelogs.com>

On Mon, Feb 04, 2013 at 02:54:30PM -0500, Ted Zlatanov wrote:

> +	print <<EOHIPPUS;
> +
> +$0 [-f AUTHFILE] [-d] get
> +
> +Version $VERSION by tzz\@lifelogs.com.  License: BSD.

This here-doc is interpolated so you can use $0 and $VERSION, and
therefore have to quote the @-sign. But later in the here-doc...

> +Thus, when we get "protocol=https\nusername=tzz", this credential
> +helper will look for lines in AUTHFILE that match

Do you need to quote "\n" here?

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

Hmm, so it's not an error (just a warning) to say:

  git credential-netrc -f /does/not/exist

but it is an error to say:

  git credential-netrc

and have it fail to find any netrc files. Shouldn't the latter be a
lesser error than the former?

> +while (<STDIN>) {
> +	next unless m/([^=]+)=(.+)/;
> +
> +	my ($token, $value) = ($1, $2);
> +	die "Unknown search token $1" unless exists $q{$token};
> +	$q{$token} = $value;
> +}

Should this regex be anchored at the start of the string? I think the
left-to-right matching means we will correctly match:

  key=value with=in it

so it may be OK.

> +if ($debug) {
> +	printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)'
> +	 foreach sort keys %q;
> +}

Leftover one-char indent.

> [...]

The rest looks OK to me.