Ted Zlatanov <t...@lifelogs.com> writes:
>>> +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';
>
> JCH> The above looks strange. Why does the invoker get the error message
> JCH> only when it runs this without arguments? Did you mean to say more
> JCH> like this?
>
> JCH> unless (defined $mode && $mode eq 'get') {
> JCH> die "...";
> JCH> }
>
> I mean:
>
> - if the mode is not given, exit badly (since it's required)
>
> - if the mode is given but we don't support it, exit pleasantly
>
> I thought that was the right thing, according to my reading of the
> credentials API. If not, I'll be glad to change it.
As Peff noted, I mistead what the code was doing, especially with
somewhat cryptic "only support x mode" comment, as if it is
rejecting other modes.
>>> + print STDERR "Sorry, we could not load data from [$file]\n" if $debug;
>>> + exit;
>
> JCH> Is this really an error? The file perhaps was empty. Shouldn't
> JCH> that case treated the same way as the case where no entry that
> JCH> matches the criteria invoker gave you was found?
>
> exit(0) is not an error, so the behavior is exactly the same, we just
> don't print anything to STDOUT because there was no data, with a nicer
> error message. I think that's what we want?
"Sorry we couldn't" sounded like an error messag to me. If this is
a normal exit, then please make sure it is a normal exit.
The review cycle is not like reviewers give you instructions and
designs and you blindly implement them. It is a creative process
where you show the design and a clear implementation of that design.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
