> -----Original Message-----
> From: stsp_at_apache.org [mailto:stsp_at_apache.org]
> Sent: zondag 12 mei 2013 21:32
> To: commits_at_subversion.apache.org
> Subject: svn commit: r1481628 -
> /subversion/trunk/subversion/libsvn_repos/commit.c
>
> Author: stsp
> Date: Sun May 12 19:32:01 2013
> New Revision: 1481628
>
> URL: http://svn.apache.org/r1481628
> Log:
> * subversion/libsvn_repos/commit.c
> (illegal_path_escape): Remove.
> (check_valid_path): Update caller, use svn_path_illegal_path_escape()
> instead.
>
> Modified:
> subversion/trunk/subversion/libsvn_repos/commit.c
>
> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/c
> ommit.c?rev=1481628&r1=1481627&r2=1481628&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/commit.c Sun May 12
> 19:32:01 2013
> @@ -259,61 +259,6 @@ make_dir_baton(struct edit_baton *edit_b
> return db;
> }
>
> -/* Return a copy of PATH, allocated from POOL, for which control
> - characters have been escaped using the form \NNN (where NNN is the
> - octal representation of the byte's ordinal value). */
> -static const char *
> -illegal_path_escape(const char *path, apr_pool_t *pool)
> -{
> - svn_stringbuf_t *retstr;
> - apr_size_t i, copied = 0;
> - int c;
> -
> - /* At least one control character:
> - strlen - 1 (control) + \ + N + N + N + null . */
> - retstr = svn_stringbuf_create_ensure(strlen(path) + 4, pool);
> - for (i = 0; path[i]; i++)
> - {
> - c = (unsigned char)path[i];
> - if (! svn_ctype_iscntrl(c))
> - continue;
> -
> - /* If we got here, we're looking at a character that isn't
> - supported by the (or at least, our) URI encoding scheme. We
> - need to escape this character. */
> -
> - /* First things first, copy all the good stuff that we haven't
> - yet copied into our output buffer. */
> - if (i - copied)
> - svn_stringbuf_appendbytes(retstr, path + copied,
> - i - copied);
> -
> - /* Make sure buffer is big enough for '\' 'N' 'N' 'N' (and NUL) */
> - svn_stringbuf_ensure(retstr, retstr->len + 4);
> - /*### The backslash separator doesn't work too great with Windows,
> - but it's what we'll use for consistency with invalid utf8
> - formatting (until someone has a better idea) */
> - apr_snprintf(retstr->data + retstr->len, 5, "\\%03o", (unsigned char)c);
> - retstr->len += 4;
> -
> - /* Finally, update our copy counter. */
> - copied = i + 1;
> - }
> -
> - /* If we didn't encode anything, we don't need to duplicate the string. */
> - if (retstr->len == 0)
> - return path;
> -
> - /* Anything left to copy? */
> - if (i - copied)
> - svn_stringbuf_appendbytes(retstr, path + copied, i - copied);
> -
> - /* retstr is null-terminated either by apr_snprintf or the svn_stringbuf
> - functions. */
> -
> - return retstr->data;
> -}
> -
> /* Reject paths which contain control characters (related to issue #4340). */
> static svn_error_t *
> check_valid_path(const char *path,
> @@ -326,7 +271,7 @@ check_valid_path(const char *path,
> if (svn_ctype_iscntrl(*c))
> return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> _("Invalid control character '0x%02x' in path '%s'"),
> - (unsigned char)*c, illegal_path_escape(path, pool));
> + (unsigned char)*c, svn_path_illegal_path_escape(path, pool));
[Not really targeted to this specific commit, but I don't see the real revision that introduced this code... Maybe some 'following up' would improve the log message]
Is it a requirement for all error creators to escape all valid but unexpected characters, or should console specific clients such as 'svn' make sure their error output doesn't contain unexpected escape characters?
I'm not sure if we are really fixing this in the right place. I would guess that we should fix this in the output routines which know that control characters are really control characters.
We shouldn't add much processing to the creation of error messages as all that effort is lost in svn_error_clear() that is often applied to code paths.
In this specific case I'm pretty sure that the error will actually just go to the user, but in general I would be -0.5 on performing this kind of escapes in the creation of error messages that are part of our libraries that are also used by GUIs and bindings that might want to apply different handling and see the raw values.
Bert
Received on 2013-05-12 22:49:39 CEST