Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enc=utf-8 時に外部コマンドに渡されるマルチバイト文字がutf-8である為、正しく実行されない。 #453

Closed
mattn opened this issue Aug 5, 2013 · 33 comments

Comments

@mattn
Copy link
Member

mattn commented Aug 5, 2013

ネタ元は

mattn/jvgrep#18

です。読んで頂ければだいたい分かると思いますが、utf-8 で引数を渡しているので DBCS の断片に判断された場合 ) が消えてエラーとなります。

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

解決方法としては、コマンドライン引数を GetACP() に合わせて DBCS に戻す方法ですが、どうでしょ?

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

または W 系関数を使うとか

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

https://gist.github.com/6153332

誰かレビューよろ

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

これは出来れば 7.4 までには入れたい

@Shougo
Copy link
Member

Shougo commented Aug 5, 2013

すみません、この文字列変換というのはどういうときの外部ファイルの実行に適用されるのでしょうか。
system()の動作に影響がないのなら良いのですが、system()の挙動が変わる場合、後方互換性が失われます。

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

引数のみ変換します。

そもそもACPがutf-8でないのにutf-8の引数を渡すこと自体が問題なので、互換性が壊れるというより元々壊れていたというのが正解です。

@koron
Copy link
Member

koron commented Aug 5, 2013

enc==ACPの時は未変換なのね。

CreateProcessのUTF-8対応 ついでに その他のencも対応した よ、
という位置付けなんですね。有りだと思います。

@k-takata
Copy link
Member

k-takata commented Aug 5, 2013

FEAT_MBYTE ではない場合は、#define _create_process(...) CreateProcess(...) の方が無駄なコードがなくなって良い?(どっちでもいいですが。)

あと、GUIではない場合の以下の部分も対処が必要ではないでしょうか。

# define mch_system(c, o) system(c)

@koron
Copy link
Member

koron commented Aug 5, 2013

メンテ性を考えれば今のほうが良いです。inline展開的な最適化はコンパイラの仕事ですし。

@Shougo
Copy link
Member

Shougo commented Aug 5, 2013

そもそもACPがutf-8でないのにutf-8の引数を渡すこと自体が問題なので、互換性が壊れるというより元々壊れていたというのが正解です。

だいたい分かりました。つまり、utf-8を渡そうとした時に変換をするんですね。
となると、よくあるコードでVim script側でutf-8 -> ACPに変換したときは動作が変わらないという認識で良いでしょうか?

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

となると、よくあるコードでVim script側でutf-8 -> ACPに変換したときは動作が変わらないという認識で良いでしょうか?

はい。逆に正しく動きます。

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

では送ります。

@k-takata
Copy link
Member

k-takata commented Aug 5, 2013

これは?

あと、GUIではない場合の以下の部分も対処が必要ではないでしょうか。

@Shougo
Copy link
Member

Shougo commented Aug 5, 2013

はい。逆に正しく動きます。

了解です。

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

これは?

おっと忘れてた

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

パッチ更新

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

@ghost ghost assigned mattn Aug 5, 2013
@k-takata
Copy link
Member

k-takata commented Aug 5, 2013

となると、よくあるコードでVim script側でutf-8 -> ACPに変換したときは動作が変わらないという認識で良いでしょうか?

はい。逆に正しく動きます。

そうなんでしょうか?
今までのバグった動作に対するワークアラウンドとして、enc=utf-8 のときにVim script側で ACP に変換して system() を呼んでいたということであれば、そのワークアラウンドを削除しないと、このパッチを当てたあとは正しく動かない気がしますが。

@Shougo
Copy link
Member

Shougo commented Aug 5, 2013

今までのバグった動作に対するワークアラウンドとして、enc=utf-8 のときにVim script側で ACP に変換して system() を呼んでいたということであれば、そのワークアラウンドを削除しないと、このパッチを当てたあとは正しく動かない気がしますが。

system()の実行にも影響があるので、コードを見る限りだと、たしかにそのとおりになりそうですね。
うーん、プラグインの検証の手間が大変になりそうな気がします。

@mattn
Copy link
Member Author

mattn commented Aug 5, 2013

