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 <address@hidden> 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 <address@hidden>
> > > > Commit: Michael R. Mauger <address@hidden>
> > > >
> > > >       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