Re: svn commit: r1925722 - in /subversion/branches/utf8-cmdline-prototype/subversion: include/svn_client.h libsvn_client/cmdline.c

On 26. 5. 25 14:07, Timofei Zhakov wrote:
> On Tue, May 20, 2025 at 9:29 PM Branko Čibej <[email protected]> wrote:
>
>     On 20. 5. 25 20:31, [email protected] wrote:
>>     Author: rinrab
>>     Date: Tue May 20 18:31:35 2025
>>     New Revision: 1925722
>>
>>     URL:http://svn.apache.org/viewvc?rev=1925722&view=rev  <http://svn.apache.org/viewvc?rev=1925722&view=rev>
>>     Log:
>>     On the 'utf8-cmdline-prototype' branch: add a new function that retrieves
>>     target list from args, instead of converting the encoding, assumes that
>>     args have already been converted.
>
>
>     Once again, why don't you use apr_app_initialize() that does all
>     that and more, instead of perpetuating the same problem? As long
>     as this is used on Windows from main() instead of wmain(), there's
>     no way it will work consistently.
>
>
> First I wanna mention that this exact change is still needed even if 
> we decide to use apr app to host the binary. At this point there is no 
> difference between using apr app and svn's implementation of wmain and 
> main wrappers.
>
> Also, I'd like to argue about using apr app in general. The 
> subversion's command line wasn't using this library during the whole 
> history (as far as I know), so I don't think it's correct to start 
> using it now. What would we do if we need some functionality apr app 
> doesn't implement? I mean that would be a quite huge change...
>
> In my opinion, apr app is not something fully ready to use in 
> subversion. For example, it doesn't wrap main on unix platforms, while 
> it still can be executed with non-utf8-ized arguments. In wmain() 
> apr_conv_ucs2_to_utf8() just ignores the error, which is bad (on win32).
>
> And finally, the code we have right now is pretty good, and it seems 
> possible to support non-ASCII arguments (which I'm working on).
>
> I hope this makes some sense.


OK, fine, but I still don't see where you're converting the argument 
list directly from UTF-16-LE to UTF-8. 
svn_cmdline__win32_get_cstring_argv doesn't do that, and it should.

That's the main problem on Windows: that function is converting to the 
local multibyte character set, then svn_client_args_to_target_array2 
converts /that/ to UTF-8. That's fine-ish on Unix, where we don't 
convert twice, but completely brain-dead on Windows. And I freely admit 
I had a lot do do with that brain-deadedness.

This is what should be happening, IMO:

 1. deprecate svn_client_args_to_target_array2
 2. rename your svn_client_args_to_target_array_utf8
    svn_client_args_to_target_array3 and make it explicitly require that
    the args are already in UTF-8
 3. svn_cmdline__win32_get_cstring_argv must convert to UTF-8
 4. svn_cmdline__default_get_cstring_argv must *also* convert to UTF-8


You've done steps 1 and 2, 3 should be trivial and 4 will make the 
result compatible with 3.

-- Brane