On 12/23/10 3:01 AM, Daniel Shahaf wrote:
I've made a quick sketch --- looks good?
The suggestions look good.
We could add some more tests, checking the behavior with SVN_NO_ERROR input and
one where all errors are tracing, which should return an error from
SVN_ERR_ASSERT().
As an aside, there's a comment about adding an svn_boolean_t to svn_error_t
instead of using strcmp(), which could be done in a totally separate commit.
Index: subversion/mod_dav_svn/deadprops.c
===================================================================
--- subversion/mod_dav_svn/deadprops.c (revision 1052167)
+++ subversion/mod_dav_svn/deadprops.c (working copy)
@@ -222,18 +222,22 @@
+ purged_serr->message = apr_xml_quote_string
+ (purged_serr->pool,
Our current policy is to have the '(' on the same line as the function name.
I looked through the hacking guide and didn't see anything, but doing some greps
through our code shows a disposition to this style:
$ cd subversion/
$ grep '^ *(' */*.c| wc -l
1757
$ grep '($' */*.c| wc -l
454
Index: subversion/mod_dav_svn/version.c
===================================================================
--- subversion/mod_dav_svn/version.c (revision 1052167)
+++ subversion/mod_dav_svn/version.c (working copy)
@@ -922,16 +922,16 @@
{
if (serr)
{
- const char *post_commit_err;
- apr_err = serr->apr_err;
- post_commit_err = svn_repos__post_commit_error_str
- (serr, resource->pool);
- serr = SVN_NO_ERROR;
- ap_log_perror(APLOG_MARK, APLOG_ERR, apr_err, resource->pool,
+ const char *post_commit_err = svn_repos__post_commit_error_str
+ (serr, resource->pool);
+ ap_log_perror(APLOG_MARK, APLOG_ERR, serr->apr_err,
+ resource->pool,
The rest looks good. (I am assuming --- didn't check --- that you
updated all callers.)
Yes.
Blair
