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.

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

Reply via email to