そうなんでしょうか?
今までのバグった動作に対するワークアラウンドとして、enc=utf-8 のときにVim script側で ACP に変換して system() を呼んでいたということであれば、そのワークアラウンドを削除しないと、このパッチを当てたあとは正しく動かない気がしますが

それはありますね。

@Shougo
Copy link
Member

Shougo commented Aug 6, 2013

system()以外の外部コマンド実行に関しては、プラグイン側でWork around する余地が少ないので問題が発生しにくいですが、system()に関しては、プラグイン側でどれだけ対応が必要になるのか見積もったほうが良いように思います。

@mattn
Copy link
Member Author

mattn commented Aug 6, 2013

Thanks. I wonder why nobody reported a problem with this before. Just
not so many users in this situation or are they not using multi-byte
characters with external commands?

:vimgrep works properly, right?

なぜ今まで誰も報告してこなかったんだろう。マルチバイト文字を外部コマンドで使っているユーザがそんなに多くなかったから?

:vimgrep がいい感じ動くから?

という質問が来てるけど、僕は cp932 使ってるので分からない。誰か答えてあげて。

@mattn
Copy link
Member Author

mattn commented Aug 6, 2013

system()以外の外部コマンド実行に関しては、プラグイン側でWork around する余地が少ないので問題が発生しにくいですが、system()に関しては、プラグイン側でどれだけ対応が必要になるのか見積もったほうが良いように思います。

見積もるって僕が?
そもそも見積もっても仕方ないと思うけど。

@Shougo
Copy link
Member

Shougo commented Aug 6, 2013

なぜ今まで誰も報告してこなかったんだろう。マルチバイト文字を外部コマンドで使っているユーザがそんなに多くなかったから?

「Windows環境で、エンコーディングをutf-8にしていて、マルチバイト文字を外部コマンドに渡す」という再現条件が限定的だからでは?

@Shougo
Copy link
Member

Shougo commented Aug 6, 2013

#179

外部コマンドのエンコーディングの話題はここで以前議論されていたと思っていて、

無駄ではありません。外部コマンドの入出力のlocaleが何になるかについてVim本体は絶対にわかりません。あくまでもその外部コマンドを実行したプラグインで制御すべきことです。

自動変換してしまうと、プラグイン側で制御する余地がなくなるのではないでしょうか。

@k-takata
Copy link
Member

k-takata commented Aug 6, 2013

コマンドラインのエンコーディングと、標準入出力のエンコーディングは完全に分けて考える必要がありますが。

@k-takata
Copy link
Member

k-takata commented Aug 6, 2013

これ以外にも enc=utf-8 でうまく動かないバグがないかが気になるところです。(私も何件か直しましたが。)

@k-takata
Copy link
Member

これは出来れば 7.4 までには入れたい

残念! todo入りになりました。

Patch to make external commands work with multi-byte characters on Win32 when
'encoding' differs from the active codepage. (Yasuhiro Matsumoto, 2013 Aug 5)

@k-takata
Copy link
Member

7.4.122でマージされました。
https://groups.google.com/d/topic/vim_dev/KXzoTZz3-MY/discussion

@k-takata
Copy link
Member

VCでビルドしたらwarningが出ました。

os_win32.c(3842) : warning C4090: 'function' : different 'const' qualifiers
os_win32.c(3856) : warning C4133: 'function' : incompatible types - from 'STARTUPINFO *' to 'LPSTARTUPINFOW'
os_win32.c(3865) : warning C4090: 'function' : different 'const' qualifiers

修正パッチ送付済み。
https://groups.google.com/d/msg/vim_dev/KXzoTZz3-MY/f6bGV2tA7e4J

@k-takata
Copy link
Member

7.4.126で取り込まれました。
https://groups.google.com/d/topic/vim_dev/NDZKiIOR47U/discussion

@k-takata
Copy link
Member

https://groups.google.com/d/msg/vim_dev/KXzoTZz3-MY/aBCzYETlBQIJ
flags と inherit_handles が逆だとの指摘。

@k-takata
Copy link
Member

k-takata commented Jan 5, 2014

7.4.132で直りました。
https://groups.google.com/d/topic/vim_dev/o2zz22kRb58/discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants