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

On Sun, 3 Feb 2013 14:41:49 -0500 Jeff King [off-list ref] wrote: 

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

quoted

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.

quoted

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.

quoted

+use Data::Dumper;

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

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

quoted

+ 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..."

quoted

+$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.

quoted

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

quoted

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

quoted

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

quoted

+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