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

[Jeff King <peff@peff.net>]

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

> I wrote a Perl credential helper for netrc parsing which is pretty
> robust, has built-in docs with -h, and doesn't depend on external
> modules.  The netrc parser regex was stolen from Net::Netrc.
>
> It will by default use ~/.authinfo.gpg, ~/.netrc.gpg, ~/.authinfo, and
> ~/.netrc (whichever is found first) and this can be overridden with -f.

Cool, thanks for working on this.

> 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?

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

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

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

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

> +use Data::Dumper;

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

> + print <<EOHIPPUS;

Cute, I haven't seen that one before.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> [...]

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

-Peff