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".)