On 2020/10/08 13:58, Yasuhito FUTATSUKI wrote:
> Hi,
>
> On 2020/10/08 13:12, Jun Omae wrote:
>> Hi all,
>>
>> On Sun, Oct 4, 2020 at 11:33 PM Yasuhito FUTATSUKI <futat...@yf.bsdclub.org>
>> wrote:
>>> I commited the v3 patch in 1882234.
>>>
>>> I confirmed that on Windows in CP932, both of with and without
>>> the v3 patch, it cannot pass the file name to edit.
>>>
>>> For Windows I confirmed that it can be fixed by using _wsystem()
>>> with WCHAR string, so I commited it in 1882235.
>>
>> I found a minor issue on Windows after r1882235.
>>
>> When the editor command (via SVN_EDITOR environment and --editor-cmd option)
>> has
>> non-ascii characters, it is unable to launch the editor.
>>
>> I think we should convert the editor command to UTF-16 encoding before
>> passing it
>> to _wsystem(), otherwise r1882235 should be reverted.
>
> I confirmed it is a regression that introduced in r1882235. So I reverted it
> in 1882313.
>
> Then I'll try to resolve it by the way you suggest.
With the patch attached, I could confirm that I can invoke editor command
even if it contains non-ascii character and/or ascii space in path via
SVN_EDITOR environment variable, --editor-cmd option, and config file
setting (saved in active sytem code page). Especially with environment
variable, Unicode path which cannot represent with active code page can
be used. (I confirmed it on Windows, with VS2019 build).
log message:
[[[
Follow up to r1882234,r1882235,r1882313: Fix file name encoding issue
when invoking editor on Windows.
* subversion/libsvn_subr/cmdline.c
(): include apr_env.h for apr_env_get
(find_editor_binary):
- Change the encoding to set to EDITOR, from active code page to
UTF-8 on Windows.
- Transcode editor_cmd to UTF-8.
- Use apr_env_get() instead of getenv().
- Transcode config setting for SVN_CONFIG_OPTION_EDITOR_CMD.
- Add pool argument for transcoding and getting environment variables.
(svn_cmdline_edit_file_externally, svn_cmdline_edit_string_externally):
- Use _wsystem() instead of system() on Windows environment.
- Enclose whole command strings with double quotes[1] in _wsystem().
[1]
https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/cmd
Suggested by jun66j5 (editor return value in find_editor_binary)
]]]
However, I still have some considerations:
* Compatibility on use of _wsystem(). I used it for all Win32 environment,
but isn't it compatibility issue here ?
* Consistency. It seems we use active system code page as encoding of
native C string, but I use UTF-8 not to break file path.
* There is no way to pass those paths which cannot be represented in
active codepage, from command line. It is because we use main()
for program entry point, and its argv is transcoded to active code page
by C run time library before main() is called. If we use _wmain(),
we can obtain native UTF-16 argv, or if we use APR's libaprapp,
we can use argv, transcoded from UTF-16 to UTF-8.
* Embeding native C string in error message. If system() or _wsystem()
causes error, we make error message from "cmd" argument passed to
system without transcode, and then we'll see the message that
incorrectly transcoded as UTF-8 to native C encoding.
Thanks,
--
Yasuhito FUTATSUKI <futat...@yf.bsclub.org>
Index: subversion/libsvn_subr/cmdline.c
===================================================================
--- subversion/libsvn_subr/cmdline.c (revision 1882313)
+++ subversion/libsvn_subr/cmdline.c (working copy)
@@ -47,6 +47,7 @@
#endif
#include <apr_general.h> /* for apr_initialize/apr_terminate */
#include <apr_strings.h> /* for apr_snprintf */
+#include <apr_env.h> /* for apr_env_get */
#include <apr_pools.h>
#include <apr_signal.h>
@@ -1240,38 +1250,84 @@
/* Helper for the edit_externally functions. Set *EDITOR to some path to an
- editor binary. Sources to search include: the EDITOR_CMD argument
- (if not NULL), $SVN_EDITOR, the runtime CONFIG variable (if CONFIG
+ editor binary, in native C string on Unix/Linux platforms and in UTF-8
+ string in Windows platform. Sources to search include: the EDITOR_CMD
+ argument (if not NULL), $SVN_EDITOR, the runtime CONFIG variable (if CONFIG
is not NULL), $VISUAL, $EDITOR. Return
SVN_ERR_CL_NO_EXTERNAL_EDITOR if no binary can be found. */
static svn_error_t *
find_editor_binary(const char **editor,
const char *editor_cmd,
- apr_hash_t *config)
+ apr_hash_t *config,
+ apr_pool_t *pool)
{
const char *e;
+ const char *e_cfg;
struct svn_config_t *cfg;
+ apr_status_t status;
/* Use the editor specified on the command line via --editor-cmd, if any. */
+#ifdef WIN32
+ /* On Windows, editor_cmd is transcoded to the system active code page
+ because we use main() as a entry point without APR's (or our own) wrapper
+ in command line tools. */
+ if (editor_cmd)
+ {
+ SVN_ERR(svn_utf_cstring_to_utf8(&e, editor_cmd, pool));
+ }
+ else
+ {
+ e = NULL;
+ }
+#else
e = editor_cmd;
+#endif
/* Otherwise look for the Subversion-specific environment variable. */
if (! e)
- e = getenv("SVN_EDITOR");
+ {
+ status = apr_env_get((char **)&e, "SVN_EDITOR", pool);
+ if (status || ! *e)
+ {
+ e = NULL;
+ }
+ }
/* If not found then fall back on the config file. */
if (! e)
{
cfg = config ? svn_hash_gets(config, SVN_CONFIG_CATEGORY_CONFIG) : NULL;
- svn_config_get(cfg, &e, SVN_CONFIG_SECTION_HELPERS,
+ svn_config_get(cfg, &e_cfg, SVN_CONFIG_SECTION_HELPERS,
SVN_CONFIG_OPTION_EDITOR_CMD, NULL);
+#ifdef WIN32
+ if (e_cfg)
+ {
+ /* On Windows, we assume that config values are set in system active
+ code page, so we need transcode it here. */
+ SVN_ERR(svn_utf_cstring_to_utf8(&e, e_cfg, pool));
+ }
+#else
+ e = e_cfg;
+#endif
}
/* If not found yet then try general purpose environment variables. */
if (! e)
- e = getenv("VISUAL");
+ {
+ status = apr_env_get((char**)&e, "VISUAL", pool);
+ if (status || ! *e)
+ {
+ e = NULL;
+ }
+ }
if (! e)
- e = getenv("EDITOR");
+ {
+ status = apr_env_get((char**)&e, "EDITOR", pool);
+ if (status || ! *e)
+ {
+ e = NULL;
+ }
+ }
#ifdef SVN_CLIENT_EDITOR
/* If still not found then fall back on the hard-coded default. */
@@ -1406,6 +1462,9 @@
{
const char *editor, *cmd, *base_dir, *file_name, *base_dir_apr;
const char *file_name_local;
+#ifdef WIN32
+ const WCHAR *wcmd;
+#endif
char *old_cwd;
int sys_err;
apr_status_t apr_err;
@@ -1412,7 +1471,7 @@
svn_dirent_split(&base_dir, &file_name, path, pool);
- SVN_ERR(find_editor_binary(&editor, editor_cmd, config));
+ SVN_ERR(find_editor_binary(&editor, editor_cmd, config, pool));
apr_err = apr_filepath_get(&old_cwd, APR_FILEPATH_NATIVE, pool);
if (apr_err)
@@ -1433,8 +1492,14 @@
escape_path(pool, file_name), pool));
/* editor is explicitly documented as being interpreted by the user's shell,
and as such should already be quoted/escaped as needed. */
+#ifndef WIN32
cmd = apr_psprintf(pool, "%s %s", editor, file_name_local);
sys_err = system(cmd);
+#else
+ cmd = apr_psprintf(pool, "\"%s %s\"", editor, file_name_local);
+ SVN_ERR(svn_utf__win32_utf8_to_utf16(&wcmd, cmd, NULL, pool));
+ sys_err = _wsystem(wcmd);
+#endif
apr_err = apr_filepath_set(old_cwd, pool);
if (apr_err)
@@ -1466,6 +1531,9 @@
{
const char *editor;
const char *cmd;
+#ifdef WIN32
+ const WCHAR *wcmd;
+#endif
apr_file_t *tmp_file;
const char *tmpfile_name;
const char *tmpfile_native;
@@ -1479,7 +1547,7 @@
int sys_err;
svn_boolean_t remove_file = TRUE;
- SVN_ERR(find_editor_binary(&editor, editor_cmd, config));
+ SVN_ERR(find_editor_binary(&editor, editor_cmd, config, pool));
/* Convert file contents from UTF-8/LF if desired. */
if (as_text)
@@ -1596,7 +1664,11 @@
/* editor is explicitly documented as being interpreted by the user's shell,
and as such should already be quoted/escaped as needed. */
+#ifndef WIN32
cmd = apr_psprintf(pool, "%s %s", editor, tmpfile_native);
+#else
+ cmd = apr_psprintf(pool, "\"%s %s\"", editor, tmpfile_native);
+#endif
/* If the caller wants us to leave the file around, return the path
of the file we'll use, and make a note not to destroy it. */
@@ -1607,7 +1679,12 @@
}
/* Now, run the editor command line. */
+#ifndef WIN32
sys_err = system(cmd);
+#else
+ SVN_ERR(svn_utf__win32_utf8_to_utf16(&wcmd, cmd, NULL, pool));
+ sys_err = _wsystem(wcmd);
+#endif
if (sys_err != 0)
{
/* Extracting any meaning from sys_err is platform specific, so just
