[PATCH] Revised patch: first step in issue 897

Hi,

Because of the comments of Brane and Files, I compiled and tested a new
revision of the patch I submitted last week. The patch addresses two point in the
issue (at first it addressed only 1):

1) make svn_handle_error not recursive;
2) make sure NULL arguments are accepted by svn_create_error and document
this.

The patch can be found below. The second point makes it possible to generate
errors without specific error messages; something we may want to do in the
future. svn_handle_error was already prepared for such cases.

bye,

Erik.

PS: If the patch is wrapped by my mailer, a copy of the patch can be
obtained at http://encorps.dnsalias.com/patches/897-libsvn_subr-error-cleanup.patch
until I find a good domain to host at.

Log message
[[[
Cleanup step to start issue 897 (error messages could be better)
Patch based on ideas brought up by Files (files@poetryunlimited.com)
and Brane (brane@xbc.nu) adding my own tweaks.

* subversion/include/svn_error.h
  Document that the message argument to svn_create_error should
  be more specific than the general error associated with the
  error code. Otherwise the argument should be NULL.

* subversion/libsvn_subr/error.c
  (svn_create_error): don't apr_pstrdup NULL message
  (handle_error): don't recurse through error structures, instead
     print only the current err; rename to print_error
  (svn_handle_error): loop over error structures, flushing after
     last call to print_error (instead of each structure)

]]]

Index: subversion/include/svn_error.h
===================================================================
--- subversion/include/svn_error.h (revision 7590)
+++ subversion/include/svn_error.h (working copy)
@@ -68,14 +68,16 @@
  *
  * Input: an APR or SVN custom error code,
  * a "child" error to wrap,
- * a descriptive message,
+ * a specific message
  *
  * Returns: a new error structure (containing the old one).
  *
  * Notes: Errors are always allocated in a subpool of the global pool,
  * since an error's lifetime is generally not related to the
  * lifetime of any convenient pool. Errors must be freed
- * with @c svn_error_clear().
+ * with @c svn_error_clear(). The specific message should be NULL
+ * if there is nothing to add to the general message associated
+ * with the error code.
  *
  * If creating the "bottommost" error in a chain, pass @c NULL for
  * the child argument.
Index: subversion/libsvn_subr/error.c
===================================================================
--- subversion/libsvn_subr/error.c (revision 7590)
+++ subversion/libsvn_subr/error.c (working copy)
@@ -121,7 +121,8 @@
 
   err = make_error_internal (apr_err, child);
 
- err->message = (const char *) apr_pstrdup (err->pool, message);
+ if (message)
+ err->message = (const char *) apr_pstrdup (err->pool, message);
 
   return err;
 }
@@ -224,8 +225,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 print_strerror)
 {
   char errbuf[256];
   const char *err_string;
@@ -243,10 +243,9 @@
 
   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 (print_strerror)
     {
       /* Is this a Subversion-specific error code? */
       if ((err->apr_err > APR_OS_START_USEERR)
@@ -262,24 +261,28 @@
   if (err->message)
     fprintf (stream, "svn: %s\n",
              convert_string_for_output (err->message, err->pool));
- fflush (stream);
+}
 
- if (err->child)
- handle_error (err->child, stream, FALSE, depth + 1, err->apr_err);
+void
+svn_handle_error (svn_error_t *err, FILE *stream, svn_boolean_t fatal)
+{
+ apr_status_t parent_apr_err = APR_SUCCESS;
 
+ while (err)
+ {
+ print_error (err, stream, (err->apr_err != parent_apr_err));
+ parent_apr_err = err->apr_err;
+ err = err->child;
+ }
+
+ fflush (stream);
   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);
-}
 
-
 void
 svn_handle_warning (FILE *stream, svn_error_t *err)
 {

-- 
NEU FÜR ALLE - GMX MediaCenter - für Fotos, Musik, Dateien...
Fotoalbum, File Sharing, MMS, Multimedia-Gruß, GMX FotoService
Jetzt kostenlos anmelden unter http://www.gmx.net
+++ GMX - die erste Adresse für Mail, Message, More! +++
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org