GNU bug report logs - #25243
26.0.50; ffap-guesser very slow w/ region active in large diff files

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 25243 in the body.
You can then email your comments to 25243 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.0.50; ffap-guesser very slow w/ region active in large diff files
Date: Thu, 22 Dec 2016 00:35:59 +0900

The slowness is apparent for diff files with > 2 klines.

emacs -Q -l ffap
;; From emacs git rep. extract a diff w/ 10 klines.
M-! git diff HEAD~200 | head -10000 > /tmp/test-Bug25243
M-: (find-file "/tmp/test-Bug25243")
C-x h
M-: (ffap-guesser)
;; It took > 2 min in my box.

In this example `ffap-guesser' is spending long time testing the
whole buffer content as a file name candidate.  It might has
sense to introduce a maximum limit in the length for file names
in `ffap-file-at-point'.

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 9e919ba3c86a912bc42cb8e439ad7b8b3660dc37 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha <at> gmail.com>
Date: Thu, 22 Dec 2016 00:27:50 +0900
Subject: [PATCH] ffap-file-at-point: Limit length of file names

Do not spend time checking large strings which are
likely not actual file names (Bug#25243).
* lisp/ffap.el (ffap-file-name-max-length): New option.
(ffap-file-at-point): Use it.
; etc/NEWS: Add entry for the new option.
---
 etc/NEWS     |  4 ++++
 lisp/ffap.el | 12 +++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 7338c0c6a7..fd89568c6e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -312,6 +312,10 @@ the file's actual content before prompting the user.
 
 * Changes in Specialized Modes and Packages in Emacs 26.1
 
+** ffap
+
+*** A new user option 'ffap-file-name-max-length'.
+
 ** Electric-Buffer-menu
 
 +++
diff --git a/lisp/ffap.el b/lisp/ffap.el
index 3d7cebadcf..1df30e4516 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -292,6 +292,13 @@ dired-at-point-require-prefix
   :group 'ffap
   :version "20.3")
 
+(defcustom ffap-file-name-max-length 1024
+  "Maximum length allowed for file names in `ffap-file-at-point'."
+  :type '(choice (const :tag "Unlimited" nil)
+                 (integer :tag "File Name Max Length" 1024))
+  :group 'ffap
+  :version "26.1")
+
 
 ;;; Compatibility:
 ;;
@@ -1244,6 +1251,9 @@ ffap-file-at-point
 	 (abs (file-name-absolute-p name))
 	 (default-directory default-directory)
          (oname name))
+    ;; When oname is very large is likely not a file name.
+    (when (or (null ffap-file-name-max-length)
+              (< (length oname) ffap-file-name-max-length))
     (unwind-protect
 	(cond
 	 ;; Immediate rejects (/ and // and /* are too common in C/C++):
@@ -1334,7 +1344,7 @@ ffap-file-at-point
             (and (not (string= dir "/"))
 		 (ffap-file-exists-string dir))))
 	 )
-      (set-match-data data))))
+      (set-match-data data)))))
 
 ;;; Prompting (`ffap-read-file-or-url'):
 ;;
-- 
2.11.0

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.4)
 of 2016-12-21
Repository revision: 8661313efd5fd5b0a27fe82f276a1ff862646424

Message #8 received at 25243 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Tino Calancha <tino.calancha <at> gmail.com>, 25243 <at> debbugs.gnu.org
Subject: RE: bug#25243: 26.0.50; ffap-guesser very slow w/ region active in
 large diff files
Date: Wed, 21 Dec 2016 08:22:19 -0800 (PST)

Amusing backstory:

In some of my code I use `ffap-guesser' to pick up
file-name defaults for own my version of `read-file-name'.

I added that pretty much naively, without digging into the
`ffap-guesser' code to check what it does.  It seemed to
work pretty well.

Tino sent me a diff file of about 14 KB, which showed the
problem.  Other files, even 10 times as large (!), didn't
necessarily show the problem.

The test case was to use `write-region' in that diff-file
buffer, after `C-x h'.  (In context, `write-region' invoked
my version of `read-file-name'.)

Looking closer, I saw that `ffap-guesser' tries to use the
text in the active region to guess a file name, and that it
passes this text around to multiple functions that examine
it, search through it, and massage it (e.g., remove text
properties).

Needless to say, this is problematic for a large active region.

Had the doc string of `ffap-guesser' (and other functions
that call it and that it calls) mentioned that it uses the
active region, I likely would never have used it as I did.

Stefan did add a comment, in Emacs 23, for one call to
`ffap-guesser', which is helpful:

 ;; Don't use the region here, since it can be something
 ;; completely unwieldy.  If the user wants that, she could
 ;; use M-w before and then C-y.  --Stef

But it's not just for that occurrence that the problem can
exist.  And putting that info in a doc string would be more
helpful than just in a comment.  It's not just a message to
Emacs implementers; it's something that users of the ffap
functions should know about.

Preventing the problem in the first place, as Tino's patch
tries to do, is even better than just documenting the gotcha.

The ffap.el code does prevent the problem itself for some
uses of `ffap-guesser' (e.g. `ffap-prompter'), but it is
in `ffap-guesser' itself (or `ffap-file-at-point' or
`ffap-string-at-point') that this should be done.

Wrt the proposed patch:

1. It is `ffap-string-at-point' that picks up the active
   region text as a string and removes its properties.
   Since that is what is slow, I think it is in that
   function that preventing this from happening should
   occur.  The patch tries to control this only in
   `ffap-file-at-point', and it does so _after_ calling
   `ffap-string-at-point', which is too late (AFAICS).

   I think that `ffap-string-at-point' should control
   this.  It should not try to pick up a file name from
   a 14 KB diff buffer.

2. I'm not sure that a user option is really what's called
   for here.  I'd suggest a defvar, but only because Emacs
   Dev does not really like programs to bind option values
   (I have little problem with that, myself), and that is
   the main place where I expect the value to be changed:
   programmatically.

Thanks, Tino, for finding this bug and reporting it.

Message #11 received at 25243 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 25243 <at> debbugs.gnu.org, tino.calancha <at> gmail.com
Subject: Re: bug#25243: 26.0.50;
 ffap-guesser very slow w/ region active in large diff files
Date: Thu, 22 Dec 2016 13:31:25 +0900

Drew Adams <drew.adams <at> oracle.com> writes:

> Amusing backstory:
Thank you very much for adding more info to the report!

> Wrt the proposed patch:
>
> 1. It is `ffap-string-at-point' that picks up the active
>    region text as a string and removes its properties.
>    Since that is what is slow, I think it is in that
>    function that preventing this from happening should
>    occur.  The patch tries to control this only in
>    `ffap-file-at-point', and it does so _after_ calling
>    `ffap-string-at-point', which is too late (AFAICS).
>
>    I think that `ffap-string-at-point' should control
>    this.  It should not try to pick up a file name from
>    a 14 KB diff buffer.
Agreed.  See updated patch below.

> 2. I'm not sure that a user option is really what's called
>    for here.  I'd suggest a defvar, but only because Emacs
>    Dev does not really like programs to bind option values
>    (I have little problem with that, myself), and that is
>    the main place where I expect the value to be changed:
>    programmatically.
Agreed.  It's good to follow an uniform style throughout core packages.
I've updated the patch to use a defvar instead.

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 46507ea7de34bd54154b9ecb05e607104ef99d67 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha <at> gmail.com>
Date: Thu, 22 Dec 2016 13:14:22 +0900
Subject: [PATCH] ffap-string-at-point: Limit max length of active region

Do not spend time checking large strings which are
likely not valid candidates (Bug#25243).
* lisp/ffap.el (ffap-max-region-length): New variable.
(ffap-string-at-point): Use it.
* test/lisp/ffap-tests.el: New test suite.
(ffap-tests-25243): Add test for this bug.
---
 lisp/ffap.el            | 17 +++++++++++-----
 test/lisp/ffap-tests.el | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 5 deletions(-)
 create mode 100644 test/lisp/ffap-tests.el

diff --git a/lisp/ffap.el b/lisp/ffap.el
index 3d7cebadcf..ad4b70d737 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -203,6 +203,9 @@ ffap-foo-at-bar-prefix
 		 )
   :group 'ffap)
 
+(defvar ffap-max-region-length 1024
+  "Maximum allowed region length in `ffap-string-at-point'.")
+
 
 ;;; Peanut Gallery (More User Variables):
 ;;
@@ -1119,11 +1122,15 @@ ffap-string-at-point
 		(save-excursion
 		  (skip-chars-forward (car args))
 		  (skip-chars-backward (nth 2 args) pt)
-		  (point)))))
-    (setq ffap-string-at-point
-	  (buffer-substring-no-properties
-	   (setcar ffap-string-at-point-region beg)
-	   (setcar (cdr ffap-string-at-point-region) end)))))
+		  (point))))
+         (region-len (- (max beg end) (min beg end))))
+    (if (or (null ffap-max-region-length)
+            (< region-len ffap-max-region-length)) ; Bug#25243.
+        (setf ffap-string-at-point-region (list beg end)
+              ffap-string-at-point
+              (buffer-substring-no-properties beg end))
+      (setf ffap-string-at-point-region (list 1 1)
+            ffap-string-at-point ""))))
 
 (defun ffap-string-around ()
   ;; Sometimes useful to decide how to treat a string.
diff --git a/test/lisp/ffap-tests.el b/test/lisp/ffap-tests.el
new file mode 100644
index 0000000000..ca6a1f4d78
--- /dev/null
+++ b/test/lisp/ffap-tests.el
@@ -0,0 +1,53 @@
+;;; ffap-tests.el --- Test suite for ffap.el -*- lexical-binding: t -*-
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; Author: Tino Calancha <tino.calancha <at> gmail.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'ffap)
+
+(ert-deftest ffap-tests-25243 ()
+  "Test for http://debbugs.gnu.org/25243 ."
+  (let ((file (make-temp-file "test-Bug#25243")))
+    (unwind-protect
+        (with-temp-file file
+          (let ((str "diff --git b/lisp/ffap.el a/lisp/ffap.el
+index 3d7cebadcf..ad4b70d737 100644
+--- b/lisp/ffap.el
++++ a/lisp/ffap.el
+@@ -203,6 +203,9 @@ ffap-foo-at-bar-prefix
+"))
+            (transient-mark-mode 1)
+            (insert
+             (concat
+              str
+              (make-string ffap-max-region-length #xa)
+              (format "%s ENDS HERE" file)))
+            (mark-whole-buffer)
+            (should (equal "" (ffap-string-at-point)))
+            (should (equal '(1 1) ffap-string-at-point-region))))
+      (and (file-exists-p file) (delete-file file)))))
+
+(provide 'ffap-tests)
+
+;;; ffap-tests.el ends here
-- 
2.11.0

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.4)
 of 2016-12-22
Repository revision: de0671096706bf7404cddce277b918c8d769f17b

Message #14 received at 25243 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: 25243 <at> debbugs.gnu.org
Subject: RE: bug#25243: 26.0.50; ffap-guesser very slow w/ region active in
 large diff files
Date: Thu, 22 Dec 2016 09:22:38 -0800 (PST)

Thanks for working on this, Tino.  Some minor comments below.

> +(defvar ffap-max-region-length 1024
> +  "Maximum allowed region length in `ffap-string-at-point'.")

1. I think it should say "active region".

Very minor (can be ignored): If we say something is not allowed it is
unclear what happens.  In particular, it can suggest that we raise an
error. You might want to say here that if the active region is larger
... it is considered empty.  (Or just refer to `ffap-string-at-point',
which you do already.) 

> +         (region-len (- (max beg end) (min beg end))))
> +    (if (or (null ffap-max-region-length)
> +            (< region-len ffap-max-region-length)) ; Bug#25243.
> +        (setf ffap-string-at-point-region (list beg end)
> +              ffap-string-at-point
> +              (buffer-substring-no-properties beg end))
> +      (setf ffap-string-at-point-region (list 1 1)
> +            ffap-string-at-point ""))))

1. The doc string should say that if the active region is
larger than `ffap-max-region-length' then those two vars
are set to ... and ....

2. Instead of testing whether the max-length var is nil, I'd suggest
testing it with `natnump', to take care of the unexpected case where
it might get assigned a non-number.

Message #17 received at 25243 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 25243 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#25243: 26.0.50;
 ffap-guesser very slow w/ region active in large diff files
Date: Fri, 23 Dec 2016 16:12:41 +0900

Drew Adams <drew.adams <at> oracle.com> writes:

Thanks for the comments!  I answer below.

>> +(defvar ffap-max-region-length 1024
>> +  "Maximum allowed region length in `ffap-string-at-point'.")
>
> 1. I think it should say "active region".
Agreed.
>
> Very minor (can be ignored): If we say something is not allowed it is
> unclear what happens.  In particular, it can suggest that we raise an
> error. You might want to say here that if the active region is larger
> ... it is considered empty.  (Or just refer to `ffap-string-at-point',
> which you do already.) 
OK.

>> +         (region-len (- (max beg end) (min beg end))))
>> +    (if (or (null ffap-max-region-length)
>> +            (< region-len ffap-max-region-length)) ; Bug#25243.
>> +        (setf ffap-string-at-point-region (list beg end)
>> +              ffap-string-at-point
>> +              (buffer-substring-no-properties beg end))
>> +      (setf ffap-string-at-point-region (list 1 1)
>> +            ffap-string-at-point ""))))
>
> 1. The doc string should say that if the active region is
> larger than `ffap-max-region-length' then those two vars
> are set to ... and ....
I see.  I added some text.  The var `ffap-string-at-point' gets the
returned value of the function with the same name; so it suffices to say
that in that case the func. returns "".  I also mention that
`ffap-string-at-point-region' is set to '(1 1).
> 2. Instead of testing whether the max-length var is nil, I'd suggest
> testing it with `natnump', to take care of the unexpected case where
> it might get assigned a non-number.
Yes, `natnump' is a better choice.

Following is the updated patch:
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
diff --git a/lisp/ffap.el b/lisp/ffap.el
index 3d7cebadcf..d91f50e060 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -203,6 +203,11 @@ ffap-foo-at-bar-prefix
 		 )
   :group 'ffap)
 
+(defvar ffap-max-region-length 1024
+  "Maximum active region length.
+When the region is active and larger than this value,
+`ffap-string-at-point' returns an empty string.")
+
 
 ;;; Peanut Gallery (More User Variables):
 ;;
@@ -1101,8 +1106,10 @@ ffap-string-at-point
 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'."
+Set the variable `ffap-string-at-point' and the variable
+`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)
@@ -1119,11 +1126,15 @@ ffap-string-at-point
 		(save-excursion
 		  (skip-chars-forward (car args))
 		  (skip-chars-backward (nth 2 args) pt)
-		  (point)))))
-    (setq ffap-string-at-point
-	  (buffer-substring-no-properties
-	   (setcar ffap-string-at-point-region beg)
-	   (setcar (cdr ffap-string-at-point-region) end)))))
+		  (point))))
+         (region-len (- (max beg end) (min beg end))))
+    (if (or (not (natnump ffap-max-region-length))
+            (< region-len ffap-max-region-length)) ; Bug#25243.
+        (setf ffap-string-at-point-region (list beg end)
+              ffap-string-at-point
+              (buffer-substring-no-properties beg end))
+      (setf ffap-string-at-point-region (list 1 1)
+            ffap-string-at-point ""))))
 
 (defun ffap-string-around ()
   ;; Sometimes useful to decide how to treat a string.

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.4)
 of 2016-12-21
Repository revision: 8661313efd5fd5b0a27fe82f276a1ff862646424

Message #20 received at 25243 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: 25243 <at> debbugs.gnu.org
Subject: RE: bug#25243: 26.0.50; ffap-guesser very slow w/ region active in
 large diff files
Date: Fri, 23 Dec 2016 07:41:02 -0800 (PST)

below

> > 2. Instead of testing whether the max-length var is nil, I'd suggest
> > testing it with `natnump', to take care of the unexpected case where
> > it might get assigned a non-number.
> Yes, `natnump' is a better choice.

> +    (if (or (not (natnump ffap-max-region-length))
> +            (< region-len ffap-max-region-length)) ; Bug#25243.
> +        (setf ffap-string-at-point-region (list beg end)
> +              ffap-string-at-point
> +              (buffer-substring-no-properties beg end))
> +      (setf ffap-string-at-point-region (list 1 1)
> +            ffap-string-at-point ""))))

I'd suggest the other way around.  What you have lets someone or
some code assign a non-number and get the same slow behavior we
want to avoid.  I'd say (and (natnump ...) (< region-len ...)).

IOW, if it's not a natnump and the size is not smaller than that
number then don't use the region.

The rest sounds good to me.

Message #23 received at 25243 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 25243 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#25243: 26.0.50;
 ffap-guesser very slow w/ region active in large diff files
Date: Sat, 24 Dec 2016 11:53:27 +0900

Drew Adams <drew.adams <at> oracle.com> writes:

> below
>
>> > 2. Instead of testing whether the max-length var is nil, I'd suggest
>> > testing it with `natnump', to take care of the unexpected case where
>> > it might get assigned a non-number.
>> Yes, `natnump' is a better choice.
>
>> +    (if (or (not (natnump ffap-max-region-length))
>> +            (< region-len ffap-max-region-length)) ; Bug#25243.
>> +        (setf ffap-string-at-point-region (list beg end)
>> +              ffap-string-at-point
>> +              (buffer-substring-no-properties beg end))
>> +      (setf ffap-string-at-point-region (list 1 1)
>> +            ffap-string-at-point ""))))
>
> I'd suggest the other way around.  What you have lets someone or
> some code assign a non-number and get the same slow behavior we
> want to avoid.  I'd say (and (natnump ...) (< region-len ...)).
>
> IOW, if it's not a natnump and the size is not smaller than that
> number then don't use the region.
It sounds reasonable.  Thank you.
If i don't see further comments in a few days i will push
the following patch to the master branch:

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 7e8268b975c8385015769755e8ba1e9854d64da1 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha <at> gmail.com>
Date: Sat, 24 Dec 2016 11:31:53 +0900
Subject: [PATCH] ffap-string-at-point: Limit max length of active region

Do not spend time checking large strings which are
likely not valid candidates (Bug#25243).
* lisp/ffap.el (ffap-max-region-length): New variable.
(ffap-string-at-point): Use it.
* test/lisp/ffap-tests.el: New test suite.
(ffap-tests-25243): Add test for this bug.
---
 lisp/ffap.el            | 25 ++++++++++++++++-------
 test/lisp/ffap-tests.el | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 7 deletions(-)
 create mode 100644 test/lisp/ffap-tests.el

diff --git a/lisp/ffap.el b/lisp/ffap.el
index 3d7cebadcf..99bb65faaf 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -203,6 +203,11 @@ ffap-foo-at-bar-prefix
 		 )
   :group 'ffap)
 
+(defvar ffap-max-region-length 1024
+  "Maximum active region length.
+When the region is active and larger than this value,
+`ffap-string-at-point' returns an empty string.")
+
 
 ;;; Peanut Gallery (More User Variables):
 ;;
@@ -1101,8 +1106,10 @@ ffap-string-at-point
 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'."
+Set the variable `ffap-string-at-point' and the variable
+`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)
@@ -1119,11 +1126,15 @@ ffap-string-at-point
 		(save-excursion
 		  (skip-chars-forward (car args))
 		  (skip-chars-backward (nth 2 args) pt)
-		  (point)))))
-    (setq ffap-string-at-point
-	  (buffer-substring-no-properties
-	   (setcar ffap-string-at-point-region beg)
-	   (setcar (cdr ffap-string-at-point-region) end)))))
+		  (point))))
+         (region-len (- (max beg end) (min beg end))))
+    (if (and (natnump ffap-max-region-length)
+             (< region-len ffap-max-region-length)) ; Bug#25243.
+        (setf ffap-string-at-point-region (list beg end)
+              ffap-string-at-point
+              (buffer-substring-no-properties beg end))
+      (setf ffap-string-at-point-region (list 1 1)
+            ffap-string-at-point ""))))
 
 (defun ffap-string-around ()
   ;; Sometimes useful to decide how to treat a string.
diff --git a/test/lisp/ffap-tests.el b/test/lisp/ffap-tests.el
new file mode 100644
index 0000000000..61fa891fe7
--- /dev/null
+++ b/test/lisp/ffap-tests.el
@@ -0,0 +1,54 @@
+;;; ffap-tests.el --- Test suite for ffap.el -*- lexical-binding: t -*-
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; Author: Tino Calancha <tino.calancha <at> gmail.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'ffap)
+
+(ert-deftest ffap-tests-25243 ()
+  "Test for http://debbugs.gnu.org/25243 ."
+  (let ((file (make-temp-file "test-Bug#25243")))
+    (unwind-protect
+        (with-temp-file file
+          (let ((str "diff --git b/lisp/ffap.el a/lisp/ffap.el
+index 3d7cebadcf..ad4b70d737 100644
+--- b/lisp/ffap.el
++++ a/lisp/ffap.el
+@@ -203,6 +203,9 @@ ffap-foo-at-bar-prefix
+"))
+            (transient-mark-mode 1)
+            (when (natnump ffap-max-region-length)
+              (insert
+               (concat
+                str
+                (make-string ffap-max-region-length #xa)
+                (format "%s ENDS HERE" file)))
+              (mark-whole-buffer)
+              (should (equal "" (ffap-string-at-point)))
+              (should (equal '(1 1) ffap-string-at-point-region)))))
+      (and (file-exists-p file) (delete-file file)))))
+
+(provide 'ffap-tests)
+
+;;; ffap-tests.el ends here
-- 
2.11.0

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.5)
 of 2016-12-23
Repository revision: eff901b8a39f42ddedf4c1db833b9071cae5962f

Message #28 received at 25243-done <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: 25243-done <at> debbugs.gnu.org
Subject: Re: bug#25243: 26.0.50;
 ffap-guesser very slow w/ region active in large diff files
Date: Fri, 30 Dec 2016 15:41:20 +0900

Tino Calancha <tino.calancha <at> gmail.com> writes:
> If i don't see further comments in a few days i will push
> the following patch to the master branch:
Pushed fix into master branch as commit c336420d9f

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.