On Sun, 3 Feb 2013 14:41:49 -0500 Jeff King <peff@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html