unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Dmitry Gutov <dgutov@yandex.ru>
To: Augusto Stoffel <arstoffel@gmail.com>
Cc: Eli Zaretskii <eliz@gnu.org>,
	58447@debbugs.gnu.org, Juri Linkov <juri@linkov.net>
Subject: bug#58447: [PATCH] In project-find-file, add absolute file name to history
Date: Thu, 15 Dec 2022 01:04:12 +0200	[thread overview]
Message-ID: <eaf94bae-983c-9d99-ed64-0329b5d7855c@yandex.ru> (raw)
In-Reply-To: <87cz8ln5lt.fsf@gmail.com>

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

On 14/12/2022 21:32, Augusto Stoffel wrote:
> On Wed, 14 Dec 2022 at 20:45, Dmitry Gutov wrote:
> 
>> On 14/12/2022 18:47, Augusto Stoffel wrote:
>>> On the other hand, your trick works by accident.  If you switch between
>>> unrelated projects, then 'C-x p f M-p' brings up a non-existing file.
>>> One might say each project should have its own history, but then it's
>>> not clear whether/when equally named projects in different locations
>>> should count as "the same" project.
>>
>> Perhaps the first step to resolving all this is for project-find-file
>> to use a different history variable than find-file.
> 
> This is fine by me, but do you feel confident such a variable will be
> a good design for the long run?  In particular, have you discarded the
> idea of per-project history variables?

Just spitballing. And on second thought: probably not.

It's iffy because of the notion of external-roots. The command 
project-or-external-find-file is supposed to reuse the same history, I 
think, and the file names there won't necessarily be relative, or 
relative to the project root. Even in project-find-file the names could 
be relative to a different (nested) dir if there are no files in the top 
dir, and it just has one subdirectory.

> The advantage of my suggestion (filter the file-name-history on the fly)
> is that no new variables need to be defined, so nothing needs to be
> obsoleted and phased out if we change our minds.

Something like let-binding the value of file-hame-history to a different 
value temporarily around completing-read?

That can work, though someone should benchmark it with a large project 
and history.

To resolve the absolute-relative divide, the filtering should be done on 
some higher level. Inside project-find-file-in, perhaps, where we still 
have access to the absolute names, and where we haven't dispatched to 
the configured project-read-file-name-function yet.

>> Which makes sense, given that one (usually) uses relative file names,
>> and the other -- absolute ones.
>>
>> Maybe project--read-file-absolute could continue using the current
>> variable, too.
>>
>> As a result, all projects will share history, for good and
>> bad. Perhaps next we could do something about that, e.g. if the
>> history iteration could allow pre-filtering, we could also filter out
>> non-existing files.
> 
> This kind of filtering would be slower than the one I proposed, and
> prohibitively slow over Tramp, I think.

We're probably talking about the same thing, if the filtering is going 
to use the list of files from project-files, rather than file-exists-p. 
In either case, the user could actually input a non-existent file (or 
file not in the completion table) which would fail that test. But 
they'll hopefully hit C-x C-s soon after.

I've fiddled with the code a little, and here are two different patches.

Patch v1 tries to indeed filter based on what project-files returns.

Problems:

* To call (member s all-files), both absolute file names have to be in 
the same format (i.e. abbreviated v. not). That would require us to 
mandate all project-files implementations, both built-in and 
third-party, to return abbreviated file names. It's a somewhat breaking 
change, and I'm not 100% sure the abbreviated form is useful in more 
situations than the expanded one.

* In a large project, where fetching all files using 'git ls-files' 
takes 1.007258s,

        (seq-filter
         (lambda (s) (member s all-files))
         file-name-history)

takes 0.137352s. That's not huge, but not insignificant either. 
Especially if we someday add some cache for the former computation. This 
could be avoided if 'history-prev-history' had some way to filter 
lazily. And history-item-predicate var, or some such.

* It seems like project-read-file-name-function functions will need to 
do some history conversions on their own anyway, because otherwise they 
insert absolute file names in the prompt.

With the latter in mind, here's a different take (patch v2), which just 
takes care of that.

The patches could be combined, but v1 seems to be too invasive for 
emacs-29, yet v2 could be just small enough to be considered "bugfix-only".

