[Oops. Attachment attached this time, I hope.]
D.J. Heap wrote:
>
> The rest of this issue involves error messages. There are hundreds I
> have found so far that use internal style paths, and in several cases
> the path could be a url -- no distinction is made, so a url check would
> need to be made before calling the conversion function, before passing
> the path to the error creation function. Should I proceed with this?
> Any comments or recommendations?
Don't add special code to every place that generates such an error message. Use a single common function to convert a path to a form suitable for display. There are several potential things to do to a path in an error message:
* Make it absolute, so that the user can find it without having to know or guess what the current working directory was at the time. If it is going to be displayed as relative, at least display "." instead of "".
* Make sure it is quoted: not only enclosed in quotation marks, but also any funny characters within it should be quoted in some unambiguous way.
* Use the native path separator (backslashes on Windows, etc.), which is what you are interested in.
Attached is a patch that I started on recently which goes some way towards this. I am not very happy that my function "path_for_err_msg" needs to take a "pool" argument, because this makes the calling syntax even longer than it would be already, but I can't see an easy way to work without it.
Please feel free to use and extend this as you wish. I only got as far as applying it throughout the one file "io.c" which is why the function is "static", but it should obviously become global if it is to be useful.
This formatting function could, if people wish, be configurable: e.g. if absolute paths are unwieldy in most cases, it could print relative paths by default and only when someone is stuck not knowing where to look they could re-try the operation with a configuration option (debug level) set to display absolute paths.
- Julian
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 7426)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -44,6 +44,28 @@
+/* Convert a PATH into a style suitable for display in error messages. */
+/* The result will be as informative and unambiguous as possible:
+ * absolute, if possible
+ * quoted
+ * in native style (e.g. backslashes on MS Windows)
+ */
+static const char *
+path_for_err_msg (const char *path, apr_pool_t *pool)
+{
+ const char *abs_path;
+ svn_error_t *err = svn_path_get_absolute (&abs_path, path, pool);
+ if (err)
+ {
+ /* We can't return an error, because this is for use within errors. */
+ svn_error_clear (err);
+ /* Do the best we can. */
+ abs_path = (*path == '\0') ? "." : path;
+ }
+ return apr_psprintf (pool, "'%s'", svn_path_local_style (abs_path, pool));
+}
+
+
/* Helper for svn_io_check_path() and svn_io_check_resolved_path();
essentially the same semantics as those two, with the obvious
interpretation for RESOLVE_SYMLINKS. */
@@ -73,7 +95,7 @@ io_check_path (const char *path,
&& !APR_STATUS_IS_ENOENT(apr_err))
return svn_error_createf
(apr_err, NULL,
- "check_path: problem checking path \"%s\"", path);
+ "check_path: problem checking path %s", path_for_err_msg (path, pool));
else if (APR_STATUS_IS_ENOENT(apr_err))
*kind = svn_node_none;
else if (APR_STATUS_IS_ENOTDIR(apr_err))
@@ -167,7 +189,8 @@ svn_io_open_unique_file (apr_file_t **f,
*unique_name_p = NULL;
return svn_error_createf
(apr_err, NULL,
- "svn_io_open_unique_file: error opening '%s'", unique_name);
+ "svn_io_open_unique_file: error opening %s",
+ path_for_err_msg (unique_name, pool));
}
else
{
@@ -181,7 +204,7 @@ svn_io_open_unique_file (apr_file_t **f,
return svn_error_createf (SVN_ERR_IO_UNIQUE_NAMES_EXHAUSTED,
NULL,
"svn_io_open_unique_file: unable to make name for "
- "'%s'", path);
+ "%s", path_for_err_msg (path, pool));
}
@@ -212,14 +235,15 @@ svn_io_copy_file (const char *src,
{
return svn_error_createf
(apr_err, NULL,
- "svn_io_copy_file: error closing '%s'", dst_tmp);
+ "svn_io_copy_file: error closing %s",
+ path_for_err_msg (dst_tmp, pool));
}
apr_err = apr_file_copy (src_apr, dst_tmp_apr, APR_OS_DEFAULT, pool);
if (apr_err)
return svn_error_createf
- (apr_err, NULL, "svn_io_copy_file: error copying '%s' to '%s'",
- src, dst_tmp);
+ (apr_err, NULL, "svn_io_copy_file: error copying %s to %s",
+ path_for_err_msg (src, pool), path_for_err_msg (dst_tmp, pool));
/* If copying perms, set the perms on dst_tmp now, so they will be
atomically inherited in the upcoming rename. But note that we
@@ -238,7 +262,8 @@ svn_io_copy_file (const char *src,
if (apr_err)
return svn_error_createf
(apr_err, NULL,
- "svn_io_copy_file: opening '%s' for perms", src);
+ "svn_io_copy_file: opening %s for perms",
+ path_for_err_msg (src, pool));
apr_err = apr_file_info_get (&finfo, APR_FINFO_PROT, s);
if (apr_err)
@@ -246,14 +271,16 @@ svn_io_copy_file (const char *src,
apr_file_close (s); /* toss any error */
return svn_error_createf
(apr_err, NULL,
- "svn_io_copy_file: getting perm info for '%s'", src);
+ "svn_io_copy_file: getting perm info for %s",
+ path_for_err_msg (src, pool));
}
apr_err = apr_file_close (s);
if (apr_err)
return svn_error_createf
(apr_err, NULL,
- "svn_io_copy_file: closing '%s' after reading perms", src);
+ "svn_io_copy_file: closing %s after reading perms",
+ path_for_err_msg (src, pool));
apr_err = apr_file_perms_set (dst_tmp_apr, finfo.protection);
@@ -268,7 +295,8 @@ svn_io_copy_file (const char *src,
{
return svn_error_createf
(apr_err, NULL,
- "svn_io_copy_file: setting perms on '%s'", dst_tmp);
+ "svn_io_copy_file: setting perms on %s",
+ path_for_err_msg (dst_tmp, pool));
}
}
#endif /* ! SVN_WIN32 */
@@ -290,10 +318,10 @@ svn_io_append_file (const char *src, con
if (apr_err)
{
- const char *msg
- = apr_psprintf (pool, "svn_io_append_file: appending %s to %s",
- src, dst);
- return svn_error_create (apr_err, NULL, msg);
+ return svn_error_createf (apr_err, NULL,
+ "svn_io_append_file: appending %s to %s",
+ path_for_err_msg (src, pool),
+ path_for_err_msg (dst, pool));
}
return SVN_NO_ERROR;
@@ -326,20 +354,20 @@ svn_error_t *svn_io_copy_dir_recursively
SVN_ERR (svn_io_check_path (src, &kind, subpool));
if (kind != svn_node_dir)
return svn_error_createf (SVN_ERR_NODE_UNEXPECTED_KIND, NULL,
- "svn_io_copy_dir: '%s' is not a directory.",
- src);
+ "svn_io_copy_dir: %s is not a directory.",
+ path_for_err_msg (src, pool));
SVN_ERR (svn_io_check_path (dst_parent, &kind, subpool));
if (kind != svn_node_dir)
return svn_error_createf (SVN_ERR_NODE_UNEXPECTED_KIND, NULL,
- "svn_io_copy_dir: '%s' is not a directory.",
- dst_parent);
+ "svn_io_copy_dir: %s is not a directory.",
+ path_for_err_msg (dst_parent, pool));
SVN_ERR (svn_io_check_path (dst_path, &kind, subpool));
if (kind != svn_node_none)
return svn_error_createf (SVN_ERR_ENTRY_EXISTS, NULL,
- "svn_io_copy_dir: '%s' already exists.",
- dst_path);
+ "svn_io_copy_dir: %s already exists.",
+ path_for_err_msg (dst_path, pool));
SVN_ERR (svn_path_cstring_from_utf8 (&dst_path_apr, dst_path, pool));
@@ -349,8 +377,8 @@ svn_error_t *svn_io_copy_dir_recursively
if (status)
return svn_error_createf (status, NULL,
"svn_io_copy_dir: "
- "Unable to create directory '%s'",
- dst_path);
+ "Unable to create directory %s",
+ path_for_err_msg (dst_path, pool));
/* Loop over the dirents in SRC. ('.' and '..' are auto-excluded) */
SVN_ERR (svn_io_get_dirents (&dirents, src, subpool));
@@ -427,7 +455,8 @@ svn_io_make_dir_recursively (const char
if (apr_err)
return svn_error_createf
(apr_err, NULL,
- "svn_io_make_dir_recursively: error making directory '%s'", path);
+ "svn_io_make_dir_recursively: error making directory %s",
+ path_for_err_msg (path, pool));
return SVN_NO_ERROR;
#else
@@ -453,8 +482,8 @@ svn_io_make_dir_recursively (const char
if (apr_err)
svn_err = svn_error_createf
(apr_err, NULL,
- "svn_io_make_dir_recursively: error creating directory '%s'",
- path);
+ "svn_io_make_dir_recursively: error creating directory %s",
+ path_for_err_msg (path, pool));
}
return svn_err;
@@ -463,7 +492,8 @@ svn_io_make_dir_recursively (const char
/* If we get here, there must be an apr_err. */
return svn_error_createf
(apr_err, NULL,
- "svn_io_make_dir_recursively: error making '%s'", path);
+ "svn_io_make_dir_recursively: error making %s",
+ path_for_err_msg (path, pool));
#endif
}
@@ -507,7 +537,8 @@ svn_io_set_file_affected_time (apr_time_
if (status)
return svn_error_createf
(status, NULL,
- "svn_io_set_file_affected_time: can't set access time of '%s'", path);
+ "svn_io_set_file_affected_time: can't set access time of %s",
+ path_for_err_msg (path, pool));
*/
return SVN_NO_ERROR;
@@ -588,7 +619,8 @@ svn_io_file_checksum (unsigned char dige
if (apr_err && ! APR_STATUS_IS_EOF(apr_err))
return svn_error_createf
(apr_err, NULL,
- "svn_io_file_checksum: error reading from '%s'", file);
+ "svn_io_file_checksum: error reading from %s",
+ path_for_err_msg (file, pool));
apr_md5_update (&context, buf, len);
@@ -598,7 +630,8 @@ svn_io_file_checksum (unsigned char dige
if (apr_err)
return svn_error_createf
(apr_err, NULL,
- "svn_io_file_checksum: error closing '%s'", file);
+ "svn_io_file_checksum: error closing %s",
+ path_for_err_msg (file, pool));
apr_md5_final (digest, &context);
@@ -628,8 +661,8 @@ svn_io_set_file_read_only (const char *p
if (!ignore_enoent || !APR_STATUS_IS_ENOENT(status))
return svn_error_createf (status, NULL,
"svn_io_set_file_read_only: "
- "failed to set file '%s' read-only",
- path);
+ "failed to set file %s read-only",
+ path_for_err_msg (path, pool));
return SVN_NO_ERROR;
}
@@ -654,8 +687,8 @@ svn_io_set_file_read_write (const char *
if (!ignore_enoent || !APR_STATUS_IS_ENOENT(status))
return svn_error_createf (status, NULL,
"svn_io_set_file_read_write: "
- "failed to set file '%s' read-write",
- path);
+ "failed to set file %s read-write",
+ path_for_err_msg (path, pool));
return SVN_NO_ERROR;
}
@@ -687,8 +720,8 @@ svn_io_set_file_executable (const char *
if (!ignore_enoent || !APR_STATUS_IS_ENOENT(status))
return svn_error_createf (status, NULL,
"svn_io_set_file_executable: "
- "failed to change executability of file '%s'",
- path);
+ "failed to change executability of file %s",
+ path_for_err_msg (path, pool));
return SVN_NO_ERROR;
}
@@ -759,8 +792,8 @@ svn_stringbuf_from_file (svn_stringbuf_t
apr_err = apr_file_close (f);
if (apr_err)
return svn_error_createf (apr_err, NULL,
- "svn_stringbuf_from_file: failed to close '%s'",
- filename);
+ "svn_stringbuf_from_file: failed to close %s",
+ path_for_err_msg (filename, pool));
return SVN_NO_ERROR;
}
@@ -810,7 +843,8 @@ svn_stringbuf_from_aprfile (svn_stringbu
return svn_error_createf
(apr_err, NULL,
- "svn_stringbuf_from_aprfile: EOF not seen for '%s'", fname_utf8);
+ "svn_stringbuf_from_aprfile: EOF not seen for %s",
+ path_for_err_msg (fname_utf8, pool));
}
/* Null terminate the stringbuf. */
@@ -854,8 +888,8 @@ svn_io_remove_file (const char *path, ap
if (apr_err)
return svn_error_createf
(apr_err, NULL,
- "svn_io_remove_file: failed to remove file \"%s\"",
- path);
+ "svn_io_remove_file: failed to remove file %s",
+ path_for_err_msg (path, pool));
return SVN_NO_ERROR;
}
@@ -869,7 +903,7 @@ svn_io_remove_file (const char *path, ap
svn_error_t *
svn_io_remove_dir (const char *path, apr_pool_t *pool)
{
- static const char err_msg_fmt[] = "svn_io_remove_dir: removing '%s'";
+ static const char err_msg_fmt[] = "svn_io_remove_dir: removing %s";
apr_status_t status;
apr_dir_t *this_dir;
apr_finfo_t this_entry;
@@ -889,7 +923,8 @@ svn_io_remove_dir (const char *path, apr
status = apr_dir_open (&this_dir, path_apr, subpool);
if (status)
- return svn_error_createf (status, NULL, err_msg_fmt, path);
+ return svn_error_createf (status, NULL, err_msg_fmt,
+ path_for_err_msg (path, pool));
for (status = apr_dir_read (&this_entry, flags, this_dir);
status == APR_SUCCESS;
@@ -922,24 +957,27 @@ svn_io_remove_dir (const char *path, apr
we remove symlinks, pipes and whatnot, too? --xbc */
svn_error_t *err = svn_io_remove_file (fullpath, subpool);
if (err)
- return svn_error_createf (err->apr_err, err,
- err_msg_fmt, path);
+ return svn_error_createf (err->apr_err, err, err_msg_fmt,
+ path_for_err_msg (path, pool));
}
}
}
if (!APR_STATUS_IS_ENOENT (status))
- return svn_error_createf (status, NULL, err_msg_fmt, path);
+ return svn_error_createf (status, NULL, err_msg_fmt,
+ path_for_err_msg (path, pool));
else
{
status = apr_dir_close (this_dir);
if (status)
- return svn_error_createf (status, NULL, err_msg_fmt, path);
+ return svn_error_createf (status, NULL, err_msg_fmt,
+ path_for_err_msg (path, pool));
}
status = apr_dir_remove (path_apr, subpool);
if (status)
- return svn_error_createf (status, NULL, err_msg_fmt, path);
+ return svn_error_createf (status, NULL, err_msg_fmt,
+ path_for_err_msg (path, pool));
apr_pool_destroy (subpool);
@@ -1001,15 +1039,15 @@ svn_io_get_dirents (apr_hash_t **dirents
if (! (APR_STATUS_IS_ENOENT (status)))
return
svn_error_createf (status, NULL,
- "svn_io_get_dirents: error while reading dir '%s'",
- path);
+ "svn_io_get_dirents: error while reading dir %s",
+ path_for_err_msg (path, pool));
status = apr_dir_close (this_dir);
if (status)
return
svn_error_createf (status, NULL,
- "svn_io_get_dirents: failed to close dir '%s'",
- path);
+ "svn_io_get_dirents: failed to close dir %s",
+ path_for_err_msg (path, pool));
return SVN_NO_ERROR;
}
@@ -1344,9 +1382,9 @@ svn_io_run_diff3 (const char *dir,
return svn_error_createf (SVN_ERR_EXTERNAL_PROGRAM, NULL,
"svn_io_run_diff3: "
"Error running '%s': exitcode was %d, args were:"
- "\nin directory '%s', basenames:\n%s\n%s\n%s",
+ "\nin directory %s, basenames:\n%s\n%s\n%s",
diff3_utf8, *exitcode,
- dir, mine, older, yours);
+ path_for_err_msg (dir, pool), mine, older, yours);
return SVN_NO_ERROR;
}
@@ -1373,8 +1411,8 @@ svn_io_detect_mimetype (const char **mim
if (kind != svn_node_file)
return svn_error_createf (SVN_ERR_BAD_FILENAME, NULL,
"svn_io_detect_mimetype: "
- "Can't detect mimetype of non-file '%s'",
- file);
+ "Can't detect mimetype of non-file %s",
+ path_for_err_msg (file, pool));
SVN_ERR (svn_io_file_open (&fh, file, APR_READ, 0, pool));
@@ -1382,8 +1420,8 @@ svn_io_detect_mimetype (const char **mim
apr_err = apr_file_read (fh, block, &amt_read);
if (apr_err && ! APR_STATUS_IS_EOF(apr_err))
return svn_error_createf (apr_err, NULL,
- "svn_io_detect_mimetype: error reading '%s'",
- file);
+ "svn_io_detect_mimetype: error reading %s",
+ path_for_err_msg (file, pool));
/* Now close the file. No use keeping it open any more. */
apr_file_close (fh);
@@ -1442,7 +1480,8 @@ svn_io_file_open (apr_file_t **new_file,
if (status)
return svn_error_createf (status, NULL,
- "svn_io_file_open: can't open '%s'", fname);
+ "svn_io_file_open: can't open %s",
+ path_for_err_msg (fname, pool));
else
return SVN_NO_ERROR;
}
@@ -1465,7 +1504,8 @@ svn_io_stat (apr_finfo_t *finfo, const c
if (status)
return svn_error_createf (status, NULL,
- "svn_io_stat: couldn't stat '%s'...", fname);
+ "svn_io_stat: couldn't stat %s",
+ path_for_err_msg (fname, pool));
else
return SVN_NO_ERROR;
}
@@ -1485,8 +1525,9 @@ svn_io_file_rename (const char *from_pat
if (status)
return svn_error_createf (status, NULL,
- "svn_io_file_rename: can't move '%s' to '%s'",
- from_path, to_path);
+ "svn_io_file_rename: can't move %s to %s",
+ path_for_err_msg (from_path, pool),
+ path_for_err_msg (to_path, pool));
else
return SVN_NO_ERROR;
}
@@ -1508,7 +1549,8 @@ dir_make (const char *path, apr_fileperm
if (status)
return svn_error_createf (status, NULL,
- "can't create directory '%s'", path);
+ "can't create directory %s",
+ path_for_err_msg (path, pool));
#ifdef APR_FILE_ATTR_HIDDEN
if (hidden)
@@ -1519,7 +1561,8 @@ dir_make (const char *path, apr_fileperm
pool);
if (status)
return svn_error_createf (status, NULL,
- "can't hide directory '%s'", path);
+ "can't hide directory %s",
+ path_for_err_msg (path, pool));
}
#endif
@@ -1556,8 +1599,8 @@ svn_io_dir_open (apr_dir_t **new_dir, co
if (status)
return svn_error_createf (status, NULL,
- "svn_io_dir_open: unable to open directory '%s'",
- dirname);
+ "svn_io_dir_open: unable to open directory %s",
+ path_for_err_msg (dirname, pool));
else
return SVN_NO_ERROR;
}
@@ -1576,8 +1619,8 @@ svn_io_dir_remove_nonrecursive (const ch
if (status)
return svn_error_createf (status, NULL,
"svn_io_dir_remove_nonrecursive: "
- "unable to remove directory '%s'",
- dirname);
+ "unable to remove directory %s",
+ path_for_err_msg (dirname, pool));
else
return SVN_NO_ERROR;
}
@@ -1627,8 +1670,8 @@ svn_io_dir_walk (const char *dirname,
if (apr_err)
return svn_error_createf (apr_err, NULL,
"svn_io_dir_walk: unable to open "
- "directory '%s'",
- dirname);
+ "directory %s",
+ path_for_err_msg (dirname, pool));
/* iteration subpool */
subpool = svn_pool_create (pool);
@@ -1646,7 +1689,8 @@ svn_io_dir_walk (const char *dirname,
{
return svn_error_createf (apr_err, NULL,
"svn_io_dir_walk: error reading "
- "directory entry in '%s'", dirname);
+ "directory entry in %s",
+ path_for_err_msg (dirname, pool));
}
if (finfo.filetype == APR_DIR)
@@ -1706,9 +1750,8 @@ svn_io_dir_walk (const char *dirname,
apr_err = apr_dir_close (handle);
if (apr_err)
return svn_error_createf (apr_err, NULL,
- "svn_io_dir_walk: error closing "
- "directory '%s'",
- dirname);
+ "svn_io_dir_walk: error closing directory %s",
+ path_for_err_msg (dirname, pool));
return SVN_NO_ERROR;
}
@@ -1813,8 +1856,8 @@ svn_io_dir_empty (svn_boolean_t *is_empt
else
return svn_error_createf (status, NULL,
"svn_io_dir_empty: "
- "unable to check directory '%s'",
- path);
+ "unable to check directory %s",
+ path_for_err_msg (path, pool));
return SVN_NO_ERROR;
}
@@ -1845,12 +1888,14 @@ svn_io_write_version_file (const char *p
apr_err = apr_file_write_full (format_file, format_contents,
strlen (format_contents), NULL);
if (apr_err)
- return svn_error_createf (apr_err, 0, "writing to '%s'", path);
+ return svn_error_createf (apr_err, 0, "writing to %s",
+ path_for_err_msg (path, pool));
/* ...and close the file. */
apr_err = apr_file_close (format_file);
if (apr_err)
- return svn_error_createf (apr_err, 0, "closing '%s'", path);
+ return svn_error_createf (apr_err, 0, "closing %s",
+ path_for_err_msg (path, pool));
return SVN_NO_ERROR;
}
@@ -1872,12 +1917,13 @@ svn_io_read_version_file (int *version,
len = sizeof(buf);
apr_err = apr_file_read (format_file, buf, &len);
if (apr_err)
- return svn_error_createf (apr_err, 0, "reading '%s'", path);
+ return svn_error_createf (apr_err, 0, "reading %s",
+ path_for_err_msg (path, pool));
/* If there was no data in PATH, return an error. */
if (len == 0)
return svn_error_createf (SVN_ERR_STREAM_UNEXPECTED_EOF, NULL,
- "reading '%s'", path);
+ "reading %s", path_for_err_msg (path, pool));
/* Check that the first line contains only digits. */
{
@@ -1892,7 +1938,8 @@ svn_io_read_version_file (int *version,
if (! apr_isdigit (c))
return svn_error_createf
(SVN_ERR_BAD_VERSION_FILE_FORMAT, NULL,
- "first line of '%s' contains non-digit", path);
+ "first line of %s contains non-digit",
+ path_for_err_msg (path, pool));
}
}
@@ -1902,7 +1949,8 @@ svn_io_read_version_file (int *version,
/* And finally, close the file. */
apr_err = apr_file_close (format_file);
if (apr_err)
- return svn_error_createf (apr_err, 0, "closing '%s'", path);
+ return svn_error_createf (apr_err, 0, "closing %s",
+ path_for_err_msg (path, pool));
return SVN_NO_ERROR;
}
@@ -1937,15 +1985,15 @@ contents_identical_p (svn_boolean_t *ide
if (status && !APR_STATUS_IS_EOF(status))
return svn_error_createf
(status, NULL,
- "contents_identical_p: full read failed on '%s'.",
- file1);
+ "contents_identical_p: full read failed on %s.",
+ path_for_err_msg (file1, pool));
status = apr_file_read_full (file2_h, buf2, sizeof(buf2), &bytes_read2);
if (status && !APR_STATUS_IS_EOF(status))
return svn_error_createf
(status, NULL,
- "contents_identical_p: full read failed on '%s'.",
- file2);
+ "contents_identical_p: full read failed on %s.",
+ path_for_err_msg (file2, pool));
if ((bytes_read1 != bytes_read2)
|| (memcmp (buf1, buf2, bytes_read1)))
@@ -1959,13 +2007,15 @@ contents_identical_p (svn_boolean_t *ide
if (status)
return svn_error_createf
(status, NULL,
- "contents_identical_p: failed to close '%s'.", file1);
+ "contents_identical_p: failed to close %s.",
+ path_for_err_msg (file1, pool));
status = apr_file_close (file2_h);
if (status)
return svn_error_createf
(status, NULL,
- "contents_identical_p: failed to close '%s'.", file2);
+ "contents_identical_p: failed to close %s.",
+ path_for_err_msg (file2, pool));
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 25 02:01:44 2003