[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] svnadmin: Print LOCK_PATH's correctly.

From: Julian Foad <julianfoad_at_apache.org>
Date: Tue, 30 May 2017 15:19:27 +0100

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

This is an archived mail posted to the Subversion Dev mailing list.