On 27/05/17 17:23, Daniel Shahaf wrote:
> Patch + log message attached. It changes the expected output, and I'm not
> sure how strictly compatible we want to be about that, hence posting here
> first.
>
> If I don't hear anything I'll go ahead and commit.
>
> (It seems that the code being changed is passing an APR-encoded path to
> a printing function that expects a UTF-8 argument? This might even be
> user-visible as mangled output in non-UTF-8 environments, I suppose,
> but I don't have such an environment to test with.)
Hi Daniel.
Your patch looks correct, as far as it goes. The 'canonical path' change
(where the main effect is it adds a leading slash) is OK as it makes for
greater consistency. The conversion to UTF-8 is also correct, but this
makes me notice the following issue.
It looks like lots of the argument parsing in 'svnadmin' lacks UTF-8
conversion. I think this should be done in the function 'parse_args'
instead of everywhere else. There are also a few places where it is
missing in our other command-line programs.
To avoid more instances of the same error in the future, I suggest that
rather than continuing to use this pattern:
const char *arg, *arg_utf8;
arg = os->argv[os->ind++];
svn_utf_cstring_to_utf8(&arg_utf8, arg);
do_stuff(arg_utf8);
do_more_stuff(arg); /* BUG: should be arg_utf8 */
we could use this pattern which is more robust:
const char *arg;
svn_utf_cstring_to_utf8(&arg, os->argv[os->ind++]);
do_stuff(arg);
do_more_stuff(arg);
I see some other instances of this same problem elsewhere in the
codebase, following uses of svn_opt_parse_num_args() and
svn_opt_parse_all_args() which are two more functions that don't convert
the command-line arguments to UTF-8 before returning them, leaving their
callers to do it (wrongly).
I also noticed the 'unlock' regression tests use patterns ending with
"unlocked." which looks like the dot was intended to match literally but
actually matches a space, as it is interpreted as a regex and the output
looks like "unlocked by user 'xxx'."
You could commit your patch anyway, or switch it to the more robust
pattern first. I might follow up with these other issues if no-one else
volunteers.
- Julian
Received on 2017-05-30 16:19:33 CEST