From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ted Zlatanov Subject: Re: [PATCH] git-send-email: add ~/.authinfo parsing Date: Mon, 04 Feb 2013 11:40:54 -0500 Organization: =?utf-8?B?0KLQtdC+0LTQvtGAINCX0LvQsNGC0LDQvdC+0LI=?= @ Cienfuegos Message-ID: <87wquovxpl.fsf@lifelogs.com> References: <2f93ce7b6b5d3f6c6d1b99958330601a5560d4ba.1359486391.git.mina86@mina86.com> <7vvcafojf4.fsf@alter.siamese.dyndns.org> <20130130074306.GA17868@sigill.intra.peff.net> <7v7gmumzo6.fsf@alter.siamese.dyndns.org> <87pq0l5qbc.fsf@lifelogs.com> <20130131193844.GA14460@sigill.intra.peff.net> <87k3qrx712.fsf@lifelogs.com> <20130203194148.GA26318@sigill.intra.peff.net> Mime-Version: 1.0 Content-Type: text/plain Cc: git@vger.kernel.org To: Jeff King X-From: git-owner@vger.kernel.org Mon Feb 04 17:41:21 2013 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1U2P6d-0001Yy-MO for gcvg-git-2@plane.gmane.org; Mon, 04 Feb 2013 17:41:20 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756532Ab3BDQk5 (ORCPT ); Mon, 4 Feb 2013 11:40:57 -0500 Received: from z.lifelogs.com ([173.255.230.239]:38225 "EHLO z.lifelogs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756391Ab3BDQkz (ORCPT ); Mon, 4 Feb 2013 11:40:55 -0500 Received: from heechee (c-65-96-148-157.hsd1.ma.comcast.net [65.96.148.157]) (using TLSv1 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) (Authenticated sender: tzz) by z.lifelogs.com (Postfix) with ESMTPSA id BFB74DE0C5; Mon, 4 Feb 2013 16:40:54 +0000 (UTC) X-Face: bd.DQ~'29fIs`T_%O%C\g%6jW)yi[zuz6;d4V0`@y-~$#3P_Ng{@m+e4o<4P'#(_GJQ%TT= D}[Ep*b!\e,fBZ'j_+#"Ps?s2!4H2-Y"sx" Mail-Copies-To: never Gmane-Reply-To-List: yes In-Reply-To: <20130203194148.GA26318@sigill.intra.peff.net> (Jeff King's message of "Sun, 3 Feb 2013 14:41:49 -0500") User-Agent: Gnus/5.130006 (Ma Gnus v0.6) Emacs/24.3.50 (gnu/linux) Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: On Sun, 3 Feb 2013 14:41:49 -0500 Jeff King 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 < 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 () >> +{ >> + 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