unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Drew Adams <drew.adams@oracle.com>
To: Tino Calancha <tino.calancha@gmail.com>, 25243@debbugs.gnu.org
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)	[thread overview]
Message-ID: <36cb0896-f437-41f6-92d1-1f8897ff141d@default> (raw)
In-Reply-To: <87k2at2t28.fsf@gmail.com>

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.





  reply	other threads:[~2016-12-21 16:22 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-21 15:35 bug#25243: 26.0.50; ffap-guesser very slow w/ region active in large diff files Tino Calancha
2016-12-21 16:22 ` Drew Adams [this message]
2016-12-22  4:31   ` Tino Calancha
2016-12-22 17:22     ` Drew Adams
2016-12-23  7:12       ` Tino Calancha
2016-12-23 15:41         ` Drew Adams
2016-12-24  2:53           ` Tino Calancha
2016-12-30  6:41             ` Tino Calancha

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=36cb0896-f437-41f6-92d1-1f8897ff141d@default \
    --to=drew.adams@oracle.com \
    --cc=25243@debbugs.gnu.org \
    --cc=tino.calancha@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).