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

[Stefan Monnier]

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.