From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.bugs Subject: bug#8439: [PATCH] ffap.el -- detect paths with spaces (v2) Date: Fri, 19 Oct 2012 21:44:02 -0400 Message-ID: References: <87pqoyaxu0.fsf@blue.sea.net> <87zk3i7tbu.fsf@picasso.cante.net> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1350697478 3051 80.91.229.3 (20 Oct 2012 01:44:38 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 20 Oct 2012 01:44:38 +0000 (UTC) Cc: 8439@debbugs.gnu.org To: Jari Aalto Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sat Oct 20 03:44:45 2012 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1TPO7G-0000CM-NH for geb-bug-gnu-emacs@m.gmane.org; Sat, 20 Oct 2012 03:44:42 +0200 Original-Received: from localhost ([::1]:51025 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TPO79-0000xE-CO for geb-bug-gnu-emacs@m.gmane.org; Fri, 19 Oct 2012 21:44:35 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:58016) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TPO76-0000x8-96 for bug-gnu-emacs@gnu.org; Fri, 19 Oct 2012 21:44:33 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TPO74-0005XS-Hp for bug-gnu-emacs@gnu.org; Fri, 19 Oct 2012 21:44:32 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:42188) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TPO74-0005XO-Ep for bug-gnu-emacs@gnu.org; Fri, 19 Oct 2012 21:44:30 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.72) (envelope-from ) id 1TPO8Y-0006qe-Fo for bug-gnu-emacs@gnu.org; Fri, 19 Oct 2012 21:46:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: debbugs-submit-bounces@debbugs.gnu.org Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 20 Oct 2012 01:46:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 8439 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 8439-submit@debbugs.gnu.org id=B8439.135069754326294 (code B ref 8439); Sat, 20 Oct 2012 01:46:02 +0000 Original-Received: (at 8439) by debbugs.gnu.org; 20 Oct 2012 01:45:43 +0000 Original-Received: from localhost ([127.0.0.1]:52439 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.72) (envelope-from ) id 1TPO8F-0006q3-EZ for submit@debbugs.gnu.org; Fri, 19 Oct 2012 21:45:43 -0400 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.182]:59010) by debbugs.gnu.org with esmtp (Exim 4.72) (envelope-from ) id 1TPO8D-0006pp-J9 for 8439@debbugs.gnu.org; Fri, 19 Oct 2012 21:45:42 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: Av0EAG6Zu09MCoeh/2dsb2JhbABEtBGBCIIWAQVWIxALNBIUGA0kiCG6CZBEA6MzgViDBQ X-IronPort-AV: E=Sophos;i="4.75,637,1330923600"; d="scan'208";a="202350689" Original-Received: from 76-10-135-161.dsl.teksavvy.com (HELO ceviche.home) ([76.10.135.161]) by ironport2-out.teksavvy.com with ESMTP/TLS/ADH-AES256-SHA; 19 Oct 2012 21:44:02 -0400 Original-Received: by ceviche.home (Postfix, from userid 20848) id 52F0E660E0; Fri, 19 Oct 2012 21:44:02 -0400 (EDT) In-Reply-To: <87zk3i7tbu.fsf@picasso.cante.net> (Jari Aalto's message of "Fri, 19 Oct 2012 11:35:17 +0300") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1.50 (gnu/linux) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.13 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 140.186.70.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:65770 Archived-At: 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.