Re: [PATCH] a little step in issue 897: non-recursivesvn_handle_error()

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. :) :) :) :)

Index: subversion/libsvn_subc/error.c
===================================================================
--- subversion/libsvn_subc/error.c (revision 7520)
+++ subversion/libsvn_subc/error.c (working copy)
@@ -224,8 +224,7 @@

 static void
-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)
 {
   char errbuf[256];
   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));
   fflush (stream);

- 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 ();
 }

 void
 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 ();
+ }
 }

 --
Shamim Islam
BA BS

> 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.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org