You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Erik Huelsmann <e....@gmx.net> on 2003/11/09 20:30:09 UTC
[PATCH] part of issue #897: errors could be better.
Ok. As promised, here is the patch which only selectively prints the general
error message. Some tests had to be adapted because they depend on the
general error being in the reported output.
bye,
Erik.
PS: The patch is also available at:
http://encorps.dnsalias.com/patches/897-no-general-on-specific-error.patch
Log:
[[[
Change reported errors (cf issue #897) to show only the specific
message if one is supplied, in favor of printing both - almost
equal - general and specific messages.
* subversion/libsvn_subr/error.c
(svn_handle_error): Print either the specific or the general
message
* subversion/tests/clients/cmdline/commit_tests.py
* subversion/tests/clients/cmdline/diff_tests.py
* subversion/tests/clients/cmdline/switch_tests.py
* subversion/tests/clients/cmdline/update_tests.py
Change checks which depend on the general error message
to check for the specific one.
]]]
Index: subversion/libsvn_subr/error.c
===================================================================
--- subversion/libsvn_subr/error.c (revision 7680)
+++ subversion/libsvn_subr/error.c (working copy)
@@ -245,7 +245,10 @@
#endif /* SVN_DEBUG */
/* Only print the same APR error string once. */
- if (print_strerror)
+ if (err->message)
+ fprintf (stream, "svn: %s\n",
+ convert_string_for_output (err->message, err->pool));
+ else if (print_strerror)
{
/* Is this a Subversion-specific error code? */
if ((err->apr_err > APR_OS_START_USEERR)
@@ -258,9 +261,6 @@
fprintf (stream, "svn: %s\n", err_string);
}
- if (err->message)
- fprintf (stream, "svn: %s\n",
- convert_string_for_output (err->message, err->pool));
}
void
Index: subversion/tests/clients/cmdline/diff_tests.py
===================================================================
--- subversion/tests/clients/cmdline/diff_tests.py (revision 7680)
+++ subversion/tests/clients/cmdline/diff_tests.py (working copy)
@@ -526,8 +526,11 @@
# error message. The appropriate response is a few lines of errors. I
wish
# there was a way to figure out if svn crashed, but all run_svn gives us
is
# the output, so here we are...
- if len(err_output) <= 1: raise svntest.Failure
-
+ for line in err_output:
+ if re.search("foo' is not a versioned resource$", line):
+ break
+ else:
+ raise svntest.Failure
# test 9
def diff_pure_repository_update_a_file(sbox):
@@ -777,8 +780,9 @@
diff_output, err_output = svntest.main.run_svn(1, 'diff',
'--old', non_extant_url,
'--new', extant_url)
+
for line in err_output:
- if re.match('svn: Filesystem has no item$', line):
+ if re.search('was not found in the repository at revision', line):
break
else:
raise svntest.Failure
@@ -787,7 +791,7 @@
'--old', extant_url,
'--new', non_extant_url)
for line in err_output:
- if re.match('svn: Filesystem has no item$', line):
+ if re.search('was not found in the repository at revision', line):
break
else:
raise svntest.Failure
Index: subversion/tests/clients/cmdline/update_tests.py
===================================================================
--- subversion/tests/clients/cmdline/update_tests.py (revision 7680)
+++ subversion/tests/clients/cmdline/update_tests.py (working copy)
@@ -686,7 +686,7 @@
"Updating failed", None, SVNAnyOutput, 'up', wc_dir)
for line in errput:
- if re.match(".*Obstructed update.*", line):
+ if re.match("svn: failed to delete file 'alpha'", line):
return
# Else never matched the expected error output, so the test failed.
@@ -942,7 +942,7 @@
for n in range(2):
out, err = svntest.main.run_svn(1, 'up', wc_dir)
for line in err:
- if line.find("Obstructed update") != -1:
+ if line.find("object of the same name already exists") != -1:
break
else:
raise svntest.Failure
Index: subversion/tests/clients/cmdline/commit_tests.py
===================================================================
--- subversion/tests/clients/cmdline/commit_tests.py (revision 7680)
+++ subversion/tests/clients/cmdline/commit_tests.py (working copy)
@@ -484,7 +484,7 @@
svntest.actions.run_and_verify_commit (wc_dir,
None,
None,
- "Can't find an entry",
+ r'.* is not a versioned
resource$',
None, None,
None, None,
os.path.join(wc_dir,'blorg'))
@@ -1356,7 +1356,7 @@
svntest.actions.run_and_verify_commit(wc_dir,
None,
None,
- 'already-locked',
+ 'svn: working copy locked: ',
None, None,
None, None,
wc_dir)
Index: subversion/tests/clients/cmdline/switch_tests.py
===================================================================
--- subversion/tests/clients/cmdline/switch_tests.py (revision 7680)
+++ subversion/tests/clients/cmdline/switch_tests.py (working copy)
@@ -178,10 +178,11 @@
# Try to commit. We expect this to fail because, if all the
# switching went as expected, A/B/pi and A/D/G/pi point to the
# same URL. We don't allow this.
- svntest.actions.run_and_verify_commit(wc_dir, None, None,
- "commit to a URL more than once",
- None, None, None, None,
- wc_dir)
+ svntest.actions.run_and_verify_commit(
+ wc_dir, None, None,
+ "svn: Cannot commit both .* as they refer to the same URL.$",
+ None, None, None, None,
+ wc_dir)
# Okay, that all taken care of, let's revert the A/D/G/pi path and
# move along. Afterward, we should be okay to commit. (Sorry,
--
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
Re: [PATCH] part of issue #897: errors could be better.
Posted by Erik Huelsmann <e....@gmx.net>.
To follow up on my own mail: The error message only eliminates the first
line in the example below if there is a specific message to print. You will get
the general error when there is no specific one.
> There's nothing wrong with the general error, but the general error
> duplicates the information in the specific error. Very often output can be
> expected
> along the lines of:
>
> svn: File not found
> svn: Can't find file 'foo'
After applying the patch you get the output:
svn: Can't find file 'foo'
which both Greg Hudson and I find as informative as the first example, yet
looking far cleaner.
If there were an error without a specific message, like this one:
svn: Tried an operation for versioned resources on an unversioned one
The reported error would be the same before and after this patch.
bye,
Erik.
--
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
Re: [PATCH] part of issue #897: errors could be better.
Posted by Erik Huelsmann <e....@gmx.net>.
> Clear yes, but why brief?
Because users tend to not-read error messages from programs longer than two
or three lines (I find myself not-doing this too).
> > Brevity is not what you get when repeating yourself in output error
> > messages.
> If repetition is such a crime, how about
>
> svn: File not found
> svn: 'foo'
The ideal situation would be IMO that the general error contains a "'%s'"
for the name of the file. For the time being, the solution would be to write
specific log messages as close as possible to the general one in order to get
the same effect.
> Suppose we are returning an SVN_ERR_FOO error, at present the general
> error error text allows us to identify the error as a FOO error. If
> the FOO error is generated in several places there will no longer be a
> single message that identifies a FOO error. Can you explain why this
> is clearly an improvement?
This part of the problem is solved by the 'for the time being' solution
above.
bye,
Erik.
--
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
Re: [PATCH] part of issue #897: errors could be better.
Posted by Philip Martin <ph...@codematters.co.uk>.
"Erik Huelsmann" <e....@gmx.net> writes:
> This point was brought up by Greg Hudson, on the thread the issue
> points to, but also yesterday on IRC.
>
> There's nothing wrong with the general error, but the general error
> duplicates the information in the specific error. Very often output
> can be expected along the lines of:
>
> svn: File not found
> svn: Can't find file 'foo'
>
> When you want to clean up error messages, you should consider being
> clear and brief.
Clear yes, but why brief?
> Brevity is not what you get when repeating yourself in output error
> messages.
If repetition is such a crime, how about
svn: File not found
svn: 'foo'
Suppose we are returning an SVN_ERR_FOO error, at present the general
error error text allows us to identify the error as a FOO error. If
the FOO error is generated in several places there will no longer be a
single message that identifies a FOO error. Can you explain why this
is clearly an improvement?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] part of issue #897: errors could be better.
Posted by Erik Huelsmann <e....@gmx.net>.
This point was brought up by Greg Hudson, on the thread the issue points to,
but also yesterday on IRC.
There's nothing wrong with the general error, but the general error
duplicates the information in the specific error. Very often output can be expected
along the lines of:
svn: File not found
svn: Can't find file 'foo'
When you want to clean up error messages, you should consider being clear
and brief. Brevity is not what you get when repeating yourself in output error
messages.
bye,
Erik.
> "Erik Huelsmann" <e....@gmx.net> writes:
>
> > [[[
> > message if one is supplied, in favor of printing both - almost
> > equal - general and specific messages.
>
> I'm not sure why we would want to make this change.
>
> > for line in err_output:
> > - if re.match('svn: Filesystem has no item$', line):
> > + if re.search('was not found in the repository at revision', line):
> > break
> > else:
> > raise svntest.Failure
>
> What's wrong with having the general message?
>
> --
> Philip Martin
>
--
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
Re: [PATCH] part of issue #897: errors could be better.
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sun, 2003-11-09 at 15:43, Philip Martin wrote:
> > message if one is supplied, in favor of printing both - almost
> > equal - general and specific messages.
This log message is a bit unclear; it should read "instead of" and not
"in favor of." I don't know whether this confused Phillip or not.
> What's wrong with having the general message?
Because it's ugly and imposing to say the same thing twice in error
messages. This is not a case where more is better.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] part of issue #897: errors could be better.
Posted by Philip Martin <ph...@codematters.co.uk>.
"Erik Huelsmann" <e....@gmx.net> writes:
> [[[
> message if one is supplied, in favor of printing both - almost
> equal - general and specific messages.
I'm not sure why we would want to make this change.
> for line in err_output:
> - if re.match('svn: Filesystem has no item$', line):
> + if re.search('was not found in the repository at revision', line):
> break
> else:
> raise svntest.Failure
What's wrong with having the general message?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org