Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(477)

Issue 989: kwallet final branch

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 9 months ago by sussman
Modified:
9 years, 6 months ago
Reviewers:
arfrever.fta, dev
Base URL:
http://svn.collab.net/repos/svn/trunk
Visibility:
Public.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+640 lines, -118 lines) Patch
Makefile.in View 7 chunks +16 lines, -5 lines 0 comments Download
build.conf View 2 chunks +13 lines, -0 lines 0 comments Download
build/generator/gen_base.py View 2 chunks +16 lines, -14 lines 0 comments Download
configure.ac View 4 chunks +103 lines, -2 lines 0 comments Download
subversion/include/private/svn_auth_private.h View 1 chunk +90 lines, -0 lines 0 comments Download
subversion/include/svn_auth.h View 4 chunks +6 lines, -3 lines 0 comments Download
subversion/include/svn_auth_kwallet.h View 1 chunk +54 lines, -0 lines 1 comment Download
subversion/libsvn_auth_kwallet/kwallet.cpp View 1 chunk +220 lines, -0 lines 7 comments Download
subversion/libsvn_subr/cmdline.c View 4 chunks +38 lines, -1 line 2 comments Download
subversion/libsvn_subr/simple_providers.c View 18 chunks +84 lines, -93 lines 2 comments Download

Messages

Total messages: 1
sussman
10 years, 9 months ago (2008-05-15 15:14:40 UTC) #1
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!)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b