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

On Mon, 04 Feb 2013 14:56:06 -0800 Junio C Hamano <gitster@pobox.com> wrote: 

JCH> I recall that netrc/authinfo files are _not_ line oriented. Earlier JCH> you said "looks for entries that match" which is a lot more correct, JCH> but then we see "look for lines in authfile".

Hmm, do you mean backslashed newlines? I think the Net::Netrc parser doesn't support them, and I haven't seen them in the wild, but I could support them if you think that's useful.

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.

JCH> By the way, I think statement modifiers tend to get overused and JCH> make the resulting program harder to read. die "..." at the JCH> beginning of line makes the reader go "Whoa, it already is done and JCH> existing on error", and then forces the eyes to scan the error JCH> message to find "unless" and the condition.

JCH> It may be a cute syntax and some may find it even cool, but cuteness JCH> or coolness is less valuable compared with the readability.

Your coding guidelines said you prefer one-line if statements, and I thought it would be OK to lean on modifiers. I changed many of the modifiers but not all; please let me know if you'd like me to change them all. It's no problem.

JCH> Is it sensible to squelch the error message by default and force JCH> user to specify --debug? You could argue that the option is to JCH> debug the user's configuration, but the name of the option sounds JCH> more like it is for debugging this script itself.

It's both... without a clear separation because it's such a small script. Let me know how you'd like to change it, if at all.

JCH> I saw Peff already pointed out error conditions, but I am not sure JCH> why all of these exit with 0. If the user has configured

JCH> 	git config credential.helper 'netrc -f $HOME/.netcr'

JCH> shouldn't it be diagnosed as an error? It is understandable to let JCH> this go silently

JCH> 	git config credential.helper 'netrc'

JCH> and let other credential helpers take over when no $HOME/.{netrc,authinfo}{,.gpg} JCH> file exist, but in that case the user may still want to remove the JCH> config item that is not doing anything useful and erroring out with JCH> a message may be a way to help the user know about the situation.

You and Peff should tell me how it should behave, or perhaps make the changes after it's in. I'm happy to change it any way you like, but at this point I'm just following instructions, not really contributing, about the exit statuses. I thought I knew what you wanted 2 iterations ago :)

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?

PATCHv3 is out with the rest of your suggestions. Thank you for the thorough review. I am happy to improve the script to meet your standards.

Thanks Ted