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

[Ted Zlatanov <tzz@lifelogs.com>]

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.

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