bug#8439: [PATCH] ffap.el -- detect paths with spaces (v2)

[Daniel Colascione]

On 10/19/2012 6:44 PM, Stefan Monnier wrote:
> A few comments about your patch below.
> 
> 
>         Stefan
> 
> 
>> +  (let* ((cygwin-p (string-match "cygwin" (emacs-version)))
> 
> Test system-type instead.
> 
>> +           (if (and ffap-paths-with-spaces
>> +                    (memq mode '(nil file)))
>> +               (if (string-match "^[ \t]*$"
>> +                                 (buffer-substring (line-beginning-position)
>> +                                                   (point)))
> 
> Better match the buffer (e.g. with forward-line + looking-at, or in this
> case with skip-chars-backward + bolp) than extract a part of the buffer
> and then apply string-match to it.
> 
>> +                   ;; Nothing interesting before point. Move to the first 
>> character
> 
> Please terminate your comments with proper punctuation.
> 
>> +                   (skip-chars-forward " \t" (line-end-position))
>> +                 ;; If at colon, move a little forward so that next
>> +                 ;; `re-search-backward' can position at drive letter.
>> +                 (if (looking-at ":/")
>> +                     (forward-char 1))
>> +                 ;; Skip until drive path start or patch start letter
>> +                 (while (re-search-backward "[a-zA-Z]:[\\\\/]\\|[/\\\\]"
>> +                                                 (line-beginning-position) 
>> t)
>> +                   (goto-char (match-beginning 0)))))
> 
> This code seems to assume Windows-style file names, whereas none of the
> var names nor comments mention anything about this assumption.
> 
> I'm not sure we should make this assumption, but it's probably OK to
> consider that spaces only appear in Windows-style file names.  Just make
> this reasoning explicit in a comment.
> 
>> +           (when (and ffap-paths-with-spaces
>> +                      (memq mode '(nil file)))
>> +             ;; Paths may contains spaces, allow those
>> +             (if (looking-at
>> +                  
>> "[^\t\r\n]*[/\\\\][^][<>()\"';:|\t\r\n]*[^][<>()\"';:|\r\n[:space:]]")
>> +                 (setq space-p (match-end 0))))
> 
> This regexp needs some explanation.  Also I don't like the control&data
> flow very much, here, so you need to compensate this complexity by
> explaining clearly what this `space-p' is supposed to represent.
> Same about `end'.
> 
>> +                  (setq end (point))
>> +                         (if (and space-p
>> +                                  (> space-p end)
>> +                                  (memq mode '(file nil)))
>> +                                (setq end space-p))
> 
> As mentioned, the control&data flow is pretty ugly, but even then
> I think the (memq mode '(file nil)) is redundant because space-p can
> only be non-nil if (memq mode '(file nil)) is non-nil.
> 
>> +    ;; Under Cygwin, convert drive letters in paths.
>> +    (when (and cygwin-p
>> +               (memq mode '(nil file))
>> +               (string-match "^\\([a-zA-Z]\\):[/\\\\]\\(.*\\)" str))
>> +      (let ((drive (downcase (match-string 1 str)))
>> +            (path (match-string 2 str)))
>> +        (setq str (format "/cygdrive/%s/%s"
>> +                          drive
>> +                          (replace-regexp-in-string "[\\\\]" "/" path)))))
> 
> This shouldn't be here: the right way to do it  is to make the Cygwin
> Emacs accept Windows-style file name.

Stefan is right. But if you do want to convert Cygwin paths in some other
context, please use cygwin-convert-path-from-windows and
cygwin-convert-path-to-windows instead of hard-coding a particular mapping. (Not
everyone's Cygwin drive prefix is "/cygdrive".)

signature.asc
Description: OpenPGP digital signature