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

[Dmitry Gutov]

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.

project-find-file-history-v1.diff
Description: Text Data

project-find-file-history-v2.diff
Description: Text Data