I've made a bunch of comments in the code, pointed out a few small problems. 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!)
