bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 4509 bytes --]

Hi all,

I lived with this bug for a long time and then I lived with a patched
ffap-string-at-point for a long time :)

The bug is that if

(1) a major mode uses "//" as comment start chars, and
(2) ido-mode is enabled, and
(3) ffap feature is enabled, and
(4) user does C-x C-f on a comment like "//foo"

attempt is made to access "//foo" path.

It's a very corner case bug, but I hit is very often as I work most of the
time in a major mode that uses "//" to comment lines (verilog-mode).

Here is the recipe to reproduce this in emacs -Q:

(1) emacs -Q
(2) Evaluate:

(progn
  (c-mode)
  (insert "//This is a comment")
  (ido-mode)
  (setq ido-use-filename-at-point 'guess)
  (re-search-backward "//" (line-beginning-position)))

(3) Hit "C-x C-f"

You will see that emacs is trying to look for a path "//This"!

This must not happen.

The proposed fix adds a little intelligence to skip the comment start chars
in the string stored to ffap-string-at-point var.

Here is the proposed patch generated ignoring white space. I will post a
proper git formatted patch in a follow up email.

=====

diff --git a/lisp/ffap.el b/lisp/ffap.el
index 7013e6e..ba62012 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -1097,29 +1097,50 @@ ffap-string-at-point

 (defun ffap-string-at-point (&optional mode)
   "Return a string of characters from around point.
+
 MODE (defaults to value of `major-mode') is a symbol used to look up
 string syntax parameters in `ffap-string-at-point-mode-alist'.
+
 If MODE is not found, we use `file' instead of MODE.
+
 If the region is active, return a string from the region.
-Sets the variable `ffap-string-at-point' and the variable
-`ffap-string-at-point-region'."
+
+If the point is in a comment, ensure that the returned string does not
contain
+the comment start characters (especially for major modes that have '//' as
+comment start characters).
+
+Sets variables `ffap-string-at-point' and `ffap-string-at-point-region'. "
   (let* ((args
           (cdr
            (or (assq (or mode major-mode) ffap-string-at-point-mode-alist)
                (assq 'file ffap-string-at-point-mode-alist))))
+         (region-selected (use-region-p))
          (pt (point))
- (beg (if (use-region-p)
+         (beg (if region-selected
                   (region-beginning)
                 (save-excursion
                   (skip-chars-backward (car args))
                   (skip-chars-forward (nth 1 args) pt)
                   (point))))
- (end (if (use-region-p)
+         ;; If point is in a comment like "//abc" (in `c-mode'), and a
+         ;; region is not selected, return the position of 'a'.
+         (comment-start-pos (unless region-selected
+                              (save-excursion
+                                (goto-char beg)
+                                (comment-search-forward
+                                 (line-end-position) :noerror)
+                                (point))))
+         (end (if region-selected
                   (region-end)
                 (save-excursion
                   (skip-chars-forward (car args))
                   (skip-chars-backward (nth 2 args) pt)
                   (point)))))
+    (when (and comment-start-pos
+               (> end comment-start-pos))
+      (setq beg comment-start-pos))
+    ;; (message "comment-start-pos = %d end = %d beg = %d"
+    ;;          comment-start-pos end beg)
     (setq ffap-string-at-point
           (buffer-substring-no-properties
            (setcar ffap-string-at-point-region beg)

=====


In GNU Emacs 25.1.50.10 (x86_64-unknown-linux-gnu, GTK+ Version 2.24.23)
 of 2016-07-22 built on ...
Repository revision: 03f32876210f3dd68c71baa210e523c3b7581758
Windowing system distributor 'The X.Org Foundation', version 11.0.60900000
System Description: Red Hat Enterprise Linux Workstation release 6.6
(Santiago)

Configured using:
 'configure --with-modules
 --prefix=/home/kmodi/usr_local/apps/6/emacs/master
 'CPPFLAGS=-fgnu89-inline -I/home/kmodi/usr_local/6/include
 -I/usr/include/freetype2 -I/usr/include' 'CFLAGS=-ggdb3 -O0'
 'CXXFLAGS=-ggdb3 -O0' 'LDFLAGS=-L/home/kmodi/usr_local/6/lib
 -L/home/kmodi/usr_local/6/lib64 -ggdb3''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK2 X11 MODULES

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=none
  locale-coding-system: utf-8-unix

-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 6710 bytes --]

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 4041 bytes --]

I am copying the last 3 committers on this section of the code to review
this patch.

Patch follows:

From a9e94ba5789d2b1d524120efecd3f9ab9af641cc Mon Sep 17 00:00:00 2001
From: Kaushal Modi <kaushal.modi@gmail.com>
Date: Fri, 22 Jul 2016 18:58:29 -0400
Subject: [PATCH] Do not include comment start chars in ffap string

* lisp/ffap.el (ffap-string-at-point): If the point is in a comment,
ensure that the returned string does not contain the comment start
characters (especially for major modes that have '//' as comment start
characters).

Otherwise, in a major mode like c-mode, with `ido-mode' enabled and
`ido-use-filename-at-point' set to `guess', doing "C-x C-f" on a "//foo"
comment will initiate an attempt to access a path "//foo" (Bug#24057).
---
 lisp/ffap.el | 63
++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 21 deletions(-)

diff --git a/lisp/ffap.el b/lisp/ffap.el
index 7013e6e..ba62012 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -1097,33 +1097,54 @@ ffap-string-at-point

 (defun ffap-string-at-point (&optional mode)
   "Return a string of characters from around point.
+
 MODE (defaults to value of `major-mode') is a symbol used to look up
 string syntax parameters in `ffap-string-at-point-mode-alist'.
+
 If MODE is not found, we use `file' instead of MODE.
+
 If the region is active, return a string from the region.
-Sets the variable `ffap-string-at-point' and the variable
-`ffap-string-at-point-region'."
+
+If the point is in a comment, ensure that the returned string does not
contain
+the comment start characters (especially for major modes that have '//' as
+comment start characters).
+
+Sets variables `ffap-string-at-point' and `ffap-string-at-point-region'. "
   (let* ((args
-  (cdr
-   (or (assq (or mode major-mode) ffap-string-at-point-mode-alist)
-       (assq 'file ffap-string-at-point-mode-alist))))
- (pt (point))
- (beg (if (use-region-p)
-  (region-beginning)
- (save-excursion
-  (skip-chars-backward (car args))
-  (skip-chars-forward (nth 1 args) pt)
-  (point))))
- (end (if (use-region-p)
-  (region-end)
- (save-excursion
-  (skip-chars-forward (car args))
-  (skip-chars-backward (nth 2 args) pt)
-  (point)))))
+          (cdr
+           (or (assq (or mode major-mode) ffap-string-at-point-mode-alist)
+               (assq 'file ffap-string-at-point-mode-alist))))
+         (region-selected (use-region-p))
+         (pt (point))
+         (beg (if region-selected
+                  (region-beginning)
+                (save-excursion
+                  (skip-chars-backward (car args))
+                  (skip-chars-forward (nth 1 args) pt)
+                  (point))))
+         ;; If point is in a comment like "//abc" (in `c-mode'), and a
+         ;; region is not selected, return the position of 'a'.
+         (comment-start-pos (unless region-selected
+                              (save-excursion
+                                (goto-char beg)
+                                (comment-search-forward
+                                 (line-end-position) :noerror)
+                                (point))))
+         (end (if region-selected
+                  (region-end)
+                (save-excursion
+                  (skip-chars-forward (car args))
+                  (skip-chars-backward (nth 2 args) pt)
+                  (point)))))
+    (when (and comment-start-pos
+               (> end comment-start-pos))
+      (setq beg comment-start-pos))
+    ;; (message "comment-start-pos = %d end = %d beg = %d"
+    ;;          comment-start-pos end beg)
     (setq ffap-string-at-point
-  (buffer-substring-no-properties
-   (setcar ffap-string-at-point-region beg)
-   (setcar (cdr ffap-string-at-point-region) end)))))
+          (buffer-substring-no-properties
+           (setcar ffap-string-at-point-region beg)
+           (setcar (cdr ffap-string-at-point-region) end)))))

 (defun ffap-string-around ()
   ;; Sometimes useful to decide how to treat a string.
-- 
2.9.2

-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 6885 bytes --]

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[npostavs]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> Hi all,
>
> I lived with this bug for a long time and then I lived with a patched ffap-string-at-point for a long time :)
>
> The bug is that if 
>
> (1) a major mode uses "//" as comment start chars, and
> (2) ido-mode is enabled, and
> (3) ffap feature is enabled, and
> (4) user does C-x C-f on a comment like "//foo"
>
> attempt is made to access "//foo" path.

Is possibly a dup of #7229 and/or #8990?

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Date: Fri, 22 Jul 2016 21:26:45 -0400
> Cc: 24057@debbugs.gnu.org
> 
> Is possibly a dup of #7229 and/or #8990?

Yes, please merge all those.

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Fri, 22 Jul 2016 23:00:00 +0000
> 
> (1) a major mode uses "//" as comment start chars, and
> (2) ido-mode is enabled, and
> (3) ffap feature is enabled, and
> (4) user does C-x C-f on a comment like "//foo"
> 
> attempt is made to access "//foo" path.

I see no reason to assume that file names cannot appear in comments.

> (3) Hit "C-x C-f"
> 
> You will see that emacs is trying to look for a path "//This"!
> 
> This must not happen.

I could agree with a user option to disable this behavior, but
disabling it unconditionally is IMO a mistake.

Thanks.

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

On Sat, Jul 23, 2016 at 3:34 AM Eli Zaretskii <eliz@gnu.org> wrote:

> I see no reason to assume that file names cannot appear in comments.
>

I have already tested with these use cases and now it seems to
do-the-right-thing:

In the below table, 'x' represents the point (the cursor would be on the
character to its right).
The second column shows the value that ffap-string-at-point is set to on
doing C-x C-f (with the ido setup explained in the first email).


|-----------------------------------+---------------------------------|
| Example string in `c-mode' buffer | Returned `ffap-string-at-point' |
|-----------------------------------+---------------------------------|
| x//tmp                            | "tmp"                           |
| //xtmp                            | "tmp"                           |
| x////tmp                          | "tmp"                           |
| ////xtmp                          | "tmp"                           |
| x// //tmp                         | ""                              |
| // //xtmp                         | "//tmp"                         |
|-----------------------------------+---------------------------------|


To try this out, paste the below in a new buffer and M-x c-mode.

//tmp
////tmp
// //tmp

Now do C-x C-f with cursor at those 6 different points and you will see
that an attempt to read /tmp happens only in the case of "// //tmp" when
the point is anywhere in the "//tmp" portion following "// ".


I could agree with a user option to disable this behavior, but
> disabling it unconditionally is IMO a mistake.
>

Of course, if this patch returns in unintended ffap-string-at-point, I will
add a defcustom to enable this behavior.
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 3293 bytes --]

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Sat, 23 Jul 2016 11:56:40 +0000
> Cc: 24057@debbugs.gnu.org, Noam Postavsky <npostavs@users.sourceforge.net>
> 
>  I see no reason to assume that file names cannot appear in comments.
> 
> I have already tested with these use cases and now it seems to do-the-right-thing:
> 
> In the below table, 'x' represents the point (the cursor would be on the character to its right).
> The second column shows the value that ffap-string-at-point is set to on doing C-x C-f (with the ido setup
> explained in the first email).
> 
> |-----------------------------------+---------------------------------|
> | Example string in `c-mode' buffer | Returned `ffap-string-at-point' |
> |-----------------------------------+---------------------------------|
> | x//tmp | "tmp" |
> | //xtmp | "tmp" |
> | x////tmp | "tmp" |
> | ////xtmp | "tmp" |
> | x// //tmp | "" |
> | // //xtmp | "//tmp" |
> |-----------------------------------+---------------------------------|
> 
> To try this out, paste the below in a new buffer and M-x c-mode.
> 
> //tmp
> ////tmp
> // //tmp
> 
> Now do C-x C-f with cursor at those 6 different points and you will see that an attempt to read /tmp happens
> only in the case of "// //tmp" when the point is anywhere in the "//tmp" portion following "// ".

Did you try this on some system where "//tmp" is a valid file name,
which is different from "/tmp"?

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On Sat, Jul 23, 2016 at 9:05 AM Eli Zaretskii <eliz@gnu.org> wrote:

> Did you try this on some system where "//tmp" is a valid file name,
> which is different from "/tmp"?
>

I have never dealt with such files.

My brief attempt just to try creating such a file failed:

km²~/temp/:> touch '//abc'

touch: cannot touch `//abc': Permission denied
km²~/temp/:> touch '\/\/abc'

touch: cannot touch `\\/\\/abc': No such file or directory

Can you show how to recreate the issue that you are thinking?
Or can you try recreating the problem on your system with this patch?

Or, should this patch anyways go in the master, and we let the emacs-devel
give us feedback if this results in anything unexpected?

I have used this patch in my config for few years now without any issue.

-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 1594 bytes --]

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Sat, 23 Jul 2016 13:23:08 +0000
> Cc: 24057@debbugs.gnu.org, npostavs@users.sourceforge.net
> 
>  Did you try this on some system where "//tmp" is a valid file name,
>  which is different from "/tmp"?
> 
> I have never dealt with such files.
> 
> My brief attempt just to try creating such a file failed:
> 
> km²~/temp/:> touch '//abc' 
> touch: cannot touch `//abc': Permission denied
> km²~/temp/:> touch '\/\/abc' 
> touch: cannot touch `\\/\\/abc': No such file or directory
> 
> Can you show how to recreate the issue that you are thinking?

You need to be on a system that supports that.  Windows is one of
them; not sure if there are any others left nowadays.

> Or can you try recreating the problem on your system with this patch?

I didn't try.

> Or, should this patch anyways go in the master, and we let the emacs-devel give us feedback if this results in
> anything unexpected?

It should definitely go to master only.

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Noam Postavsky]

On Sat, Jul 23, 2016 at 9:59 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>>  Did you try this on some system where "//tmp" is a valid file name,
>>  which is different from "/tmp"?
> You need to be on a system that supports that.  Windows is one of
> them; not sure if there are any others left nowadays.

How does it work?

C:\Users\npostavs>echo foo > //tmp
The specified path is invalid.

In Emacs:

Saving file //tmp...
Directory `//' does not exist; create? (y or n) y
basic-save-buffer-2: //: no such directory

This is on a newly-upgraded-to-Windows-10 box, Emacs 24.5

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sat, 23 Jul 2016 14:02:36 -0400
> Cc: Kaushal Modi <kaushal.modi@gmail.com>, 24057@debbugs.gnu.org
> 
> On Sat, Jul 23, 2016 at 9:59 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> >>  Did you try this on some system where "//tmp" is a valid file name,
> >>  which is different from "/tmp"?
> > You need to be on a system that supports that.  Windows is one of
> > them; not sure if there are any others left nowadays.
> 
> How does it work?
> 
> C:\Users\npostavs>echo foo > //tmp
> The specified path is invalid.
> 
> In Emacs:
> 
> Saving file //tmp...
> Directory `//' does not exist; create? (y or n) y
> basic-save-buffer-2: //: no such directory
> 
> This is on a newly-upgraded-to-Windows-10 box, Emacs 24.5

It's a UNC notation.  Normally used for network shares.  If you want
to use this locally, you will need to create a share first.  Valid UNC
file/directory names look like //machine/share.

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sat, 23 Jul 2016 14:02:36 -0400
> Cc: Kaushal Modi <kaushal.modi@gmail.com>, 24057@debbugs.gnu.org
> 
> How does it work?
> 
> C:\Users\npostavs>echo foo > //tmp
> The specified path is invalid.

Oh, and of course you cannot use forward slashes in CMD, you need to
use backslashes instead, as in \\tmp\foo.

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

On Sat, Jul 23, 2016 at 9:59 AM Eli Zaretskii <eliz@gnu.org> wrote:

> You need to be on a system that supports that.  Windows is one of
> them; not sure if there are any others left nowadays.
>
> > Or can you try recreating the problem on your system with this patch?
>
> I didn't try.
>

Can you please try it out and see if this patch causes any issue with such
paths? I believe that if you have "// //share/foo", it should still work
fine.


> > Or, should this patch anyways go in the master, and we let the
> emacs-devel give us feedback if this results in
> > anything unexpected?
>
> It should definitely go to master only.
>

Correct, I meant to emphasize that the patch be committed without a
defcustom to begin with .. and then we can add the defcustom if needed.
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 1356 bytes --]

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Sat, 23 Jul 2016 21:51:47 +0000
> Cc: 24057@debbugs.gnu.org, npostavs@users.sourceforge.net
> 
>  You need to be on a system that supports that. Windows is one of
>  them; not sure if there are any others left nowadays.
> 
>  > Or can you try recreating the problem on your system with this patch?
> 
>  I didn't try.
> 
> Can you please try it out and see if this patch causes any issue with such paths? I believe that if you have "//
> //share/foo", it should still work fine.

I tried with a comment that begins like this:

  ////share/foo

and I don't think the proposed behavior will be correct in all cases.
The problem is that comment-search-forward moves all the way past the
leading 4 slashes, instead of only 2.  While I can understand why the
comment-start sequence should not be considered a potential file name,
the stuff that follows it -- in this case, //share/foo -- should IMO
be allowed to be a file name.

Thanks.

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 2930 bytes --]

On Sun, Jul 24, 2016 at 10:17 AM Eli Zaretskii <eliz@gnu.org> wrote:

> I tried with a comment that begins like this:
>
>   ////share/foo
>
> and I don't think the proposed behavior will be correct in all cases.
> The problem is that comment-search-forward moves all the way past the
> leading 4 slashes, instead of only 2.


That is expected, as I posted in a table earlier:

|-----------------------------------+---------------------------------|
| Example string in `c-mode' buffer | Returned `ffap-string-at-point' |
|-----------------------------------+---------------------------------|
| x//tmp                            | "tmp"                           |
| //xtmp                            | "tmp"                           |
| x////tmp                          | "tmp"                           |
| ////xtmp                          | "tmp"                           |
| x// //tmp                         | ""                              |
| // //xtmp                         | "//tmp"                         |
|-----------------------------------+---------------------------------|

The problem with that is .. what is a user has a decorative comment like:

///I would like to
///share foo

It is not possible to know if the user liked to use "//" or "///" or "////"
or .. as the comment prefix. Also it is not possible to know if the user
meant "/share" or "//share".

So the best way to make the user's intent clear is by preceding the path
with a space. I can also create a patch with this info in the NEWS.

"// /share" -> User means "/share"
"// //share" -> User means "//share"
"/// /share" -> User means "/share"
"/// //share" -> User means "//share"

"//share" -> User means "share"
"///share" -> User means "share"
"////share" -> User means "share"
"/////share" -> User means "share"

Also thinking that the user meant "//share" in "////share" does not make
much sense. It's very confusing to count the number of slashes in there.
What is the user has "/////share" (5 slashes)? Where would we want the
ffap-string-at-point to guess the comment-starter<>comment boundary then?
We would rather have the user put a space to make it less confusing to read
the comment and also much simpler to implement the ffap parsing and much
less error prone.

While I can understand why the
> comment-start sequence should not be considered a potential file name,
> the stuff that follows it -- in this case, //share/foo -- should IMO
> be allowed to be a file name.
>

With the current state of ffap-string-at-point, it creates erroneous
behavior for many people including me. But with the patch, if a user needs
to put a path like "//share" in a comment for a major mode using "//" as
comment prefix, all they need to do is use a space to separate the two.
Also I think that it is very unlikely that someone would have a confusing
comment like "////share" where the user meant "//share" (or "/share").
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 6082 bytes --]

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Mon, 25 Jul 2016 02:19:13 +0000
> Cc: 24057@debbugs.gnu.org, npostavs@users.sourceforge.net
> 
>     I tried with a comment that begins like this:
> 
>       ////share/foo
> 
>     and I don't think the proposed behavior will be correct in all cases.
>     The problem is that comment-search-forward moves all the way past the
>     leading 4 slashes, instead of only 2.  
> 
> That is expected, as I posted in a table earlier:

If I understood you correctly, the table you posted didn't cover
systems which support UNC file names, so you asked me to try on such a
system.  The above was my response to your request.

> The problem with that is .. what is a user has a decorative comment like:
> 
> ///I would like to
> ///share foo
> 
> It is not possible to know if the user liked to use "//" or "///" or "////" or .. as the comment prefix. Also it is not possible to know if the user meant "/share" or "//share".

IMO, it isn't Emacs's place to second-guess the user in a way that
prohibits valid use cases.

> So the best way to make the user's intent clear is by preceding the path with a space.

When there's whitespace between the comment leader and the rest, the
problem I'm talking about doesn't exist.

> Also thinking that the user meant "//share" in "////share" does not make much sense. It's very confusing to count the number of slashes in there. What is the user has "/////share" (5 slashes)? Where would we want the ffap-string-at-point to guess the comment-starter<>comment boundary then?

If you want to code a backward-compatible solution, then skipping the
comment-start regexp should do, I think.  Where it stops, ffap should
start looking for valid file names.

> With the current state of ffap-string-at-point, it creates erroneous behavior for many people including me. But with the patch, if a user needs to put a path like "//share" in a comment for a major mode using "//" as comment prefix, all they need to do is use a space to separate the two. Also I think that it is very unlikely that someone would have a confusing comment like "////share" where the user meant "//share" (or "/share").

I see your point.  My point is that when we introduce
backward-incompatible behavior, which makes previously valid use cases
invalid, such incompatible behavior should initially be opt-in, even
if it looks to you that the previous behavior made no sense.  I have
enough gray hair to testify how such decisions in the past turned out
to be annoyances for some users.  Later, usually years later, we could
collect user experience and decide to make this the default behavior.

So if you disagree with my suggestion to skip comment-start instead of
using comment-search-forward, the behavior you propose should IMO be
off by default.

Thanks.

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 3603 bytes --]

On Mon, Jul 25, 2016 at 1:03 PM Eli Zaretskii <eliz@gnu.org> wrote:

> If I understood you correctly, the table you posted didn't cover
> systems which support UNC file names, so you asked me to try on such a
> system.  The above was my response to your request.
>

Thanks. AFAIU, ffap-string-at-point basically deals with just parsing the
string and does not check if a path is valid. So the "////tmp" example in
the table should apply to any use case.


> > The problem with that is .. what is a user has a decorative comment like:
> >
> > ///I would like to
> > ///share foo
> >
> > It is not possible to know if the user liked to use "//" or "///" or
> "////" or .. as the comment prefix. Also it is not possible to know if the
> user meant "/share" or "//share".
>
> IMO, it isn't Emacs's place to second-guess the user in a way that
> prohibits valid use cases.
>

OK.


> > So the best way to make the user's intent clear is by preceding the path
> with a space.
>
> When there's whitespace between the comment leader and the rest, the
> problem I'm talking about doesn't exist.
>

Thanks for confirming that.

> Also thinking that the user meant "//share" in "////share" does not make
> much sense. It's very confusing to count the number of slashes in there.
> What is the user has "/////share" (5 slashes)? Where would we want the
> ffap-string-at-point to guess the comment-starter<>comment boundary then?
>
> If you want to code a backward-compatible solution, then skipping the
> comment-start regexp should do, I think.  Where it stops, ffap should
> start looking for valid file names.
>

I thought about the comment-start variable, but for c-mode

C-h v comment-start gives "/* " and
C-h v comment-start-skip gives "\\(//+\\|/\\*+\\)\\s *"

So it will take "/" followed by 1 or more "/" as comment start.


> > With the current state of ffap-string-at-point, it creates erroneous
> behavior for many people including me. But with the patch, if a user needs
> to put a path like "//share" in a comment for a major mode using "//" as
> comment prefix, all they need to do is use a space to separate the two.
> Also I think that it is very unlikely that someone would have a confusing
> comment like "////share" where the user meant "//share" (or "/share").
>
> I see your point.


Thanks.


> My point is that when we introduce
> backward-incompatible behavior, which makes previously valid use cases
> invalid, such incompatible behavior should initially be opt-in, even
> if it looks to you that the previous behavior made no sense.  I have
> enough gray hair to testify how such decisions in the past turned out
> to be annoyances for some users.  Later, usually years later, we could
> collect user experience and decide to make this the default behavior.
>

If the new behavior is not made the default then we will never know if it
causes user annoyance.

So if you disagree with my suggestion to skip comment-start instead of
> using comment-search-forward, the behavior you propose should IMO be
> off by default.
>

If that's the only way forward, I will add a defcustom in the next patch.
But it doesn't feel right. A defcustom is being created for a very corner
case. Even if I agree with that, it does not feel right to set its default
value so that the buggy behavior is retained by default just to backward
compatibility sake.

How about we add the defcustom and set the default value so that this bug
is fixed. And we wait for people using the master branch to report any
issues caused by this. Also we let emacs-devel know that this defcustom is
going in.
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 5331 bytes --]

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Mon, 25 Jul 2016 17:13:51 +0000
> Cc: 24057@debbugs.gnu.org, npostavs@users.sourceforge.net
> 
>  So if you disagree with my suggestion to skip comment-start instead of
>  using comment-search-forward, the behavior you propose should IMO be
>  off by default.
> 
> If that's the only way forward, I will add a defcustom in the next patch. But it doesn't feel right. A defcustom is
> being created for a very corner case. Even if I agree with that, it does not feel right to set its default value so
> that the buggy behavior is retained by default just to backward compatibility sake.

Not sure which behavior you call "buggy".  I'm okay with
unconditionally skipping the comment leader string, such as "//" in
C/C++ case.  If you want to skip more than that, I think skipping the
rest should be optional, defaulting to off.  How's that "buggy"?

> How about we add the defcustom and set the default value so that this bug is fixed. And we wait for people
> using the master branch to report any issues caused by this. Also we let emacs-devel know that this
> defcustom is going in. 

I don't want us to introduce backward-incompatible behavior, except
when strictly necessary.  In this case, skipping the initial "//" is
necessary, but skipping more slashes is not.

Thanks.

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 4328 bytes --]

On Mon, Jul 25, 2016 at 1:24 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Not sure which behavior you call "buggy".  I'm okay with
> unconditionally skipping the comment leader string, such as "//" in
> C/C++ case.  If you want to skip more than that, I think skipping the
> rest should be optional, defaulting to off.  How's that "buggy"?
>

Thanks for the clarification. I thought that you were suggesting to have a
defcustom that would either use or not use comment-search-forward in
ffap-string-at-point with the default set such that the old (current)
behavior of not skipping the comment starter at all is retained. I refer to
this "not skipping the comment starter at all" behavior as buggy.


> > How about we add the defcustom and set the default value so that this
> bug is fixed. And we wait for people
> > using the master branch to report any issues caused by this. Also we let
> emacs-devel know that this
> > defcustom is going in.
>
> I don't want us to introduce backward-incompatible behavior, except
> when strictly necessary.  In this case, skipping the initial "//" is
> necessary, but skipping more slashes is not.
>

Understood. In that case, the code will have to be made more complex...

Here's a quick attempt .. Can you please have a look at it?

 (defun modi/ffap-string-at-point (&optional mode)
      "Return a string of characters from around point.

MODE (defaults to value of `major-mode') is a symbol used to look up
string syntax parameters in `ffap-string-at-point-mode-alist'.

If MODE is not found, we use `file' instead of MODE.

If the region is active,return a string from the region.

If the point is in a comment, ensure that the returned string does not
contain
the comment start characters (especially for major modes that have '//' as
comment start characters).

Sets variables `ffap-string-at-point' and `ffap-string-at-point-region'. "
      (let* ((args
              (cdr
               (or (assq (or mode major-mode)
ffap-string-at-point-mode-alist)
                   (assq 'file ffap-string-at-point-mode-alist))))
             (region-selected (use-region-p))
             (pt (point))
             (beg (if region-selected
                      (region-beginning)
                    (save-excursion
                      (skip-chars-backward (car args))
                      (skip-chars-forward (nth 1 args) pt)
                      (point))))
             (end (if region-selected
                      (region-end)
                    (save-excursion
                      (skip-chars-forward (car args))
                      (skip-chars-backward (nth 2 args) pt)
                      (point))))
             beg-temp)
        ;; Ensure that if "//" is a valid comment starter in the current
major
        ;; mode, then that is removed from the returned string.
        (when (and (null region-selected)
                   ;; Check if the first character is '/' and not part of
the
                   ;; comment
                   (save-excursion
                     (goto-char beg)
                     (and (looking-at "/")
                          (null (nth 4 (syntax-ppss)))))
                   ;; Check if the second character is '/' and also not
part of
                   ;; the comment
                   (setq beg-temp (+ 1 beg))
                   (if (> end beg-temp)
                       (save-excursion
                         (goto-char beg-temp)
                         (and (looking-at "/")
                              (null (nth 4 (syntax-ppss)))))
                     nil))
          ;; Check if the third character *is* part of the comment
          (setq beg-temp (+ 2 beg))
          (if (> end beg-temp)
              (save-excursion
                (goto-char beg-temp)
                (when (nth 4 (syntax-ppss))
                  (setq beg beg-temp)))))
        (message "beg = %d beg-temp = %S end = %d "
                 beg beg-temp end)
        (prog1
            (setq ffap-string-at-point
                  (buffer-substring-no-properties
                   (setcar ffap-string-at-point-region beg)
                   (setcar (cdr ffap-string-at-point-region) end)))
          (message "dbg: %S" ffap-string-at-point)
          )))
    (advice-add 'ffap-string-at-point :override #'modi/ffap-string-at-point)
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 6530 bytes --]

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 6268 bytes --]

Here a git formatted patch .. it's an optimized, generic version of the
snippet in my previous email. It's now "/" agnostic.

Here's the new table of expected values of ffap-string-at-point:
(x indicates the (point))

|-----------------------------------+---------------------------------|
| Example string in `c-mode' buffer | Returned `ffap-string-at-point' |
|-----------------------------------+---------------------------------|
| x//tmp                            | "tmp"                           |
| //xtmp                            | "tmp"                           |
| x///tmp                           | "/tmp"                          |
| //x/tmp                           | "/tmp"                          |
| x////tmp                          | "//tmp"                         |
| ////xtmp                          | "//tmp"                         |
| x// //tmp                         | ""                              |
| // x/tmp                          | "/tmp"                          |
| // x//tmp                         | "//tmp"                         |
|-----------------------------------+---------------------------------|

===== Patch follows

From 5e000cebb993a8cdccdf5e67f6b0eb66b4a267d8 Mon Sep 17 00:00:00 2001
From: Kaushal Modi <kaushal.modi@gmail.com>
Date: Mon, 25 Jul 2016 16:08:50 -0400
Subject: [PATCH] Do not include comment start chars in ffap string

* lisp/ffap.el (ffap-string-at-point): If the point is in a comment,
ensure that the returned string does not contain the comment start
characters (especially for major modes that have '//' as comment start
characters).

Otherwise, in a major mode like c-mode, with `ido-mode' enabled and
`ido-use-filename-at-point' set to `guess', doing "C-x C-f" on a "//foo"
comment will initiate an attempt to access a path "//foo" (Bug#24057).
---
 lisp/ffap.el | 87
+++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 22 deletions(-)

diff --git a/lisp/ffap.el b/lisp/ffap.el
index 7013e6e..8708a17 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -1097,33 +1097,76 @@ ffap-string-at-point

 (defun ffap-string-at-point (&optional mode)
   "Return a string of characters from around point.
+
 MODE (defaults to value of `major-mode') is a symbol used to look up
 string syntax parameters in `ffap-string-at-point-mode-alist'.
+
 If MODE is not found, we use `file' instead of MODE.
-If the region is active, return a string from the region.
-Sets the variable `ffap-string-at-point' and the variable
-`ffap-string-at-point-region'."
+
+If the region is active,return a string from the region.
+
+If the point is in a comment, ensure that the returned string does not
contain
+the comment start characters (especially for major modes that have '//' as
+comment start characters).
+
+Sets variables `ffap-string-at-point' and `ffap-string-at-point-region'. "
   (let* ((args
-  (cdr
-   (or (assq (or mode major-mode) ffap-string-at-point-mode-alist)
-       (assq 'file ffap-string-at-point-mode-alist))))
- (pt (point))
- (beg (if (use-region-p)
-  (region-beginning)
- (save-excursion
-  (skip-chars-backward (car args))
-  (skip-chars-forward (nth 1 args) pt)
-  (point))))
- (end (if (use-region-p)
-  (region-end)
- (save-excursion
-  (skip-chars-forward (car args))
-  (skip-chars-backward (nth 2 args) pt)
-  (point)))))
+          (cdr
+           (or (assq (or mode major-mode) ffap-string-at-point-mode-alist)
+               (assq 'file ffap-string-at-point-mode-alist))))
+         (region-selected (use-region-p))
+         (pt (point))
+         (beg (if region-selected
+                  (region-beginning)
+                (save-excursion
+                  (skip-chars-backward (car args))
+                  (skip-chars-forward (nth 1 args) pt)
+                  (point))))
+         (end (if region-selected
+                  (region-end)
+                (save-excursion
+                  (skip-chars-forward (car args))
+                  (skip-chars-backward (nth 2 args) pt)
+                  (point))))
+         (beg-new beg))
+    ;; (message "ffap-string-at-point dbg: beg = %d end = %d" beg end)
+    ;; If the initial characters of the to-be-returned string are the
+    ;; current major mode's comment starter characters, *and* are not
+    ;; part of a comment, remove those from the returned string
+    ;; (Bug#24057).
+    ;; Example comments in `c-mode' (which considers lines beginning
+    ;; with "//" as comments):
+    ;;  //tmp - This is a comment. It does not contain any path reference.
+    ;;  ///tmp - This is a comment. The "/tmp" portion in that is a path.
+    ;;  ////tmp - This is a comment. The "//tmp" portion in that is a path.
+    (when (and
+           ;; Proceed if no region is selected by the user.
+           (null region-selected)
+           ;; Check if END character is part of a comment.
+           (save-excursion
+             (goto-char end)
+             (nth 4 (syntax-ppss))))
+      (save-excursion
+        ;; Increment BEG till point at BEG is in a comment too.
+        ;; (nth 4 (syntax-ppss)) will be null for comment start
+        ;; characters (for example, for the "//" characters in
+        ;; `c-mode' line comments).
+        (setq beg (catch 'break
+                    (while (< beg-new end)
+                      (goto-char beg-new)
+                      (if (nth 4 (syntax-ppss)) ; in a comment
+                          (throw 'break beg-new)
+                        (setq beg-new (1+ beg-new))))
+                    end)))) ; Set BEG to END if no throw happens
+    ;; (message "ffap-string-at-point dbg: beg = %d beg-new = %d"
+    ;;          beg beg-new)
     (setq ffap-string-at-point
-  (buffer-substring-no-properties
-   (setcar ffap-string-at-point-region beg)
-   (setcar (cdr ffap-string-at-point-region) end)))))
+          (buffer-substring-no-properties
+           (setcar ffap-string-at-point-region beg)
+           (setcar (cdr ffap-string-at-point-region) end)))
+    ;; (message "ffap-string-at-point dbg: ffap-string-at-point = %S"
+    ;;          ffap-string-at-point)
+    ffap-string-at-point))

 (defun ffap-string-around ()
   ;; Sometimes useful to decide how to treat a string.
-- 
2.9.2

-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 10390 bytes --]

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Mon, 25 Jul 2016 20:18:27 +0000
> Cc: 24057@debbugs.gnu.org, npostavs@users.sourceforge.net
> 
> Here a git formatted patch .. it's an optimized, generic version of the
> snippet in my previous email. It's now "/" agnostic.

Thanks.  Most of the patch looks like whitespace changes?

> +        (setq beg (catch 'break
> +                    (while (< beg-new end)
> +                      (goto-char beg-new)
> +                      (if (nth 4 (syntax-ppss)) ; in a comment
> +                          (throw 'break beg-new)
> +                        (setq beg-new (1+ beg-new))))
> +                    end)))) ; Set BEG to END if no throw happens

Is there any problem with a more conventional while loop that stops
when syntax-ppss has its 5th element non-nil?  Why did you need to use
throw and catch here?

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 5129 bytes --]

On Tue, Jul 26, 2016 at 10:42 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > Here a git formatted patch .. it's an optimized, generic version of the
> > snippet in my previous email. It's now "/" agnostic.
>
> Thanks.  Most of the patch looks like whitespace changes?
>

I selected the whole defun and did auto-ident (C-M-\). The submitted patch
was a result of that. I thought it's a good idea to update the indentation
of the whole function. May be the lisp-indent-function changed since this
function was updated last time?

In any case, here is the patch ignoring whitespace changes. Note of caution
that the indentation will look messed up when you merge this patch:

=====

diff --git a/lisp/ffap.el b/lisp/ffap.el
index 7013e6e..d1bca04 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -1097,33 +1097,73 @@ ffap-string-at-point

 (defun ffap-string-at-point (&optional mode)
   "Return a string of characters from around point.
+
 MODE (defaults to value of `major-mode') is a symbol used to look up
 string syntax parameters in `ffap-string-at-point-mode-alist'.
+
 If MODE is not found, we use `file' instead of MODE.
+
 If the region is active,return a string from the region.
-Sets the variable `ffap-string-at-point' and the variable
-`ffap-string-at-point-region'."
+
+If the point is in a comment, ensure that the returned string does not
contain
+the comment start characters (especially for major modes that have '//' as
+comment start characters).
+
+Sets variables `ffap-string-at-point' and `ffap-string-at-point-region'. "
   (let* ((args
           (cdr
            (or (assq (or mode major-mode) ffap-string-at-point-mode-alist)
                (assq 'file ffap-string-at-point-mode-alist))))
+         (region-selected (use-region-p))
          (pt (point))
- (beg (if (use-region-p)
+         (beg (if region-selected
                   (region-beginning)
                 (save-excursion
                   (skip-chars-backward (car args))
                   (skip-chars-forward (nth 1 args) pt)
                   (point))))
- (end (if (use-region-p)
+         (end (if region-selected
                   (region-end)
                 (save-excursion
                   (skip-chars-forward (car args))
                   (skip-chars-backward (nth 2 args) pt)
-  (point)))))
+                  (point))))
+         (beg-new beg))
+    ;; (message "ffap-string-at-point dbg: beg = %d end = %d" beg end)
+    ;; If the initial characters of the to-be-returned string are the
+    ;; current major mode's comment starter characters, *and* are not part
+    ;; of a comment, remove those from the returned string (Bug#24057).
+    ;; Example comments in `c-mode' (which considers lines beginning with
+    ;; "//" as comments):
+    ;;  //tmp - This is a comment. It does not contain any path reference.
+    ;;  ///tmp - This is a comment. The "/tmp" portion in that is a path.
+    ;;  ////tmp - This is a comment. The "//tmp" portion in that is a path.
+    (when (and
+           ;; Proceed if no region is selected by the user.
+           (null region-selected)
+           ;; Check if END character is part of a comment.
+           (save-excursion
+             (goto-char end)
+             (nth 4 (syntax-ppss))))
+      (save-excursion
+        ;; Increment BEG till point at BEG is in a comment too.
+        ;; (nth 4 (syntax-ppss)) will be nil for comment start characters
+        ;; (for example, for the "//" characters in `c-mode' line
comments).
+        (setq beg (catch 'break
+                    (while (< beg-new end)
+                      (goto-char beg-new)
+                      (if (nth 4 (syntax-ppss)) ; in a comment
+                          (throw 'break beg-new)
+                        (setq beg-new (1+ beg-new))))
+                    end)))) ; Set BEG to END if no throw happens
+    ;; (message "ffap-string-at-point dbg: beg = %d beg-new = %d" beg
beg-new)
     (setq ffap-string-at-point
           (buffer-substring-no-properties
            (setcar ffap-string-at-point-region beg)
-   (setcar (cdr ffap-string-at-point-region) end)))))
+           (setcar (cdr ffap-string-at-point-region) end)))
+    ;; (message "ffap-string-at-point dbg: ffap-string-at-point = %S"
+    ;;          ffap-string-at-point)
+    ffap-string-at-point))

 (defun ffap-string-around ()
   ;; Sometimes useful to decide how to treat a string.
=====

> +        (setq beg (catch 'break
> > +                    (while (< beg-new end)
> > +                      (goto-char beg-new)
> > +                      (if (nth 4 (syntax-ppss)) ; in a comment
> > +                          (throw 'break beg-new)
> > +                        (setq beg-new (1+ beg-new))))
> > +                    end)))) ; Set BEG to END if no throw happens
>
> Is there any problem with a more conventional while loop that stops
> when syntax-ppss has its 5th element non-nil?  Why did you need to use
> throw and catch here?
>

Throw and catch just feels more intuitive to me. This construct also sets
beg to end if no throw happens.
It's just a difference of style; feel free to refactor it.
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 7645 bytes --]

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[npostavs]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> * lisp/ffap.el (ffap-string-at-point): If the point is in a comment,
> ensure that the returned string does not contain the comment start
> characters (especially for major modes that have '//' as comment start
> characters).
>

Why is there a blank line here?

> Otherwise, in a major mode like c-mode, with `ido-mode' enabled and
> `ido-use-filename-at-point' set to `guess', doing "C-x C-f" on a "//foo"
> comment will initiate an attempt to access a path "//foo" (Bug#24057).
> ---

> +    ;; (message "ffap-string-at-point dbg: beg = %d end = %d" beg end)

This can be removed.

> +           ;; Check if END character is part of a comment.
> +           (save-excursion
> +             (goto-char end)
> +             (nth 4 (syntax-ppss))))

This could be just (nth 4 (syntax-ppss end)).

> +      (save-excursion
> +        ;; Increment BEG till point at BEG is in a comment too.
> +        ;; (nth 4 (syntax-ppss)) will be null for comment start
> +        ;; characters (for example, for the "//" characters in
> +        ;; `c-mode' line comments).
> +        (setq beg (catch 'break
> +                    (while (< beg-new end)
> +                      (goto-char beg-new)
> +                      (if (nth 4 (syntax-ppss)) ; in a comment
> +                          (throw 'break beg-new)
> +                        (setq beg-new (1+ beg-new))))
> +                    end)))) ; Set BEG to END if no throw happens

This could be just

    ;; Move BEG to beginning of comment (after the comment start
    ;; characters), or END, whichever comes first.
    (let ((state (syntax-ppss beg)))
      (unless (nth 4 state)
        (parse-partial-sexp beg end nil nil state t)
        (setq beg (point))))

> +    ;; (message "ffap-string-at-point dbg: beg = %d beg-new = %d"
> +    ;;          beg beg-new)

> +    ;; (message "ffap-string-at-point dbg: ffap-string-at-point = %S"
> +    ;;          ffap-string-at-point)

These can be removed.

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[npostavs]

npostavs@users.sourceforge.net writes:

>
>> +           ;; Check if END character is part of a comment.
>> +           (save-excursion
>> +             (goto-char end)
>> +             (nth 4 (syntax-ppss))))
>
> This could be just (nth 4 (syntax-ppss end)).

Sorry, that should be (save-excursion (nth 4 (syntax-ppss end)))

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1.1: Type: text/plain, Size: 3139 bytes --]

Hi Noam,

Thanks for the detailed review. My comments are inline below.

Also I have attached a new patch re-based to the current master, and
applying all your suggestions.

On Thu, Mar 16, 2017 at 10:09 PM <npostavs@users.sourceforge.net> wrote:

> Kaushal Modi <kaushal.modi@gmail.com> writes:
>
> > * lisp/ffap.el (ffap-string-at-point): If the point is in a comment,
> > ensure that the returned string does not contain the comment start
> > characters (especially for major modes that have '//' as comment start
> > characters).
> >
>
> Why is there a blank line here?
>

Is it is convention to not format the commit summaries into multiple
paragraphs? I reread
http://git.savannah.gnu.org/cgit/emacs.git/plain/CONTRIBUTE but didn't find
any mention on that. But I have removed that newline from the patch that I
attach with this email.


> > Otherwise, in a major mode like c-mode, with `ido-mode' enabled and
> > `ido-use-filename-at-point' set to `guess', doing "C-x C-f" on a "//foo"
> > comment will initiate an attempt to access a path "//foo" (Bug#24057).
> > ---
>
> > +    ;; (message "ffap-string-at-point dbg: beg = %d end = %d" beg end)
>
> This can be removed.
>

Understood, the patch was still not in final stages.. The attached patch
does not have any debug statements.


> > +           ;; Check if END character is part of a comment.
> > +           (save-excursion
> > +             (goto-char end)
> > +             (nth 4 (syntax-ppss))))
>
> This could be just (nth 4 (syntax-ppss end)).
>

Thanks. TIL that syntax-ppss as POS as optional arg. Also retaining the
save-excursion as you correct yourself in the followup email.


> > +      (save-excursion
> > +        ;; Increment BEG till point at BEG is in a comment too.
> > +        ;; (nth 4 (syntax-ppss)) will be null for comment start
> > +        ;; characters (for example, for the "//" characters in
> > +        ;; `c-mode' line comments).
> > +        (setq beg (catch 'break
> > +                    (while (< beg-new end)
> > +                      (goto-char beg-new)
> > +                      (if (nth 4 (syntax-ppss)) ; in a comment
> > +                          (throw 'break beg-new)
> > +                        (setq beg-new (1+ beg-new))))
> > +                    end)))) ; Set BEG to END if no throw happens
>
> This could be just
>
>     ;; Move BEG to beginning of comment (after the comment start
>     ;; characters), or END, whichever comes first.
>     (let ((state (syntax-ppss beg)))
>       (unless (nth 4 state)
>         (parse-partial-sexp beg end nil nil state t)
>         (setq beg (point))))
>

Thanks! I did not know about parse-partial-sexp. Here too, I need to retain
the save-excursion, else the point will move after the comment start chars
if it is at the BOL in c-mode on a line like

    //tmp



> > +    ;; (message "ffap-string-at-point dbg: beg = %d beg-new = %d"
> > +    ;;          beg beg-new)
>
> > +    ;; (message "ffap-string-at-point dbg: ffap-string-at-point = %S"
> > +    ;;          ffap-string-at-point)
>
> These can be removed.
>

Done!

Review review the attached. Thanks!
-- 

Kaushal Modi

[-- Attachment #1.2: Type: text/html, Size: 5788 bytes --]

[-- Attachment #2: 0001-Do-not-include-comment-start-chars-in-ffap-string.patch --]
[-- Type: application/octet-stream, Size: 3852 bytes --]

From eda7a96b66f6937e094b0d76364c6711984db254 Mon Sep 17 00:00:00 2001
From: Kaushal Modi <kaushal.modi@gmail.com>
Date: Fri, 17 Mar 2017 18:03:23 -0400
Subject: [PATCH] Do not include comment start chars in ffap string

* lisp/ffap.el (ffap-string-at-point): If the point is in a comment,
ensure that the returned string does not contain the comment start
characters (especially for major modes that have '//' as comment start
characters). Otherwise, in a major mode like c-mode, with `ido-mode'
enabled and `ido-use-filename-at-point' set to `guess', doing "C-x
C-f" on a "//foo" comment will initiate an attempt to access a path
"//foo" (Bug#24057).

Co-authored-by: Noam Postavsky <npostavs@gmail.com>
---
 lisp/ffap.el | 41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/lisp/ffap.el b/lisp/ffap.el
index d7222bfb68..a15083f4fa 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -1110,32 +1110,67 @@ ffap-string-at-point
 
 (defun ffap-string-at-point (&optional mode)
   "Return a string of characters from around point.
+
 MODE (defaults to value of `major-mode') is a symbol used to look up
 string syntax parameters in `ffap-string-at-point-mode-alist'.
+
 If MODE is not found, we use `file' instead of MODE.
+
 If the region is active, return a string from the region.
-Set the variable `ffap-string-at-point' and the variable
+
+If the point is in a comment, ensure that the returned string does not
+contain the comment start characters (especially for major modes that
+have '//' as comment start characters).
+
+Set the variables `ffap-string-at-point' and
 `ffap-string-at-point-region'.
+
 When the region is active and larger than `ffap-max-region-length',
 return an empty string, and set `ffap-string-at-point-region' to '(1 1)."
   (let* ((args
 	  (cdr
 	   (or (assq (or mode major-mode) ffap-string-at-point-mode-alist)
 	       (assq 'file ffap-string-at-point-mode-alist))))
+         (region-selected (use-region-p))
 	 (pt (point))
-	 (beg (if (use-region-p)
+         (beg (if region-selected
 		  (region-beginning)
 		(save-excursion
 		  (skip-chars-backward (car args))
 		  (skip-chars-forward (nth 1 args) pt)
 		  (point))))
-	 (end (if (use-region-p)
+         (end (if region-selected
 		  (region-end)
 		(save-excursion
 		  (skip-chars-forward (car args))
 		  (skip-chars-backward (nth 2 args) pt)
 		  (point))))
          (region-len (- (max beg end) (min beg end))))
+
+    ;; If the initial characters of the to-be-returned string are the
+    ;; current major mode's comment starter characters, *and* are
+    ;; not part of a comment, remove those from the returned string
+    ;; (Bug#24057).
+    ;; Example comments in `c-mode' (which considers lines beginning
+    ;; with "//" as comments):
+    ;;  //tmp - This is a comment. It does not contain any path reference.
+    ;;  ///tmp - This is a comment. The "/tmp" portion in that is a path.
+    ;;  ////tmp - This is a comment. The "//tmp" portion in that is a path.
+    (when (and
+           ;; Proceed if no region is selected by the user.
+           (null region-selected)
+           ;; Check if END character is part of a comment.
+           (save-excursion
+             (nth 4 (syntax-ppss end))))
+      ;; Move BEG to beginning of comment (after the comment start
+      ;; characters), or END, whichever comes first.
+      (save-excursion
+        (let ((state (syntax-ppss beg)))
+          ;; (nth 4 (syntax-ppss)) will be nil for comment start chars
+          (unless (nth 4 state)
+            (parse-partial-sexp beg end nil nil state :commentstop)
+            (setq beg (point))))))
+
     (if (and (natnump ffap-max-region-length)
              (< region-len ffap-max-region-length)) ; Bug#25243.
         (setf ffap-string-at-point-region (list beg end)
-- 
2.11.0

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[npostavs]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> Is it is convention to not format the commit summaries into multiple
> paragraphs? I reread
> http://git.savannah.gnu.org/cgit/emacs.git/plain/CONTRIBUTE but didn't find
> any mention on that. But I have removed that newline from the patch that I
> attach with this email.

Oh, then I don't understand why you separated that sentence into its own
paragraph, it seemed like a continuation of the same idea.  (In general,
paragraphs should go before the ChangeLog entries, because that format
doesn't allow for multiple paragraphs in the same entry (AFAIK)).

> characters (especially for major modes that have '//' as comment start
> characters). Otherwise, in a major mode like c-mode, with `ido-mode'
             ^^^

There should be double space between sentences though.

>
> Thanks! I did not know about parse-partial-sexp. Here too, I need to retain
> the save-excursion, else the point will move after the comment start chars
> if it is at the BOL in c-mode on a line like

Oops, right.  (I would consider just putting a single save-excursion
around the 'when', but it doesn't hugely matter either way (and Drew
would probably not like the single save-excursion version ;) [1]).)

[1]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25777#38

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1.1: Type: text/plain, Size: 1880 bytes --]

On Fri, Mar 17, 2017 at 7:29 PM <npostavs@users.sourceforge.net> wrote:

>
> Oh, then I don't understand why you separated that sentence into its own
> paragraph, it seemed like a continuation of the same idea.  (In general,
> paragraphs should go before the ChangeLog entries, because that format
> doesn't allow for multiple paragraphs in the same entry (AFAIK)).
>

It's alright. I have kept the whole thing as a single paragraph.


> > characters (especially for major modes that have '//' as comment start
> > characters). Otherwise, in a major mode like c-mode, with `ido-mode'
>              ^^^
>
> There should be double space between sentences though.
>

The attached patch has this fixed.

Btw if you don't mind explaining:
- How did you detect that minor missing space? Do you have a minor mode? Or
do you have a check function?
- Being used to using single spaces at end of sentences, I always forget
adding double spaces. Is there a robust way to ensure that sentences always
end in double spaces in commit messages and in the docstrings?



> >
> > Thanks! I did not know about parse-partial-sexp. Here too, I need to
> retain
> > the save-excursion, else the point will move after the comment start
> chars
> > if it is at the BOL in c-mode on a line like
>
> Oops, right.  (I would consider just putting a single save-excursion
> around the 'when', but it doesn't hugely matter either way (and Drew
> would probably not like the single save-excursion version ;) [1]).)
>
> [1]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25777#38


It did actually occur to merge the two save-excursions, but for code
clarity, I kept them separate. I did not want to merge the save-excursion
in the "condition" part with the "action" part in

    (when (condition)
      action)

 The attached patch has the double space fixed in the commit message.

Thanks.
-- 

Kaushal Modi

[-- Attachment #1.2: Type: text/html, Size: 3290 bytes --]

[-- Attachment #2: 0001-Do-not-include-comment-start-chars-in-ffap-string.patch --]
[-- Type: application/octet-stream, Size: 3853 bytes --]

From cf61fa11dd0e0a7353c697957a16ec182b188fa9 Mon Sep 17 00:00:00 2001
From: Kaushal Modi <kaushal.modi@gmail.com>
Date: Fri, 17 Mar 2017 18:03:23 -0400
Subject: [PATCH] Do not include comment start chars in ffap string

* lisp/ffap.el (ffap-string-at-point): If the point is in a comment,
ensure that the returned string does not contain the comment start
characters (especially for major modes that have '//' as comment start
characters).  Otherwise, in a major mode like c-mode, with `ido-mode'
enabled and `ido-use-filename-at-point' set to `guess', doing "C-x
C-f" on a "//foo" comment will initiate an attempt to access a path
"//foo" (Bug#24057).

Co-authored-by: Noam Postavsky <npostavs@gmail.com>
---
 lisp/ffap.el | 41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/lisp/ffap.el b/lisp/ffap.el
index d7222bfb68..a15083f4fa 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -1110,32 +1110,67 @@ ffap-string-at-point
 
 (defun ffap-string-at-point (&optional mode)
   "Return a string of characters from around point.
+
 MODE (defaults to value of `major-mode') is a symbol used to look up
 string syntax parameters in `ffap-string-at-point-mode-alist'.
+
 If MODE is not found, we use `file' instead of MODE.
+
 If the region is active, return a string from the region.
-Set the variable `ffap-string-at-point' and the variable
+
+If the point is in a comment, ensure that the returned string does not
+contain the comment start characters (especially for major modes that
+have '//' as comment start characters).
+
+Set the variables `ffap-string-at-point' and
 `ffap-string-at-point-region'.
+
 When the region is active and larger than `ffap-max-region-length',
 return an empty string, and set `ffap-string-at-point-region' to '(1 1)."
   (let* ((args
 	  (cdr
 	   (or (assq (or mode major-mode) ffap-string-at-point-mode-alist)
 	       (assq 'file ffap-string-at-point-mode-alist))))
+         (region-selected (use-region-p))
 	 (pt (point))
-	 (beg (if (use-region-p)
+         (beg (if region-selected
 		  (region-beginning)
 		(save-excursion
 		  (skip-chars-backward (car args))
 		  (skip-chars-forward (nth 1 args) pt)
 		  (point))))
-	 (end (if (use-region-p)
+         (end (if region-selected
 		  (region-end)
 		(save-excursion
 		  (skip-chars-forward (car args))
 		  (skip-chars-backward (nth 2 args) pt)
 		  (point))))
          (region-len (- (max beg end) (min beg end))))
+
+    ;; If the initial characters of the to-be-returned string are the
+    ;; current major mode's comment starter characters, *and* are
+    ;; not part of a comment, remove those from the returned string
+    ;; (Bug#24057).
+    ;; Example comments in `c-mode' (which considers lines beginning
+    ;; with "//" as comments):
+    ;;  //tmp - This is a comment. It does not contain any path reference.
+    ;;  ///tmp - This is a comment. The "/tmp" portion in that is a path.
+    ;;  ////tmp - This is a comment. The "//tmp" portion in that is a path.
+    (when (and
+           ;; Proceed if no region is selected by the user.
+           (null region-selected)
+           ;; Check if END character is part of a comment.
+           (save-excursion
+             (nth 4 (syntax-ppss end))))
+      ;; Move BEG to beginning of comment (after the comment start
+      ;; characters), or END, whichever comes first.
+      (save-excursion
+        (let ((state (syntax-ppss beg)))
+          ;; (nth 4 (syntax-ppss)) will be nil for comment start chars
+          (unless (nth 4 state)
+            (parse-partial-sexp beg end nil nil state :commentstop)
+            (setq beg (point))))))
+
     (if (and (natnump ffap-max-region-length)
              (< region-len ffap-max-region-length)) ; Bug#25243.
         (setf ffap-string-at-point-region (list beg end)
-- 
2.11.0

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[npostavs]

tags 24057 patch
quit

Kaushal Modi <kaushal.modi@gmail.com> writes:

> Btw if you don't mind explaining:
> - How did you detect that minor missing space? Do you have a minor mode? Or
> do you have a check function?

I enable ocd-mode in my brain while reading patches ;)

> - Being used to using single spaces at end of sentences, I always forget
> adding double spaces.

More seriously, I've gotten used to using double spaces and so single
spaced sentences stand out to me more now.

> Is there a robust way to ensure that sentences always
> end in double spaces in commit messages and in the docstrings?

The trouble is that sometimes a period followed by a space doesn't end a
sentence, e.g., in "Mr. Foo".  Actually, this ambiguity is what
convinced me to switch over to double spacing.  Such cases might be rare
enough in commit messages and docstrings that we could get away with
some automatic warning on all matches for "\\sw[.] \\<" though.

>
>  The attached patch has the double space fixed in the commit message.

Thanks, I will push to master in a couple of days.

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[npostavs]

tags 24057 fixed
close 24057 26.1
quit

npostavs@users.sourceforge.net writes:

>
> Thanks, I will push to master in a couple of days.

Done [1: e472cfe8f3]

1: 2017-03-23 08:57:13 -0400 e472cfe8f3b01f29a49614f6207e4128e8b36b8c
  Do not include comment start chars in ffap string

bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 256 bytes --]

Thank you!

On Thu, Mar 23, 2017, 8:59 AM <npostavs@users.sourceforge.net> wrote:

>
> Done [1: e472cfe8f3]
>
> 1: 2017-03-23 08:57:13 -0400 e472cfe8f3b01f29a49614f6207e4128e8b36b8c
>   Do not include comment start chars in ffap string
>
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 720 bytes --]