From: Daniel Colascione <dancol@dancol.org>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: 8439@debbugs.gnu.org, Jari Aalto <jari.aalto@cante.net>
Subject: bug#8439: [PATCH] ffap.el -- detect paths with spaces (v2)
Date: Fri, 19 Oct 2012 18:49:10 -0700 [thread overview]
Message-ID: <50820316.1030402@dancol.org> (raw)
In-Reply-To: <jwvzk3ihr2i.fsf-monnier+emacs@gnu.org>
[-- Attachment #1: Type: text/plain, Size: 3362 bytes --]
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".)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 256 bytes --]
next prev parent reply other threads:[~2012-10-20 1:49 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-07 15:24 bug#8439: [PATCH] ffap.el -- detect paths with spaces Jari Aalto
2011-07-03 23:29 ` Lars Magne Ingebrigtsen
2012-10-19 7:39 ` bug#8439: [PATCH] ffap.el -- detect paths with spaces (v2) Jari Aalto
2012-10-19 9:49 ` Eli Zaretskii
2012-10-19 10:05 ` jari
2012-10-19 11:15 ` Yigal Hochberg
2012-10-19 11:22 ` Eli Zaretskii
2012-10-19 12:15 ` Nicolas Richard
2012-10-19 21:47 ` Andrew W. Nosenko
2012-10-19 22:23 ` Eli Zaretskii
2012-10-19 22:36 ` Yigal Hochberg
2012-10-19 22:57 ` Andrew W. Nosenko
2012-10-19 23:24 ` Yigal Hochberg
2012-10-20 1:45 ` Stefan Monnier
2012-10-20 11:01 ` Juri Linkov
2012-10-19 8:35 ` Jari Aalto
2012-10-20 1:44 ` Stefan Monnier
2012-10-20 1:49 ` Daniel Colascione [this message]
2012-10-20 7:56 ` bug#8439: [PATCH] ffap.el -- detect paths with spaces (v3) jari
2012-10-20 8:45 ` bug#8439: [PATCH] ffap.el -- detect paths with spaces (v4) jari
2012-10-20 10:20 ` Eli Zaretskii
2012-10-21 0:18 ` Stefan Monnier
2012-10-21 8:07 ` Jari Aalto
2019-11-23 13:31 ` bug#6695: " Lars Ingebrigtsen
2020-08-14 13:08 ` Lars Ingebrigtsen
2020-08-15 9:07 ` Eli Zaretskii
2020-08-15 10:13 ` Lars Ingebrigtsen
2012-10-20 8:11 ` bug#8439: [PATCH] ffap.el -- detect paths with spaces (v2) Eli Zaretskii
2012-10-21 0:16 ` Stefan Monnier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50820316.1030402@dancol.org \
--to=dancol@dancol.org \
--cc=8439@debbugs.gnu.org \
--cc=jari.aalto@cante.net \
--cc=monnier@iro.umontreal.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).