I noticed a bug in the last posting. (there was an == instead of an !=).
What's w/ this "print_message" parameter though. That's really not it's role.
It's role is "new_message", no? :) :) :) :)
The outside world should know nothing of the choices made inside the black
box. It should only supply the information.
The black box should decide what to do when it sees a new message. :) :) :)
Also, I disagree w/ the fatal trigger to abort being in the secondary
function. The secondary function really isn't *handling* anything. It's focus
should be printing.
Why the static function that "prints" error messages is called "handle_error"
is beyond me. I've called it "print_error". It really doesn't
open/close/finalize anything. Or did I miss that part?
And lastly, I keep thinking that the debug pretty printing should happen in
the main handler, since it really doesn't make use of the buffers and what
not, but I think that's 6 of one and half dozen of the other.
I do agree that the functions need to be small. I also agree w/ you Greg that
the changes need to be idiomatic. Esp. in reference to the use of 'for'.
That being said, here is my proposal. I've attached a bz2 file to preserve
formatting. If you like, great. If not, it was fun to do. :) :) :) :)
--- subversion/libsvn_subc/error.c (revision 7520)
+++ subversion/libsvn_subc/error.c (working copy)
@@ -224,8 +224,7 @@
-handle_error (svn_error_t *err, FILE *stream, svn_boolean_t fatal,
- int depth, apr_status_t parent_apr_err)
+print_error (svn_error_t *err, FILE *stream, svn_boolean_t new_message)
const char *err_string;
@@ -244,9 +243,8 @@
fprintf (stream, ": (apr_err=%d)\n", err->apr_err);
#endif /* SVN_DEBUG */
- /* When we're recursing, don't repeat the top-level message if its
- the same as before. */
- if (depth == 0 || err->apr_err != parent_apr_err)
+ /* Only print the same APR error string once. */
+ if (new_message)
/* Is this a Subversion-specific error code? */
if ((err->apr_err > APR_OS_START_USEERR)
@@ -264,19 +262,23 @@
convert_string_for_output (err->message, err->pool));
- if (err->child)
- handle_error (err->child, stream, FALSE, depth + 1, err->apr_err);
- if (fatal)
- /* XXX Shouldn't we exit(1) here instead, so that atexit handlers
- get called? --xbc */
- abort ();
svn_handle_error (svn_error_t *err, FILE *stream, svn_boolean_t fatal)
- handle_error (err, stream, fatal, 0, APR_SUCCESS);
+ for(apr_status_t last_apr_err = APR_SUCCESS;
+ err != NULL;
+ last_apr_err = err->apr_err , err = err->child)
+ print_error (err, stream, last_apr_err != err->apr_err);
+ if (fatal)
+ /* XXX Shouldn't we exit(1) here instead, so that atexit handlers
+ get called? --xbc */
+ abort ();
> On the other hand, I don't think it's terribly worthwhile to make
> svn_handle_error() iterative without simplifying the function as
> recommended in the larger parts of #897.
Received on Sun Oct 26 15:39:54 2003
To unsubscribe, e-mail: email@example.com
For additional commands, e-mail: firstname.lastname@example.org