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...)
