Re: [PATCH] svn_cmdline__edit_file_externally() may not be able to open the target file in locale other than UTF-8

On 2020/09/21 5:27, Stefan Sperling wrote:
> On Mon, Sep 21, 2020 at 01:18:54AM +0900, Yasuhito FUTATSUKI wrote:
>> On 2020/09/20 23:53, Yasuhito FUTATSUKI wrote:
>>> On 2020/09/20 23:41, Stefan Sperling wrote:
>>>> On Sun, Sep 20, 2020 at 11:06:21PM +0900, Yasuhito FUTATSUKI wrote:
>>>>> On 2020/09/19 5:26, Yasuhito FUTATSUKI wrote:
>>
>>>> It looks like apr_escape_shell() will escape 0x5c (backslash, \) when
>>>> looking for ASCII character bytes. When 0x5c is escaped this breaks the
>>>> SJIS-encoded byte sequence.
>>>
>>> Yes, it is right.
>>>
>>>> Have you already tried always using escape_path() on the UTF-8 version of
>>>> the string, and then converting the escaped path to the locale's encoding?
>>>> In other words: First use escape_path, then use svn_path_cstring_from_utf8?
>>>> Perhaps that will make SJIS work?
>>>
>>> Ah, I didn't make sense. I'll try and then post a new patch.
>>> Thank you very much!
>>
>> I've tried and it works on SJIS environment. Thanks.
>>
>> I attached an updated patch. (fix-edit-file-externally-patch-v2.txt)
>> [[[
>> Fix file name to edit from utf8 to local style.
>>
>> * subversion/libsvn_subr/cmdline.c (svn_cmdline__edit_file_externally):
>> Apply svn_path_cstring_from_utf8() to target file name after escape_shell().
>>
>> Suggested by: stsp (applying order of file name conversion)
>> ]]]
>>
>
> Yes, looks good to me. Thank you :)

I looked subversion/libsvn_subr/cmdline.c over again, then I was aware
that svn_cmdline__edit_string_externally() has also the issue of
applying order potentially.

Current use of svn_cmdline__edit_string_externally() is by
subversion/svn/util.c, subversion/svn/propedit-cmd.c, and
subversion/svnmucc/svnmucc.c. They pass "svn-commit", "svn-prop" and
"svnmucc-commit" for prefix(filename), and svn_io_open_uniquely_named
add only ".tmp" as suffix and numbers, so it is not need to apply
escape_path(). However, the description of
svn_cmdline__edit_string_externally in svn_cmdline_prvate.h does not
restrict character class for PREFIX, so I think it is good to fix
the issue.

So I updated the patch again. (fix-edit-file-externally-patch-v3.txt)

[[[
Fix file name to edit from utf8 to local style.

* subversion/libsvn_subr/cmdline.c
  (svn_cmdline__edit_file_externally):
  Apply svn_path_cstring_from_utf8() to target file name after escape_shell().
  (svn_cmdline__edit_string_externally):
  Fix order to apply svn_path_cstring_from_utf8() and escape_shell().

Suggested by: stsp (applying order of file name conversion)
]]]
 
>> Parhaps even with this patch, it may not work on Windows system other than
>> UTF-8 code page, because svn_path_cstring_from_utf8() returns just same
>> string as input and system() will interpret with system active code page.
>> (I don't know Windows so much, so I leave it.)
>
> I don't know Windows either.
>
> Perhaps someone else reading this could test the patch?

The patch itself affect nothing on Windows and macOS environment
because svn_path_cstring_from_utf8() returns just same string as a
input string. So if there is the same issue on Windows environment,
it is need another patch to fix it. So I'll commit this patch if
it is OK on Linux and Unix like OS.

My reproduction script was only for Linux/Unix, but it is not
difficult to reproduce the issue, just make textual context in file
which file name has different representation in UTF-8 and in system
active code page, and select resolution action 'edit'.
If the editor can open the file of right name, there is no problem,
else there is a problem.

Thanks,

-- 
Yasuhito FUTATSUKI <futatuki_at_yf.bsclub.org>