Re: [PATCH] git-send-email: add ~/.authinfo parsing

From: Ted Zlatanov <tzz@lifelogs.com>
Date: Feb 4, 2013, 16:40 UTC
Message-ID: <87wquovxpl.fsf@lifelogs.com>
In-Reply-To: <20130203194148.GA26318@sigill.intra.peff.net>

On Sun, 3 Feb 2013 14:41:49 -0500 Jeff King <peff@peff.net> wrote:

JK> On Sat, Feb 02, 2013 at 06:57:29AM -0500, Ted Zlatanov wrote:

>> If the file name ends with ".gpg", it will run "gpg --decrypt FILE" and
>> use the output.  So non-interactively, that could hang if GPG was
>> waiting for input.  Does Git handle that, or should I check for a TTY?

JK> No, git does not do anything special with respect to credential helpers JK> and ttys (nor should it, since one use of helpers is to get credentials JK> when there is no tty). I think it is GPG's problem to deal with, though. JK> We will invoke it, and it is up to it to decide whether it can acquire JK> the passphrase or not (either through the tty, or possibly from JK> gpg-agent). So it would be wrong to do the tty check yourself.

JK> I haven't tested GPG, but I assume it properly tries to read from JK> /dev/tty and not stdin. Your helper's stdio is connected to git and JK> speaking the credential-helper protocol, so GPG reading from stdin would JK> either steal your input (if run before you read it), or just get EOF (if JK> you have read all of the pipe content already). If GPG isn't well JK> behaved, it may be worth redirecting its stdin from /dev/null as a JK> safety measure.

In my testing GPG did the right thing, so I think this is OK.

>> Take a look at the proposed patch and let me know if it's usable, if you
>> need a formal copyright assignment, etc.

JK> Overall looks sane to me, though my knowledge of .netrc is not JK> especially good. Usually we try to send patches inline in the email JK> (i.e., as generated by git-format-patch), and include a "Signed-off-by" JK> line indicating that content is released to the project; see JK> Documentation/SubmittingPatches.

OK, thanks.  I will fire that off.

>> +use Data::Dumper;

JK> I don't see it used here. Leftover from debugging?

It's part of my Perl new script skeleton, sorry.

>> + print <<EOHIPPUS;

JK> Cute, I haven't seen that one before.

Heh heh. I've had to explain that one in code review many times. "See, it's the precursor to the modern horse..."

>> +$0 [-f AUTHFILE] [-d] get
>> +
>> +Version $VERSION by tzz\@lifelogs.com.  License: any use is OK.

JK> I don't know if we have a particular policy for items in contrib/, but JK> this license may be too vague. In particular, it does not explicitly JK> allow redistribution, which would make Junio shipping a release with it JK> a copyright violation.

JK> Any objection to just putting it under some well-known simple license JK> (GPL, BSD, or whatever)?

No, I didn't know what Git requires, and I'd like it to be the least restrictive, so BSD is OK. Stated in -h now.

>> +if ($file =~ m/\.gpg$/)
>> +{
>> + $file = "gpg --decrypt $file|";
>> +}

JK> Does this need to quote $file, since the result will get passed to the JK> shell? It might be easier to just use the list form of open(), like:

JK> my @data = $file =~ /\.gpg$/ ? JK> load('-|', qw(gpg --decrypt), $file) : JK> load('<', $file);

JK> (and then obviously update load to just dump all of @_ to open()).

Yes, thanks.  Done.

>> +die "Sorry, we could not load data from [$file]"
>> + unless (scalar @data);

JK> Probably not that interesting a corner case, but this means we die on an JK> empty .netrc, whereas it might be more sensible for it to behave as "no JK> match".

JK> For the same reason, it might be worth silently exiting when we don't JK> find a .netrc (or any of its variants). That lets people who share their JK> dot-files across machines configure git globally, even if they don't JK> necessarily have a netrc on every machine.

OK; done.

Show 7 quoted lines

>> +# the query
>> +my %q;
>> +
>> +foreach my $v (values %{$options{tmap}})
>> +{
>> + undef $q{$v};
>> +}

JK> Just my personal style, but I find the intent more obvious with "map" (I JK> know some people find it unreadable, though):

JK>   my %q = map { $_ => undef } values(%{$options{tmap}});

Yes, changed.

>> +while (<STDIN>)
>> +{
>> + next unless m/([a-z]+)=(.+)/;

JK> We don't currently have any exotic tokens that this would not match, nor JK> do I plan to add them, but the credential documentation defines a valid JK> line as /^([^=]+)=(.+)/.

JK> It's also possible for the value to be empty, but I do not think JK> off-hand that current git will ever send such an empty value.

Yes, changed.

JK> The rest of it looks fine to me. I don't think any of my comments are JK> show-stoppers. Tests would be nice, but integrating contrib/ stuff with JK> the test harness is kind of a pain.

"I tested it on AIX, it works great!" :)

It's pretty easy to write a local Makefile with a test target, if you think it worthwhile.

Ted