svncodereview_at_gmail.com writes:
> New code review comments by sussman have been published.
> Please go to http://codereview.appspot.com/989 to read them.
Neat! But is there any way to configure it so that the (e.g.) output
below includes diff context along with the comments? The rest of this
mail is significantly less useful for not being self-contained: unless
one knows the code *really* well, one has to constantly flip back and
forth between the mail and other files / web pages in order to
understand the comments. I get that one is supposed to just go to the
main URL above (and I have), but presumably the rest of the mail was
intended to be useful...
-Karl
> 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
---------------------------------------------------------------------
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:42:10 CEST