bug#58447: [PATCH] In project-find-file, add absolute file name to history

[Dmitry Gutov <dgutov@yandex.ru>]

[-- 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