So, what does everyone think about the latter?

If people agree that the v2 patch is an improvement, we can check it in 
and leave project-local histories until later.

[-- Attachment #2: project-find-file-history-v1.diff --]
[-- Type: text/x-patch, Size: 3368 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 7cdaba9c07..d7d1740cfb 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -610,6 +610,7 @@ project--vc-list-files
   (pcase backend
     (`Git
      (let* ((default-directory (expand-file-name (file-name-as-directory dir)))
+            (abbr-dd (abbreviate-file-name default-directory))
             (args '("-z"))
             (vc-git-use-literal-pathspecs nil)
             (include-untracked (project--value-in-dir
@@ -647,7 +648,7 @@ project--vc-list-files
                                    extra-ignores)))))
        (setq files
              (mapcar
-              (lambda (file) (concat default-directory file))
+              (lambda (file) (concat abbr-dd file))
               (split-string
                (apply #'vc-git--run-command-string nil "ls-files" args)
                "\0" t)))
@@ -670,6 +671,7 @@ project--vc-list-files
        (delete-consecutive-dups files)))
     (`Hg
      (let* ((default-directory (expand-file-name (file-name-as-directory dir)))
+            (abbr-dd (abbreviate-file-name default-directory))
             (include-untracked (project--value-in-dir
                                 'project-vc-include-untracked
                                 dir))
@@ -685,7 +687,7 @@ project--vc-list-files
        (with-temp-buffer
          (apply #'vc-hg-command t 0 "." "status" args)
          (mapcar
-          (lambda (s) (concat default-directory s))
+          (lambda (s) (concat abbr-dd s))
           (split-string (buffer-string) "\0" t)))))))
 
 (defun project--vc-merge-submodules-p (dir)
@@ -1042,11 +1044,8 @@ project--read-file-cpd-relative
                     (project--completing-read-strict prompt
                                                      new-collection
                                                      predicate
-                                                     hist mb-default)))
-         (absname (expand-file-name relname common-parent-directory)))
-    (when (and hist history-add-new-input)
-      (add-to-history hist (abbreviate-file-name absname)))
-    absname))
+                                                     hist mb-default))))
+    (expand-file-name relname common-parent-directory)))
 
 (defun project--read-file-absolute (prompt
                                     all-files &optional predicate
@@ -1076,9 +1075,16 @@ project-find-file-in
                dirs)
             (project-files project dirs)))
          (completion-ignore-case read-file-name-completion-ignore-case)
-         (file (funcall project-read-file-name-function
-                        "Find file" all-files nil 'file-name-history
-                        suggested-filename)))
+         (file
+          (let ((file-name-history (seq-filter
+                                    (lambda (s) (member s all-files))
+                                    file-name-history))
+                (history-add-new-input nil))
+            (funcall project-read-file-name-function
+                     "Find file" all-files nil 'file-name-history
+                     suggested-filename))))
+    (when history-add-new-input
+      (add-to-history 'file-name-history (abbreviate-file-name file)))
     (if (string= file "")
         (user-error "You didn't specify the file")
       (find-file file))))

