Dear dev, arfrever.fta,
New code review comments by sussman have been published.
Please go to http://codereview.appspot.com/989 to read them.
Message:
I've made a bunch of comments in the code, pointed out a few small problems.
Details:
http://codereview.appspot.com/989/diff/1/4
File subversion/include/svn_auth_kwallet.h (right):
http://codereview.appspot.com/989/diff/1/4#newcode19
Line 19: * @brief Subversion's authentication system - Support for KWallet
I don't understand why a whole new public header was created just to
advertise your new auth provider. The OS X keychain, provider (for
example) declares itself in svn_auth.h. Can't we do the same for
kwallet provider?
http://codereview.appspot.com/989/diff/1/9
File subversion/libsvn_auth_kwallet/kwallet.cpp (right):
http://codereview.appspot.com/989/diff/1/9#newcode58
Line 58: if (non_interactive)
This routine has a few 'if' blocks where the curly braces aren't
properly indented (per subversion style.)
http://codereview.appspot.com/989/diff/1/9#newcode82
Line 82: QString key = QString::fromUtf8(username) + "@" + QString::fromUtf8(realmstring);
Line is > 80 columns.
http://codereview.appspot.com/989/diff/1/9#newcode85
Line 85: KWallet::Wallet *wallet =
KWallet::Wallet::openWallet(wallet_name, wid, KWallet::Wallet::Synchronous);
Same here.
http://codereview.appspot.com/989/diff/1/9#newcode95
Line 95: *password = apr_pstrmemdup(pool, q_password.toUtf8().data(), q_password.size());
Same here.
http://codereview.appspot.com/989/diff/1/9#newcode116
Line 116: if (non_interactive)
This function also has curly-brace indentation problems.
http://codereview.appspot.com/989/diff/1/9#newcode141
Line 141: KWallet::Wallet *wallet =
KWallet::Wallet::openWallet(wallet_name, wid, KWallet::Wallet::Synchronous);
Line >80 columns.
http://codereview.appspot.com/989/diff/1/9#newcode152
Line 152: QString key = QString::fromUtf8(username) + "@" + QString::fromUtf8(realmstring);
Same here.
http://codereview.appspot.com/989/diff/1/7
File subversion/libsvn_subr/cmdline.c (right):
http://codereview.appspot.com/989/diff/1/7#newcode372
Line 372: funcname =
apr_psprintf(pool, "svn_auth_get_%s_simple_provider", provider_name);
Line >80 chars.
http://codereview.appspot.com/989/diff/1/7#newcode430
Line 430: if (get_auth_simple_provider(&provider, "kwallet", pool))
I'm confused here. Why does the kwallet provider need to dynamically
load a whole new library, but keychain and windows providers don't?
http://codereview.appspot.com/989/diff/1/8
File subversion/libsvn_subr/simple_providers.c (right):
http://codereview.appspot.com/989/diff/1/8#newcode52
Line 52: #define SVN_AUTH__KWALLET_PASSWORD_TYPE "kwallet"
Uhoh, this same constant is defined in kwallet.cpp! For safety, it
should only be defined in one place.
http://codereview.appspot.com/989/diff/1/8#newcode273
Line 273: || strcmp(passtype, SVN_AUTH__KWALLET_PASSWORD_TYPE) == 0)
This isn't your fault, Arfrever, but I worry about the sustainability
of this logic. I wonder if we shouldn't create a follow-up change
which abstracts "password type" into a real structure with a "bool
stores_plaintext_password" field. It seems like we've outgrown
passtypes just being strings. (Remember that someone is going to add
Gnome keyring support too!)
Issue Description:
Sincerely,
Your friendly code review daemon (http://codereview.appspot.com/).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-15 17:32:34 CEST