[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#25243: 26.0.50; ffap-guesser very slow w/ region active in large dif

From: Drew Adams
Subject: 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

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:

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]