[-- Attachment #3: project-find-file-history-v2.diff --]
[-- Type: text/x-patch, Size: 1087 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 7cdaba9c07..47eb9f9982 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1038,7 +1038,14 @@ project--read-file-cpd-relative
          (_ (when included-cpd
               (setq substrings (cons "./" substrings))))
          (new-collection (project--file-completion-table substrings))
-         (relname (let ((history-add-new-input nil))
+         (abbr-cpd (abbreviate-file-name common-parent-directory))
+         (relname (cl-letf ((history-add-new-input nil)
+                            ((symbol-value hist)
+                             (mapcan
+                              (lambda (s)
+                                (and (string-prefix-p abbr-cpd s)
+                                     (list (substring s (length abbr-cpd)))))
+                              (symbol-value hist))))
                     (project--completing-read-strict prompt
                                                      new-collection
                                                      predicate

  reply	other threads:[~2022-12-14 23:04 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-11 18:29 bug#58447: [PATCH] In project-find-file, add absolute file name to history Augusto Stoffel
2022-10-11 19:13 ` Eli Zaretskii
2022-10-26  9:04   ` Augusto Stoffel
2022-10-27  8:44     ` Dmitry Gutov
2022-10-27 13:15       ` Eli Zaretskii
2022-10-27 14:21         ` Dmitry Gutov
2022-10-27 14:26           ` Augusto Stoffel
2022-10-27 15:36             ` Dmitry Gutov
2022-10-27 15:56               ` Augusto Stoffel
2022-10-27 16:07                 ` Dmitry Gutov
2022-10-27 15:52           ` Eli Zaretskii
2022-10-27 16:10             ` Dmitry Gutov
2022-10-27 16:37               ` Augusto Stoffel
2022-10-27 16:48                 ` Dmitry Gutov
2022-10-27 16:53                   ` Augusto Stoffel
2022-10-27 17:34                     ` Dmitry Gutov
2022-10-27 17:48                       ` Augusto Stoffel
2022-10-27 18:05                         ` Dmitry Gutov
2022-10-27 18:12                           ` Augusto Stoffel
2022-10-27 19:51                             ` Dmitry Gutov
2022-10-27 19:41                 ` Juri Linkov
2022-10-28 18:11                   ` Dmitry Gutov
2022-10-29 17:45                     ` Juri Linkov
2022-10-30 19:42                       ` Dmitry Gutov
2022-10-31  8:06                         ` Augusto Stoffel
2022-11-01 17:30                           ` Juri Linkov
2022-11-01 22:14                             ` Dmitry Gutov
2022-11-18 16:28                               ` Augusto Stoffel
2022-11-25 13:39                                 ` Eli Zaretskii
2022-11-26  2:28                                 ` Dmitry Gutov
2022-11-28 22:58                                   ` Augusto Stoffel
2022-11-29 16:29                                     ` Dmitry Gutov
2022-12-09  7:38                             ` Juri Linkov
2022-12-09 13:00                               ` Dmitry Gutov
2022-12-10 17:27                                 ` Juri Linkov
2022-12-10 19:50                                   ` Dmitry Gutov
2022-12-11 17:57                                     ` Juri Linkov
2022-12-11 18:14                                       ` Dmitry Gutov
2022-12-12 17:41                                         ` Juri Linkov
2022-12-12 18:49                                           ` Augusto Stoffel
2022-12-13 17:28                                             ` Juri Linkov
2022-12-14 16:47                                               ` Augusto Stoffel
2022-12-14 18:45                                                 ` Dmitry Gutov
2022-12-14 19:32                                                   ` Augusto Stoffel
2022-12-14 23:04                                                     ` Dmitry Gutov [this message]
2022-12-15  7:24                                                       ` Juri Linkov
2022-12-15 11:07                                                         ` Augusto Stoffel
2022-12-15 14:04                                                           ` Dmitry Gutov
2022-12-15 13:54                                                         ` Dmitry Gutov
2022-12-15 17:24                                                           ` Juri Linkov
2022-12-15 22:20                                                             ` Dmitry Gutov
2022-12-16  7:48                                                               ` Juri Linkov
2022-12-18  0:32                                                                 ` Dmitry Gutov
2022-12-15 11:07                                                       ` Augusto Stoffel
2022-12-15 14:08                                                         ` Dmitry Gutov
2022-12-15 17:21                                                           ` Juri Linkov
2022-12-15 22:50                                                             ` Dmitry Gutov
2022-12-18  8:36                                                               ` Juri Linkov
2022-12-18 11:51                                                                 ` Dmitry Gutov
2022-12-19 17:55                                                                   ` Juri Linkov
2022-12-19 18:01                                                                     ` Eli Zaretskii
2022-12-19 19:48                                                                       ` Juri Linkov
2022-12-19 20:00                                                                         ` Eli Zaretskii
2022-12-19 19:18                                                                     ` Dmitry Gutov

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=eaf94bae-983c-9d99-ed64-0329b5d7855c@yandex.ru \
    --to=dgutov@yandex.ru \
    --cc=58447@debbugs.gnu.org \
    --cc=arstoffel@gmail.com \
    --cc=eliz@gnu.org \
    --cc=juri@linkov.net \
    /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).