[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: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Tue, 30 May 2017 22:07:48 +0000

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

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