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

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

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;
> +}

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

Show 31 quoted lines

> +# there are CPAN modules to do this better, but we want to avoid
> +# dependencies and generally, complex netrc-style files are rare
> +
> +if ($debug) {
> +	printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)'
> +		foreach sort keys %q;
> +}
> +
> +LINE: foreach my $line (@data) {
> +
> +	print STDERR "line [$line]\n" if $debug;
> +	my @tok;
> +	# gratefully stolen from Net::Netrc
> +	while (length $line &&
> +	       $line =~ s/^("((?:[^"]+|\\.)*)"|((?:[^\\\s]+|\\.)*))\s*//) {
> +		(my $tok = $+) =~ s/\\(.)/$1/g;
> +		push(@tok, $tok);
> +	}
> +
> +	# skip blank lines, comments, etc.
> +	next LINE unless scalar @tok;
> +
> +	my %tokens;
> +	my $num_port;
> +	while (@tok) {
> +		my ($k, $v) = (shift @tok, shift @tok);
> +		next unless defined $v;
> +		next unless exists $options{tmap}->{$k};
> +		$tokens{$options{tmap}->{$k}} = $v;
> +		$num_port = $v if $k eq 'port' && $v =~ m/^\d+$/;
> +	}

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

> +	# 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;

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

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

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

> +	foreach my $check (sort keys %q) {

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

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

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

Show 8 quoted lines

> +		if (exists $tokens{$check} && defined $q{$check}) {
> +			print STDERR "comparing [$tokens{$check}] to [$q{$check}] in line [$line]\n" if $debug;
> +			next LINE unless $tokens{$check} eq $q{$check};
> +		}
> +		else {
> +			print STDERR "we could not find [$check] but it's OK\n" if $debug;
> +		}
> +	}

I would probably structure this part like this:

	%pending = ();
        split the whole input into tokens, regardless of lines;
        iterate over the tokens {
		peek the token
		if (it is not "default") {
			take (token, value) pair;
		} else {
			take "default" as token; value does not matter.
		}
                if (token is "default" or "machine") {
			# finished reading one entry and we are
                        # at the beginning of the next entry.
                        # see if this entry matches
			if (%pending is not empty &&
                            %pending matches %q) {
				found a match; use %pending;
			}
                        # done with that entry. now start a new one.
                        %pending = ();
		}
		$pending{token} = value;
	}