Kamesh Jayachandran wrote:
> Thanks Julian, DLR, Garrett.
> Julian,
> Not sure how to reduce it further eventhough I could understand your
> points on stuffs like some spurious newline introduced by the program
> that generates the dump file.
See below.
> [[[
> Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
> intolerably vague
>
> * subversion/libsvn_repos/load.c
> (contextual_header_block_error): New function to return a detailed
> error message.
> (read_header_block): Call contextual_header_block_error().
>
> ]]]
> Index: subversion/libsvn_repos/load.c
> ===================================================================
> --- subversion/libsvn_repos/load.c (revision 17993)
> +++ subversion/libsvn_repos/load.c (working copy)
> @@ -105,6 +105,58 @@
> }
>
>
> +/* Compose and return an error describing the problem REASON with the
> + * dump file header KEY, and the block where these header key is located.
> + */
> +static svn_error_t *
> +contextual_header_block_error (apr_hash_t *headers, const char *key, const char *reason)
> +{
> + char *block;
> + if ((block = apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_REVISION_NUMBER,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed revision '%s' header block "
> + "near the line starting with a key '%s' %s "
> + "in dumpfile stream"), block, key, reason);
> + }
> + else if ((block = apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_NODE_PATH,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed node '%s' header block "
> + "near the line starting with a key '%s' %s "
> + "in dumpfile stream"), block, key, reason);
> + }
> + else if ((block = apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_UUID,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed UUID header block "
> + "near the line starting with a key '%s' %s "
> + "in dumpfile stream"), key, reason);
> + }
> + else if ((block = apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_MAGIC_HEADER,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed magic header block "
> + "near the line starting with a key '%s' %s "
> + "in dumpfile stream"), key, reason);
> + }
> + else
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed unknown header block "
> + "near the line starting with a key '%s' %s "
> + "in dumpfile stream"), key, reason);
> + }
> +}
There's a lot of duplication in that function. At the least I would like to
see the common code (well, text, mainly) factored out. Additionally I would
not bother trying to report the content of another header from the same block,
unless there is evidence that doing so is really useful. As I said before, I
suspect that it is not often really useful.
> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> - _("Found malformed header block "
> - "in dumpfile stream"));
> + return contextual_header_block_error (*headers, header_str->data,
> + _("with no ':' character"));
Translating a sentence fragment doesn't work in all languages. Generally, text
to be translated has to be in complete sentences.
Here is what I am suggesting for the whole patch:
> Index: subversion/libsvn_repos/load.c
> ===================================================================
> --- subversion/libsvn_repos/load.c (revision 18020)
> +++ subversion/libsvn_repos/load.c (working copy)
> @@ -153,9 +153,10 @@
> while (header_str->data[i] != ':')
> {
> if (header_str->data[i] == '\0')
> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> - _("Found malformed header block "
> - "in dumpfile stream"));
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Dump stream contains a malformed "
> + "header (with no colon): '%s'"),
> + header_str->data);
> i++;
> }
> /* Create a 'name' string and point to it. */
> @@ -165,9 +166,10 @@
> /* Skip over the NULL byte and the space following it. */
> i += 2;
> if (i > header_str->len)
> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> - _("Found malformed header block "
> - "in dumpfile stream"));
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Dump stream contains a malformed "
> + "header (no value after colon): '%s'"),
> + header_str->data);
>
> /* Point to the 'value' string. */
> value = header_str->data + i;
I can already see another problem: the supposed malformed header string is
quite likely to be a block of arbitrary ("binary") data of arbitrary size.
Therefore it would be polite not to print it raw but to restrict the length and
character set of what we print.
Further, I see that load.c also emits other vague dumpfile errors (e.g. in
stream_malformed()) that could do with a bit more detail.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jan 10 18:46:20 2006