From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Kaushal Modi Newsgroups: gmane.emacs.bugs Subject: bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path Date: Mon, 25 Jul 2016 17:13:51 +0000 Message-ID: References: <83lh0sx0yf.fsf@gnu.org> <83shv0v716.fsf@gnu.org> <83poq4v4jk.fsf@gnu.org> <83wpkbt92i.fsf@gnu.org> <83mvl5tzv7.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=94eb2c1917a016a6bd053878e9b3 X-Trace: ger.gmane.org 1469466926 9152 80.91.229.3 (25 Jul 2016 17:15:26 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 25 Jul 2016 17:15:26 +0000 (UTC) Cc: 24057@debbugs.gnu.org, npostavs@users.sourceforge.net To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Jul 25 19:15:17 2016 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 1bRjTG-0003GS-H4 for geb-bug-gnu-emacs@m.gmane.org; Mon, 25 Jul 2016 19:15:14 +0200 Original-Received: from localhost ([::1]:33647 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRjTF-0007Sp-P1 for geb-bug-gnu-emacs@m.gmane.org; Mon, 25 Jul 2016 13:15:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:33462) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRjT8-0007PV-Os for bug-gnu-emacs@gnu.org; Mon, 25 Jul 2016 13:15:08 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bRjT4-0006RZ-ED for bug-gnu-emacs@gnu.org; Mon, 25 Jul 2016 13:15:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:53037) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRjT4-0006RV-As for bug-gnu-emacs@gnu.org; Mon, 25 Jul 2016 13:15:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1bRjT4-0003aF-3U for bug-gnu-emacs@gnu.org; Mon, 25 Jul 2016 13:15:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Kaushal Modi Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 25 Jul 2016 17:15:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 24057 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 24057-submit@debbugs.gnu.org id=B24057.146946684813696 (code B ref 24057); Mon, 25 Jul 2016 17:15:02 +0000 Original-Received: (at 24057) by debbugs.gnu.org; 25 Jul 2016 17:14:08 +0000 Original-Received: from localhost ([127.0.0.1]:37141 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bRjSC-0003Yp-B0 for submit@debbugs.gnu.org; Mon, 25 Jul 2016 13:14:08 -0400 Original-Received: from mail-oi0-f53.google.com ([209.85.218.53]:36046) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bRjSB-0003YU-6a for 24057@debbugs.gnu.org; Mon, 25 Jul 2016 13:14:07 -0400 Original-Received: by mail-oi0-f53.google.com with SMTP id w18so261427287oiw.3 for <24057@debbugs.gnu.org>; Mon, 25 Jul 2016 10:14:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9leNOgrZ8bY7ssVWJksgG8gB/ryxKdmEDhD9zRTTZh8=; b=XpGUcQfbwaAktKRB+9IdcUSdei/aHBpwtElxFarDZlkiDXHlsgVs8LwW3jhjdty5Qs juPks7M5MO5aHGlfi32XyxOxDvQF6nRik12nSA44rhIgr+KFh8W7hOm/OI18YVerrJGv dDbR8xD5bAGjiDt0WRe6GqT91BA4L2AQEdhQlJYtpGKaCTV3jCifV8Lb7zMduVrQN9GG x4Nadgdly4K7ALQjBEs8sqtvARnz8WL5QLrhqLEEPJJ07IBnlzSbBEOmnDKrl16j7P4U 23K1MEsCqQAywdkBJR5mBIvKYxL/vZqi4ZH6bBmJqGA+uv1GftQGmSunU0J4iQVjWTfa UNTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9leNOgrZ8bY7ssVWJksgG8gB/ryxKdmEDhD9zRTTZh8=; b=EtujzVcEf6T5ziEyuJe/mnLMz9uD3KTEdOY8Up/cqNyhFOQbj+z2Obin+jx4pdknyi EmOS1YIXxqhlXt9+3hCEpdxiQBbeDgDW0tq9fP2DvdKpZfPmmL9FEfm9Pkn4ERBYP6G3 SzfOJESSUrBb3oMlCpYxhNPajvLffxAwYz212NYLdjgUURK3uPCX2yCvaSMm58291J56 0b1W/iaXT4t9nYotXg3c9x+LSuBS3NYkMb1qg3rIFcflob1Qo2WyzhV0M+Ib5rCZCZWK o3ndW2iOzHvjxfPz+6zetuHyKfyXPUjMHspqGP4Aeki98MUz1ZiPoM5Sr6YHaUQ6gHB7 BhNg== X-Gm-Message-State: AEkoous5Bh0osLBBRFqnFFg/cDqBlfPSXGd70qpSx/VD9ZjNbto8E4wytJELdB4h3tSrIfYWAe8kXzqmDTME/Q== X-Received: by 10.157.17.169 with SMTP id v38mr9518482otf.11.1469466841351; Mon, 25 Jul 2016 10:14:01 -0700 (PDT) In-Reply-To: <83mvl5tzv7.fsf@gnu.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.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" Xref: news.gmane.org gmane.emacs.bugs:121525 Archived-At: --94eb2c1917a016a6bd053878e9b3 Content-Type: text/plain; charset=UTF-8 On Mon, Jul 25, 2016 at 1:03 PM Eli Zaretskii 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 --94eb2c1917a016a6bd053878e9b3 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On Mon, Jul 25= , 2016 at 1:03 PM Eli Zaretskii <eliz@gn= u.org> wrote:
If I understoo= d 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.=C2=A0 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.
=C2=A0
> The problem with that is .. what is a user has a decorative comment li= ke:
>
> ///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<= br> prohibits valid use cases.

OK.=C2=A0
=C2=A0
> So the best way to make the user's intent clear is by preceding th= e path with a space.

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

<= /div>
Thanks for confirming that.=C2=A0

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

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

I t= hought about the comment-start variable, but for c-mode=C2=A0
C-h v comment-start gives=C2=A0"/* " and=C2=A0
<= div>C-h v comment-start-skip gives=C2=A0"\\(//+\\|/\\*+\\)\\s *"<= /div>

So it will take "/" followed by 1 or mor= e "/" as comment start.
=C2=A0
> With the current state of ffap-string-at-point, it creates erroneous b= ehavior for many people including me. But with the patch, if a user needs t= o put a path like "//share" in a comment for a major mode using &= quot;//" as comment prefix, all they need to do is use a space to sepa= rate 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.=C2=A0

Thanks.
= =C2=A0
My point is that when we introdu= ce
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.=C2=A0 I have enough gray hair to testify how such decisions in the past turned out
to be annoyances for some users.=C2=A0 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.=C2=A0

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 f= eel right. A defcustom is being created for a very corner case. Even if I a= gree 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 defaul= t 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 th= at this defcustom is going in.=C2=A0
-- <= br>

Kaushal Modi

--94eb2c1917a016a6bd053878e9b3--