Jeff King <peff@xxxxxxxx> writes:
> 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.
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.
--
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