Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2

[Jeff King <peff@peff.net>]

On Mon, Feb 04, 2013 at 02:56:06PM -0800, Junio C Hamano wrote:

> > +my $mode = shift @ARGV;
> > +
> > +# credentials may get 'get', 'store', or 'erase' as parameters but
> > +# only acknowledge 'get'
> > +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
> > +
> > +# only support 'get' mode
> > +exit unless $mode eq 'get';
> 
> The above looks strange.  Why does the invoker get the error message
> only when it runs this without arguments?  Did you mean to say more
> like this?
> 
> 	unless (defined $mode && $mode eq 'get') {
> 		die "...";
> 	}

Not having a mode is an invocation error; the credential-helper
documentation indicates that the helper will always be invoked with an
action. The likely culprit for not having one is the user invoking it
manually, and showing the usage there is a sensible action.

Whereas invoking it with a mode other than "get" is not an error at all.
Git will run it with the "store" and "erase" actions, too. Those happen
to be no-ops for this helper, so it exits silently. The credential docs
specify that any other actions should be ignored, too, to allow for
future expansion.

> > +my $debug = $options{debug};
> > +my $file = $options{file};
> > +
> > +unless (defined $file) {
> > +	print STDERR "Please specify an existing netrc file (with or without a .gpg extension) with -f AUTHFILE\n" if $debug;
> > +	exit 0;
> > +}
> > +
> > +unless (-f $file) {
> > +	print STDERR "Sorry, the specified netrc $file is not accessible\n" if $debug;
> > +	exit 0;
> > +}
> 
> Perhaps "-r $file", if you say "is not accessible"?

Even better: look at whether opening the file was successful. Though I
guess that is complicated by the use of gpg, who will probably not
distinguish ENOENT from other failures for us.

> Is it sensible to squelch the error message by default and force
> user to specify --debug?  You could argue that the option is to
> debug the user's configuration, but the name of the option sounds
> more like it is for debugging this script itself.
> 
> I saw Peff already pointed out error conditions, but I am not sure
> why all of these exit with 0.  If the user has configured

It was from my suggestion to ignore missing files, which is that the
user might have the helper configured (e.g., via /etc/gitconfig, or by a
shared ~/.gitconfig) but not actually have a netrc.

It gets confusing because the contents of $file may have been
auto-detected, or it may have come from the command-line, and we do not
remember which at this point.

I was trying not to be too nit-picky with my review, but here is how I
would have written the outer logic of the script:

  my $tokens = read_credential_data_from_stdin();
  if ($options{file}) {
          my @entries = load_netrc($options{file})
                  or die "unable to open $options{file}: $!";
          check_netrc($tokens, @entries);
  }
  else {
          foreach my $ext ('.gpg', '') {
                  foreach my $base (qw(authinfo netrc)) {
                          my @entries = load_netrc("$base$ext")
                                  or next;
                          if (check_netrc($tokens, @entries)) {
                                  last;
                          }
                  }
          }
  }

I.e., to fail on "-f", but otherwise treat unreadable auto-selected
files as a no-op, for whatever reason. I'd also consider checking all
files if they are available, in case the user has multiple (e.g., they
keep low-quality junk unencrypted but some high-security passwords in a
.gpg file). Not that likely, but not any harder to implement.

-Peff