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

Jeff King [off-list ref] writes:

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

quoted

quoted

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

OK.  The code didn't express the above reasoning clearly enough.

I was trying not to be too nit-picky with my review,...

I wasn't either.  Mine was still at design level review to get the
semantics right (e.g. what to consider as errors, the input is _not_
one entry per line, etc.), before reviewing the details of the
implementation.

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.

Yeah, I think that looks like the right top-level codeflow.