You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by pr...@apache.org on 2013/07/03 12:49:02 UTC
svn commit: r1499315 - in /subversion/branches/verify-keep-going/subversion:
libsvn_repos/dump.c svnadmin/svnadmin.c
Author: prabhugs
Date: Wed Jul 3 10:49:01 2013
New Revision: 1499315
URL: http://svn.apache.org/r1499315
Log:
Format the summary of corrupt revisions.
* subversion/libsvn_repos/dump.c
(notify_verification_error_summary): Show the best error message instead of
the entire error chain in the summary.
* subversion/svnadmin/svnadmin.c
(repos_notify_handler): Do not print the entire error chain in the summary
of corrupt revisions.
Modified:
subversion/branches/verify-keep-going/subversion/libsvn_repos/dump.c
subversion/branches/verify-keep-going/subversion/svnadmin/svnadmin.c
Modified: subversion/branches/verify-keep-going/subversion/libsvn_repos/dump.c
URL: http://svn.apache.org/viewvc/subversion/branches/verify-keep-going/subversion/libsvn_repos/dump.c?rev=1499315&r1=1499314&r2=1499315&view=diff
==============================================================================
--- subversion/branches/verify-keep-going/subversion/libsvn_repos/dump.c (original)
+++ subversion/branches/verify-keep-going/subversion/libsvn_repos/dump.c Wed Jul 3 10:49:01 2013
@@ -1375,6 +1375,7 @@ notify_verification_error_summary(svn_re
apr_pool_t *pool)
{
svn_repos_notify_t *notify_failure;
+ char errbuf[512]; /* ### svn_strerror() magic number */
if (notify_func == NULL)
return;
@@ -1382,6 +1383,11 @@ notify_verification_error_summary(svn_re
notify_failure = svn_repos_notify_create(svn_repos_notify_failure_summary,
pool);
notify_failure->err = err;
+ notify_failure->warning_str = apr_psprintf(pool,
+ _("E%06d: %s"),
+ err->apr_err,
+ svn_err_best_message(err, errbuf,
+ sizeof(errbuf)));
notify_failure->revision = rev;
notify_func(notify_baton, notify_failure, pool);
}
Modified: subversion/branches/verify-keep-going/subversion/svnadmin/svnadmin.c
URL: http://svn.apache.org/viewvc/subversion/branches/verify-keep-going/subversion/svnadmin/svnadmin.c?rev=1499315&r1=1499314&r2=1499315&view=diff
==============================================================================
--- subversion/branches/verify-keep-going/subversion/svnadmin/svnadmin.c (original)
+++ subversion/branches/verify-keep-going/subversion/svnadmin/svnadmin.c Wed Jul 3 10:49:01 2013
@@ -850,16 +850,13 @@ repos_notify_handler(void *baton,
case svn_repos_notify_failure_summary:
if (notify->revision != SVN_INVALID_REVNUM)
cmdline_stream_printf(feedback_stream, scratch_pool,
- _("\nRevision %ld \n"),
- notify->revision);
- if (notify->err)
- svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
- "svnadmin: ");
+ _("Revision %ld: %s \n"),
+ notify->revision, notify->warning_str);
return;
case svn_repos_notify_summary:
cmdline_stream_printf(feedback_stream, scratch_pool,
- _("\n-----Summary of corrupt revisions-----"));
+ _("\n-----Summary of corrupt revisions-----\n"));
return;
case svn_repos_notify_dump_rev_end:
Re: svn commit: r1499315 - in
/subversion/branches/verify-keep-going/subversion:
libsvn_repos/dump.c svnadmin/svnadmin.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Wed, Jul 03, 2013 at 13:48:57 +0200:
> On 03.07.2013 13:37, Daniel Shahaf wrote:
> > prabhugs@apache.org wrote on Wed, Jul 03, 2013 at 10:49:02 -0000:
> >> + notify_failure->warning_str = apr_psprintf(pool,
> >> + _("E%06d: %s"),
> > Would it make sense to push the E%06d: part down to the implementation
> > of svn_err_best_message()?
> >
> > I.e., if the information is useful for this caller, presumably it'll be
> > useful for others too?
>
> Error message formatting should not be the domain of libsvn_subr.
> Different API consumers may want to format the error codes differently,
> and it's a bad idea to make them parse it out.
This is a #svn_repos_notify_failure notification, and the API does not
define the value of ->warning_str in that case. It seems to me we
should leave it unset; it suffices to pass the error in the ->err member
for the API caller to do with as it pleases.
Good catch.
Re: svn commit: r1499315 - in /subversion/branches/verify-keep-going/subversion:
libsvn_repos/dump.c svnadmin/svnadmin.c
Posted by Branko Čibej <br...@wandisco.com>.
On 03.07.2013 13:37, Daniel Shahaf wrote:
> prabhugs@apache.org wrote on Wed, Jul 03, 2013 at 10:49:02 -0000:
>> + notify_failure->warning_str = apr_psprintf(pool,
>> + _("E%06d: %s"),
> Would it make sense to push the E%06d: part down to the implementation
> of svn_err_best_message()?
>
> I.e., if the information is useful for this caller, presumably it'll be
> useful for others too?
Error message formatting should not be the domain of libsvn_subr.
Different API consumers may want to format the error codes differently,
and it's a bad idea to make them parse it out.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1499315 - in /subversion/branches/verify-keep-going/subversion:
libsvn_repos/dump.c svnadmin/svnadmin.c
Posted by Prabhu <pr...@collab.net>.
On 07/03/2013 05:07 PM, Daniel Shahaf wrote:
> prabhugs@apache.org wrote on Wed, Jul 03, 2013 at 10:49:02 -0000:
>> + notify_failure->warning_str = apr_psprintf(pool,
>> + _("E%06d: %s"),
> Would it make sense to push the E%06d: part down to the implementation
> of svn_err_best_message()?
>
> I.e., if the information is useful for this caller, presumably it'll be
> useful for others too?
>
+1
It would always be useful for other callers too because it makes more
sense to show the apr_err when it comes to best message.
--Prabhu
Re: svn commit: r1499315 - in /subversion/branches/verify-keep-going/subversion:
libsvn_repos/dump.c svnadmin/svnadmin.c
Posted by Prabhu <pr...@collab.net>.
On 07/03/2013 05:07 PM, Daniel Shahaf wrote:
> prabhugs@apache.org wrote on Wed, Jul 03, 2013 at 10:49:02 -0000:
>> + notify_failure->warning_str = apr_psprintf(pool,
>> + _("E%06d: %s"),
> Would it make sense to push the E%06d: part down to the implementation
> of svn_err_best_message()?
>
> I.e., if the information is useful for this caller, presumably it'll be
> useful for others too?
>
+1
It would always be useful for other callers too because it makes more
sense to show the apr_err when it comes to best message.
--Prabhu
RE: svn commit: r1499315 - in /subversion/branches/verify-keep-going/subversion: libsvn_repos/dump.c svnadmin/svnadmin.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:danielsh@elego.de]
> Sent: woensdag 3 juli 2013 13:38
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org; prabhugs@apache.org
> Subject: Re: svn commit: r1499315 - in /subversion/branches/verify-keep-
> going/subversion: libsvn_repos/dump.c svnadmin/svnadmin.c
>
> prabhugs@apache.org wrote on Wed, Jul 03, 2013 at 10:49:02 -0000:
> > + notify_failure->warning_str = apr_psprintf(pool,
> > + _("E%06d: %s"),
>
> Would it make sense to push the E%06d: part down to the implementation
> of svn_err_best_message()?
>
> I.e., if the information is useful for this caller, presumably it'll be
> useful for others too?
No, not in the existing function. That is a breaking change to api users.
UI clients usually don't want to show things in the same way as 'svn'. They
might want to show it in different ways or want to include only parts of the
error if they recognize the error code. And if you go this way this is
impossible due to localization.
This sort of formatting belongs in svn_cmdline if it is shared between
commandline clients, not in the lower layers of the error api.
-----------
I don't see why '_("E%06d: %s"),' should be localized. I don't think it
really can be with just this string.
Bert
RE: svn commit: r1499315 - in /subversion/branches/verify-keep-going/subversion: libsvn_repos/dump.c svnadmin/svnadmin.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:danielsh@elego.de]
> Sent: woensdag 3 juli 2013 13:38
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org; prabhugs@apache.org
> Subject: Re: svn commit: r1499315 - in /subversion/branches/verify-keep-
> going/subversion: libsvn_repos/dump.c svnadmin/svnadmin.c
>
> prabhugs@apache.org wrote on Wed, Jul 03, 2013 at 10:49:02 -0000:
> > + notify_failure->warning_str = apr_psprintf(pool,
> > + _("E%06d: %s"),
>
> Would it make sense to push the E%06d: part down to the implementation
> of svn_err_best_message()?
>
> I.e., if the information is useful for this caller, presumably it'll be
> useful for others too?
No, not in the existing function. That is a breaking change to api users.
UI clients usually don't want to show things in the same way as 'svn'. They
might want to show it in different ways or want to include only parts of the
error if they recognize the error code. And if you go this way this is
impossible due to localization.
This sort of formatting belongs in svn_cmdline if it is shared between
commandline clients, not in the lower layers of the error api.
-----------
I don't see why '_("E%06d: %s"),' should be localized. I don't think it
really can be with just this string.
Bert
Re: svn commit: r1499315 - in
/subversion/branches/verify-keep-going/subversion:
libsvn_repos/dump.c svnadmin/svnadmin.c
Posted by Daniel Shahaf <da...@elego.de>.
prabhugs@apache.org wrote on Wed, Jul 03, 2013 at 10:49:02 -0000:
> + notify_failure->warning_str = apr_psprintf(pool,
> + _("E%06d: %s"),
Would it make sense to push the E%06d: part down to the implementation
of svn_err_best_message()?
I.e., if the information is useful for this caller, presumably it'll be
useful for others too?
> + err->apr_err,
> + svn_err_best_message(err, errbuf,
> + sizeof(errbuf)));
Re: svn commit: r1499315 - in
/subversion/branches/verify-keep-going/subversion:
libsvn_repos/dump.c svnadmin/svnadmin.c
Posted by Daniel Shahaf <da...@elego.de>.
prabhugs@apache.org wrote on Wed, Jul 03, 2013 at 10:49:02 -0000:
> + notify_failure->warning_str = apr_psprintf(pool,
> + _("E%06d: %s"),
Would it make sense to push the E%06d: part down to the implementation
of svn_err_best_message()?
I.e., if the information is useful for this caller, presumably it'll be
useful for others too?
> + err->apr_err,
> + svn_err_best_message(err, errbuf,
> + sizeof(errbuf)));