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

From: Ted Zlatanov <tzz@lifelogs.com>
Date: Feb 5, 2013, 17:01 UTC
Message-ID: <87k3qmr8yc.fsf@lifelogs.com>
In-Reply-To: <7vvca6u47f.fsf@alter.siamese.dyndns.org>

On Tue, 05 Feb 2013 08:15:48 -0800 Junio C Hamano <gitster@pobox.com> wrote:

JCH> Ted Zlatanov <tzz@lifelogs.com> writes:

Show 5 quoted lines

>> +# build reverse token map
>> +my %rmap;
>> +foreach my $k (keys %{$options{tmap}}) {
>> +	push @{$rmap{$options{tmap}->{$k}}}, $k;
>> +}

JCH> Mental note: "$rmap{foo} -eq 'bar'" means that what Git calls 'bar' JCH> is found as 'foo' in the netrc/authinfo file. Keys in %rmap are JCH> what we expect to read from the netrc/authinfo file.

I'll document that better in PATCHv4.

JCH> So you grabbed one line of input, split them into token pairs, and JCH> built %tokens = ('key Git may want to see' => 'value read from file') JCH> mapping.

This will be fixed with PATCHv4 to do multiple lines (as defined by the netrc manpage, etc.).

>> +	# for "host X port Y" where Y is an integer (captured by
>> +	# $num_port above), set the host to "X:Y"
>> +	$tokens{host} = join(':', $tokens{host}, $num_port)
>> +		if defined $tokens{host} && defined $num_port;

JCH> What happens when 'host' does not exist? netrc/authinfo should be a JCH> stream of SP/HT/LF delimited tokens and 'machine' token (or JCH> 'default') begins a new entry, so it would mean the input file is JCH> corrupt if we do not have $tokens{host} when we get here, I think.

Yes. I'll make the host/machine token required, which will avoid this issue.

JCH> Oh, another thing. 'default' is like 'machine' followed by any JCH> machine name, so the above while loop that reads two tokens JCH> pair-wise needs to be aware that 'default' is not followed by a JCH> value. I think the loop will fail to parse this:

JCH> default login anonymous password me@home JCH> machine k.org login me password mysecret

I'd prefer to ignore "default" because it should not be used for the Git credential helpers (its only use case is for anonymous services AFAIK). So I'll add a case to ignore it in PATCHv4, if that's OK.

JCH> Hmph, aren't you checking what you read a bit too early? This is a JCH> valid input:

JCH> default JCH> login anonymous JCH> password me@home JCH> machine k.org JCH> login me JCH> password mysecret

JCH> but does this loop gives mysecret back to me when asked for JCH> host=k.org and user=me?

To be fixed in PATCHv4, which will require the host/machine, use it as the primary key, and only examine entries with it.

JCH> I would probably structure this part like this: [...]

I will do it like that, thank you for the suggestion.

I'll also add a simple testing Makefile for my own use, and you can consider adding tests to the general framework later.

Ted