Julian Foad wrote on Tue, 30 May 2017 15:19 +0100:
> 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.
+1: transcoding is an aspect of argv parsing, not of the business logic.
The subcommand_*() functions should be passed UTF-8 arguments.
> There are also a few places where it is
> missing in our other command-line programs.
>
> 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);
+1 to this idiom as well. In general, the non-UTF-8 variables should
have as little visibility as possible. (And if we ever have to store one
of them in a named variable, we ought to name that variable to reflect
its value's encoding --- i.e., Hungarian notation.)
> 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'."
I think the output didn't always include the "by user 'xxx'" portion, so the
regex used to match the whole line. I'll extend the expectations to match
the username.
> 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.
>
Thanks for the review, Julian. I'll commit the patch as-is when I have
a moment, and probably add something to the release notes about the
output change as well. (In case people are regex-matching the output...)
Received on 2017-05-31 00:07:55 CEST