Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

[Ken Brown]

On 6/29/2015 8:59 PM, Michael Mauger wrote:
> branch: master
> commit 7466a4ded6ded0bea50151395b7a0fccc5dfd167
> Author: Michael R. Mauger <michael@mauger.com>
> Commit: Michael R. Mauger <michael@mauger.com>
>
>      Cygwin emacsclient handles w32 file names
> ---
>   lisp/server.el |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/lisp/server.el b/lisp/server.el
> index 2007635..ce19b3c 100644
> --- a/lisp/server.el
> +++ b/lisp/server.el
> @@ -1167,6 +1167,9 @@ The following commands are accepted by the client:
>                    (let ((file (pop args-left)))
>                      (if coding-system
>                          (setq file (decode-coding-string file coding-system)))
> +                   (when (and (eq system-type 'cygwin)
> +                              (fboundp 'cygwin-convert-file-name-from-windows))

There's no need for the 'fboundp ...' here; 
cygwin-convert-file-name-from-windows is defined in all Cygwin builds.

> +                     (setq file (cygwin-convert-file-name-from-windows file)))
>                      (setq file (expand-file-name file dir))
>                      (push (cons file filepos) files)
>                      (server-log (format "New file: %s %s"

Are you sure that emacsclient will still handle ordinary Cygwin file 
names properly after this change?  I'm concerned about file names that 
contain characters from the (default) UTF-8 character set.  I'm not very 
familiar with exactly how cygwin-convert-file-name-from-windows works, 
but its name suggests that it should be given a file name that's 
understood by Windows.

Ken

Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

[Stefan Monnier]

>> +                   (when (and (eq system-type 'cygwin)
>> +                              (fboundp 'cygwin-convert-file-name-from-windows))

> There's no need for the 'fboundp ...' here;
> cygwin-convert-file-name-from-windows is defined in all Cygwin builds.

I'm not arguing for or against this test, but I'm just pointing out that
the test should have the benefit of silencing the byte-compiler when
compiling on a non-Cygwin machine.


        Stefan

Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

[Ken Brown]

On 6/29/2015 10:40 PM, Ken Brown wrote:
> On 6/29/2015 8:59 PM, Michael Mauger wrote:
>> branch: master
>> commit 7466a4ded6ded0bea50151395b7a0fccc5dfd167
>> Author: Michael R. Mauger <michael@mauger.com>
>> Commit: Michael R. Mauger <michael@mauger.com>
>>
>>      Cygwin emacsclient handles w32 file names
>> ---
>>   lisp/server.el |    3 +++
>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/lisp/server.el b/lisp/server.el
>> index 2007635..ce19b3c 100644
>> --- a/lisp/server.el
>> +++ b/lisp/server.el
>> @@ -1167,6 +1167,9 @@ The following commands are accepted by the client:
>>                    (let ((file (pop args-left)))
>>                      (if coding-system
>>                          (setq file (decode-coding-string file
>> coding-system)))
>> +                   (when (and (eq system-type 'cygwin)
>> +                              (fboundp
>> 'cygwin-convert-file-name-from-windows))
>
> There's no need for the 'fboundp ...' here;
> cygwin-convert-file-name-from-windows is defined in all Cygwin builds.
>
>> +                     (setq file
>> (cygwin-convert-file-name-from-windows file)))
>>                      (setq file (expand-file-name file dir))
>>                      (push (cons file filepos) files)
>>                      (server-log (format "New file: %s %s"
>
> Are you sure that emacsclient will still handle ordinary Cygwin file
> names properly after this change?  I'm concerned about file names that
> contain characters from the (default) UTF-8 character set.  I'm not very
> familiar with exactly how cygwin-convert-file-name-from-windows works,
> but its name suggests that it should be given a file name that's
> understood by Windows.

I've tested this a little with file names containing UTF-8-encoded 
Chinese and other non-ASCII characters, and it appears to work OK.  But 
I *think* it only works because of accidental implementation details of 
cygwin-convert-file-name-from-windows (and the underlying Cygwin 
function cygwin_conv_path).  Basically, it seems that these functions 
don't actually try to do any conversion if they are given a multibyte 
string instead of the expected UTF-16 string.

So even though this change *might* be harmless, I think it could lead to 
bugs later if implementations change.  I don't think 
cygwin-convert-file-name-from-windows should be called on a file name 
that is not known to be a (UTF-16-encoded) Windows file name.  If you 
look at the (very few) places in the emacs code where that function is 
currently called, you'll see that the argument is indeed known to be a 
Windows file name.

Ken

Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

[Eli Zaretskii]

> Date: Wed, 01 Jul 2015 10:14:20 -0400
> From: Ken Brown <kbrown@cornell.edu>
> Cc: emacs-devel@gnu.org
> 
> I've tested this a little with file names containing UTF-8-encoded 
> Chinese and other non-ASCII characters, and it appears to work OK.  But 
> I *think* it only works because of accidental implementation details of 
> cygwin-convert-file-name-from-windows (and the underlying Cygwin 
> function cygwin_conv_path).  Basically, it seems that these functions 
> don't actually try to do any conversion if they are given a multibyte 
> string instead of the expected UTF-16 string.

That was also my conclusion.

> So even though this change *might* be harmless, I think it could lead to 
> bugs later if implementations change.  I don't think 
> cygwin-convert-file-name-from-windows should be called on a file name 
> that is not known to be a (UTF-16-encoded) Windows file name.

The UTF-16 encoding is not an issue, because
cygwin-convert-file-name-from-windows calls to_unicode to ensure that.

I think this code needs to look at the result of expand-file-name for
the file and the default directory in effect, and check whether the
result begins with a drive letter.  If it does, it should call
cygwin-convert-file-name-from-windows.

Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

[Michael Mauger]

On Wednesday, July 1, 2015 10:14 AM, Ken Brown <kbrown@cornell.edu> wrte:

>
>
>On 6/29/2015 10:40 PM, Ken Brown wrote:
>> On 6/29/2015 8:59 PM, Michael Mauger wrote:
>>> branch: master
>>> commit 7466a4ded6ded0bea50151395b7a0fccc5dfd167
>>> Author: Michael R. Mauger <michael@mauger.com>
>>> Commit: Michael R. Mauger <michael@mauger.com>
>>>
>>>      Cygwin emacsclient handles w32 file names
>>> ---
>>>   lisp/server.el |    3 +++
>>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/lisp/server.el b/lisp/server.el
>>> index 2007635..ce19b3c 100644
>>> --- a/lisp/server.el
>>> +++ b/lisp/server.el
>>> @@ -1167,6 +1167,9 @@ The following commands are accepted by the client:
>>>                    (let ((file (pop args-left)))
>>>                      (if coding-system
>>>                          (setq file (decode-coding-string file
>>> coding-system)))
>>> +                   (when (and (eq system-type 'cygwin)
>>> +                              (fboundp
>>> 'cygwin-convert-file-name-from-windows))
>>
>> There's no need for the 'fboundp ...' here;
>> cygwin-convert-file-name-from-windows is defined in all Cygwin builds.
>>
>>> +                     (setq file
>>> (cygwin-convert-file-name-from-windows file)))
>>>                      (setq file (expand-file-name file dir))
>>>                      (push (cons file filepos) files)
>>>                      (server-log (format "New file: %s %s"
>>
>> Are you sure that emacsclient will still handle ordinary Cygwin file
>> names properly after this change?  I'm concerned about file names that
>> contain characters from the (default) UTF-8 character set.  I'm not very
>> familiar with exactly how cygwin-convert-file-name-from-windows works,
>> but its name suggests that it should be given a file name that's
>> understood by Windows.
>
>I've tested this a little with file names containing UTF-8-encoded 
>Chinese and other non-ASCII characters, and it appears to work OK.  But 
>I *think* it only works because of accidental implementation details of 
>cygwin-convert-file-name-from-windows (and the underlying Cygwin 
>function cygwin_conv_path).  Basically, it seems that these functions 
>don't actually try to do any conversion if they are given a multibyte 
>string instead of the expected UTF-16 string.
>
>So even though this change *might* be harmless, I think it could lead to 
>bugs later if implementations change.  I don't think 
>cygwin-convert-file-name-from-windows should be called on a file name 
>that is not known to be a (UTF-16-encoded) Windows file name.  If you 
>look at the (very few) places in the emacs code where that function is 
>currently called, you'll see that the argument is indeed known to be a 
>Windows file name.
>
>Ken
>


While I think there may be legit concerns about the character encoding, 

the entire Cygwin environment is susceptible to such problems so I do 

not think it is a risky new exposure.  What this enables is to use the
cygwin'ified emacsclient to be used as a file handler under MSWindows.  

MSWindows passes the full file path to the emacsclient process and this
will translate the file name to the equivalent cygwin path. Passing a 

cygwin file name through this function seems to return the file name 

unmolested so it doesn't require a lot of guarding for file name syntax
before calling it (But I will defer to Ken who knows the internal 

workings of cygwin far better than I).

I use the above version of the patch on both cygwin and GNU/Linux ports
of Emacs daily.

Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

[Ken Brown]

On 7/1/2015 10:28 PM, Michael Mauger wrote:
>
> On Wednesday, July 1, 2015 10:14 AM, Ken Brown <kbrown@cornell.edu> wrte:
>
>>
>>
>> On 6/29/2015 10:40 PM, Ken Brown wrote:
>>> On 6/29/2015 8:59 PM, Michael Mauger wrote:
>>>> branch: master
>>>> commit 7466a4ded6ded0bea50151395b7a0fccc5dfd167
>>>> Author: Michael R. Mauger <michael@mauger.com>
>>>> Commit: Michael R. Mauger <michael@mauger.com>
>>>>
>>>>       Cygwin emacsclient handles w32 file names
>>>> ---
>>>>    lisp/server.el |    3 +++
>>>>    1 files changed, 3 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/lisp/server.el b/lisp/server.el
>>>> index 2007635..ce19b3c 100644
>>>> --- a/lisp/server.el
>>>> +++ b/lisp/server.el
>>>> @@ -1167,6 +1167,9 @@ The following commands are accepted by the client:
>>>>                     (let ((file (pop args-left)))
>>>>                       (if coding-system
>>>>                           (setq file (decode-coding-string file
>>>> coding-system)))
>>>> +                   (when (and (eq system-type 'cygwin)
>>>> +                              (fboundp
>>>> 'cygwin-convert-file-name-from-windows))
>>>
>>> There's no need for the 'fboundp ...' here;
>>> cygwin-convert-file-name-from-windows is defined in all Cygwin builds.
>>>
>>>> +                     (setq file
>>>> (cygwin-convert-file-name-from-windows file)))
>>>>                       (setq file (expand-file-name file dir))
>>>>                       (push (cons file filepos) files)
>>>>                       (server-log (format "New file: %s %s"
>>>
>>> Are you sure that emacsclient will still handle ordinary Cygwin file
>>> names properly after this change?  I'm concerned about file names that
>>> contain characters from the (default) UTF-8 character set.  I'm not very
>>> familiar with exactly how cygwin-convert-file-name-from-windows works,
>>> but its name suggests that it should be given a file name that's
>>> understood by Windows.
>>
>> I've tested this a little with file names containing UTF-8-encoded
>> Chinese and other non-ASCII characters, and it appears to work OK.  But
>> I *think* it only works because of accidental implementation details of
>> cygwin-convert-file-name-from-windows (and the underlying Cygwin
>> function cygwin_conv_path).  Basically, it seems that these functions
>> don't actually try to do any conversion if they are given a multibyte
>> string instead of the expected UTF-16 string.
>>
>> So even though this change *might* be harmless, I think it could lead to
>> bugs later if implementations change.  I don't think
>> cygwin-convert-file-name-from-windows should be called on a file name
>> that is not known to be a (UTF-16-encoded) Windows file name.  If you
>> look at the (very few) places in the emacs code where that function is
>> currently called, you'll see that the argument is indeed known to be a
>> Windows file name.
>>
>> Ken
>>
>
>
> While I think there may be legit concerns about the character encoding,
>
> the entire Cygwin environment is susceptible to such problems so I do
>
> not think it is a risky new exposure.  What this enables is to use the
> cygwin'ified emacsclient to be used as a file handler under MSWindows.
>
> MSWindows passes the full file path to the emacsclient process and this
> will translate the file name to the equivalent cygwin path. Passing a
>
> cygwin file name through this function seems to return the file name
>
> unmolested so it doesn't require a lot of guarding for file name syntax
> before calling it (But I will defer to Ken who knows the internal
>
> workings of cygwin far better than I).

It's not a question of knowing the internal workings of Cygwin.  My point is 
that the code should be made to be clearly correct, without any knowledge of the 
internals of Cygwin and without any undocumented knowledge of how 
cygwin-convert-file-name-from-windows is implemented.  Even though the latter 
appears to return a Cygwin file name unmolested, its documentation says only 
"Convert a Windows-style file name FILE to a Cygwin file name".

So I would be much more comfortable with this change if you added a check for a 
file name starting with a drive letter.  And it might also be good to add a 
comment explaining the intended use, in which Windows passes a full file path to 
emacsclient.

Ken

Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

[Ken Brown]

On 7/2/2015 3:48 PM, Ken Brown wrote:
> So I would be much more comfortable with this change if you added a check for a
> file name starting with a drive letter.  And it might also be good to add a
> comment explaining the intended use, in which Windows passes a full file path to
> emacsclient.

Something like the following is what I had in mind:

diff --git a/lisp/server.el b/lisp/server.el
index ce19b3c..46687a3 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1167,8 +1167,11 @@ The following commands are accepted by the client:
                   (let ((file (pop args-left)))
                     (if coding-system
                         (setq file (decode-coding-string file coding-system)))
+                   ;; Allow Cygwin's emacsclient to be used as a file
+                   ;; handler on MS-Windows, in which case FILENAME
+                   ;; might start with a drive letter.
                     (when (and (eq system-type 'cygwin)
-                              (fboundp 'cygwin-convert-file-name-from-windows))
+                              (string-match "^[A-Za-z]:" file))
                       (setq file (cygwin-convert-file-name-from-windows file)))
                     (setq file (expand-file-name file dir))
                     (push (cons file filepos) files)

Ken

Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

[Stefan Monnier]

> +                              (string-match "^[A-Za-z]:" file))
                                               ^^^
You mean                                       \\`


        Stefan

Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

[Ken Brown]

On 7/4/2015 2:29 PM, Stefan Monnier wrote:
>> +                              (string-match "^[A-Za-z]:" file))
>                                                 ^^^
> You mean                                       \\`

Now that I've re-read the documentation about regexps, yes, that's what I mean. 
  Somehow I never noticed the difference between ^ and \` before.  Thanks.

Ken

Re: [Emacs-diffs] master 7466a4d: Cygwin emacsclient handles w32 file names

[Ken Brown]

On 7/4/2015 11:20 AM, Ken Brown wrote:
> diff --git a/lisp/server.el b/lisp/server.el
> index ce19b3c..46687a3 100644
> --- a/lisp/server.el
> +++ b/lisp/server.el
> @@ -1167,8 +1167,11 @@ The following commands are accepted by the client:
>                    (let ((file (pop args-left)))
>                      (if coding-system
>                          (setq file (decode-coding-string file
> coding-system)))
> +                   ;; Allow Cygwin's emacsclient to be used as a file
> +                   ;; handler on MS-Windows, in which case FILENAME
> +                   ;; might start with a drive letter.
>                      (when (and (eq system-type 'cygwin)
> -                              (fboundp
> 'cygwin-convert-file-name-from-windows))
> +                              (string-match "^[A-Za-z]:" file))
>                        (setq file (cygwin-convert-file-name-from-windows
> file)))
>                      (setq file (expand-file-name file dir))
>                      (push (cons file filepos) files)

I've pushed this change (with Stefan's correction) as git commit b25770a.

Ken