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

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

[Augusto Stoffel]

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

Tags: patch

`project-find-file' and related commands share their history with more
general commands like `find-file', so they should add the absolute file
name to history.  Currently, file names relative to the project root are
added.  The attached patch fixes this.

I've also replaced `concat' with `expand-file-name' at a certain point.
I see no reason why one shouldn't do that, but please have a look.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-In-project-find-file-and-the-like-add-absolute-file-.patch --]
[-- Type: text/patch, Size: 1724 bytes --]

From 699732687f559c7984e76310971094f251423d5d Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Tue, 11 Oct 2022 20:21:34 +0200
Subject: [PATCH] In project-find-file and the like, add absolute file name to
 history

* lisp/progmodes/project.el (project--read-file-cpd-relative): Add
absolute file name to history.
---
 lisp/progmodes/project.el | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index ee94d0d85d..95087ae6a8 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -931,11 +931,15 @@ project--read-file-cpd-relative
          (_ (when included-cpd
               (setq substrings (cons "./" substrings))))
          (new-collection (project--file-completion-table substrings))
-         (res (project--completing-read-strict prompt
-                                               new-collection
-                                               predicate
-                                               hist mb-default)))
-    (concat common-parent-directory res)))
+         (relname (let ((history-add-new-input nil))
+                    (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 absname))
+    absname))
 
 (defun project--read-file-absolute (prompt
                                     all-files &optional predicate
-- 
2.37.3

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

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Date: Tue, 11 Oct 2022 20:29:24 +0200
> 
> `project-find-file' and related commands share their history with more
> general commands like `find-file', so they should add the absolute file
> name to history.  Currently, file names relative to the project root are
> added.  The attached patch fixes this.

Shouldn't project.el have its separate history of file names, perhaps
even a project-specific history?

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

[Augusto Stoffel]

I'm copying Dmitry.

On Tue, 11 Oct 2022 at 22:13, Eli Zaretskii wrote:

>> From: Augusto Stoffel <arstoffel@gmail.com>
>> Date: Tue, 11 Oct 2022 20:29:24 +0200
>> 
>> `project-find-file' and related commands share their history with more
>> general commands like `find-file', so they should add the absolute file
>> name to history.  Currently, file names relative to the project root are
>> added.  The attached patch fixes this.
>
> Shouldn't project.el have its separate history of file names,

I'm not sure that's useful, but then the history items should be
absolute file names anyway.

> perhaps even a project-specific history?

This might make sense.

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

[Dmitry Gutov]

On 26.10.2022 12:04, Augusto Stoffel wrote:
> I'm copying Dmitry.

Hi!

> On Tue, 11 Oct 2022 at 22:13, Eli Zaretskii wrote:
> 
>>> From: Augusto Stoffel<arstoffel@gmail.com>
>>> Date: Tue, 11 Oct 2022 20:29:24 +0200
>>>
>>> `project-find-file' and related commands share their history with more
>>> general commands like `find-file', so they should add the absolute file
>>> name to history.  Currently, file names relative to the project root are
>>> added.  The attached patch fixes this.
>> Shouldn't project.el have its separate history of file names,
> I'm not sure that's useful, but then the history items should be
> absolute file names anyway.
> 
>> perhaps even a project-specific history?
> This might make sense.

I like both of these ideas, except I'm not sure how to implement the 
project-specific part better.

I suppose we won't be passing the history var to completing-read anymore?

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

[Eli Zaretskii]

> Date: Thu, 27 Oct 2022 11:44:09 +0300
> Cc: 58447@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 26.10.2022 12:04, Augusto Stoffel wrote:
> > I'm copying Dmitry.
> 
> Hi!
> 
> > On Tue, 11 Oct 2022 at 22:13, Eli Zaretskii wrote:
> > 
> >>> From: Augusto Stoffel<arstoffel@gmail.com>
> >>> Date: Tue, 11 Oct 2022 20:29:24 +0200
> >>>
> >>> `project-find-file' and related commands share their history with more
> >>> general commands like `find-file', so they should add the absolute file
> >>> name to history.  Currently, file names relative to the project root are
> >>> added.  The attached patch fixes this.
> >> Shouldn't project.el have its separate history of file names,
> > I'm not sure that's useful, but then the history items should be
> > absolute file names anyway.
> > 
> >> perhaps even a project-specific history?
> > This might make sense.
> 
> I like both of these ideas, except I'm not sure how to implement the 
> project-specific part better.
> 
> I suppose we won't be passing the history var to completing-read anymore?

I thought about a project-specific history variable.

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

[Dmitry Gutov]

On 27.10.2022 16:15, Eli Zaretskii wrote:
>> Date: Thu, 27 Oct 2022 11:44:09 +0300
>> Cc:58447@debbugs.gnu.org
>> From: Dmitry Gutov<dgutov@yandex.ru>
>>
>> On 26.10.2022 12:04, Augusto Stoffel wrote:
>>> I'm copying Dmitry.
>> Hi!
>>
>>> On Tue, 11 Oct 2022 at 22:13, Eli Zaretskii wrote:
>>>
>>>>> From: Augusto Stoffel<arstoffel@gmail.com>
>>>>> Date: Tue, 11 Oct 2022 20:29:24 +0200
>>>>>
>>>>> `project-find-file' and related commands share their history with more
>>>>> general commands like `find-file', so they should add the absolute file
>>>>> name to history.  Currently, file names relative to the project root are
>>>>> added.  The attached patch fixes this.
>>>> Shouldn't project.el have its separate history of file names,
>>> I'm not sure that's useful, but then the history items should be
>>> absolute file names anyway.
>>>
>>>> perhaps even a project-specific history?
>>> This might make sense.
>> I like both of these ideas, except I'm not sure how to implement the
>> project-specific part better.
>>
>> I suppose we won't be passing the history var to completing-read anymore?
> I thought about a project-specific history variable.

...one global variable per project?

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

[Augusto Stoffel]

On Thu, 27 Oct 2022 at 17:21, Dmitry Gutov wrote:

> On 27.10.2022 16:15, Eli Zaretskii wrote:
>>> Date: Thu, 27 Oct 2022 11:44:09 +0300
>>> Cc:58447@debbugs.gnu.org
>>> From: Dmitry Gutov<dgutov@yandex.ru>
>>>
>>> On 26.10.2022 12:04, Augusto Stoffel wrote:
>>>> I'm copying Dmitry.
>>> Hi!
>>>
>>>> On Tue, 11 Oct 2022 at 22:13, Eli Zaretskii wrote:
>>>>
>>>>>> From: Augusto Stoffel<arstoffel@gmail.com>
>>>>>> Date: Tue, 11 Oct 2022 20:29:24 +0200
>>>>>>
>>>>>> `project-find-file' and related commands share their history with more
>>>>>> general commands like `find-file', so they should add the absolute file
>>>>>> name to history.  Currently, file names relative to the project root are
>>>>>> added.  The attached patch fixes this.
>>>>> Shouldn't project.el have its separate history of file names,
>>>> I'm not sure that's useful, but then the history items should be
>>>> absolute file names anyway.
>>>>
>>>>> perhaps even a project-specific history?
>>>> This might make sense.
>>> I like both of these ideas, except I'm not sure how to implement the
>>> project-specific part better.
>>>
>>> I suppose we won't be passing the history var to completing-read anymore?
>> I thought about a project-specific history variable.
>
> ...one global variable per project?

One thing to consider here is compatibility with savehist-mode and the
like.  I guess the only simple way to achieve this is with one global
variable per project.  And then there would be the issue of when to
garbage-collect the saved save pertaining to old projects.

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

[Dmitry Gutov]

On 27.10.2022 17:26, Augusto Stoffel wrote:
> One thing to consider here is compatibility with savehist-mode and the
> like.  I guess the only simple way to achieve this is with one global
> variable per project.

I suppose interning a symbol with the project root directory in its name 
could work.

Passing it to completing-read will unavoidably result in relative names 
still, though. But it won't be so much of a problem anymore.

> And then there would be the issue of when to
> garbage-collect the saved save pertaining to old projects.

Inside project--remove-from-project-list? Either one specific project, 
or doing a full scan of obarray.

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

[Eli Zaretskii]

> Date: Thu, 27 Oct 2022 17:21:24 +0300
> Cc: arstoffel@gmail.com, 58447@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> >> I like both of these ideas, except I'm not sure how to implement the
> >> project-specific part better.
> >>
> >> I suppose we won't be passing the history var to completing-read anymore?
> > I thought about a project-specific history variable.
> 
> ...one global variable per project?

Yes, I think so.  Are there any downsides that I overlooked?

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

[Augusto Stoffel]

On Thu, 27 Oct 2022 at 18:36, Dmitry Gutov wrote:

> On 27.10.2022 17:26, Augusto Stoffel wrote:
>> One thing to consider here is compatibility with savehist-mode and the
>> like.  I guess the only simple way to achieve this is with one global
>> variable per project.
>
> I suppose interning a symbol with the project root directory in its
> name could work.

The project root directory or the project name?  I'd say the latter.

If you have two copies of the same project in two different places
(which I often have -- a local copy and one in a remote compute
machine), then the shell and compilation buffers are shared.  One can
argue whether this is good or bad (I find it okay, with some caveats),
but for the sake of consistency the history variable should also be
shared among copies of the same project.

> Passing it to completing-read will unavoidably result in relative
> names still, though. But it won't be so much of a problem anymore.

If we settle for one history variable per project, then I'd say yes, the
entries should be relative file names.

>> And then there would be the issue of when to
>> garbage-collect the saved save pertaining to old projects.
>
> Inside project--remove-from-project-list? Either one specific project,
> or doing a full scan of obarray.

Okay, if "garbage collection" of projects is already a thing, then we
just have to extend it to contemplate the history variable.

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

[Dmitry Gutov]

On 27.10.2022 18:56, Augusto Stoffel wrote:
> On Thu, 27 Oct 2022 at 18:36, Dmitry Gutov wrote:
> 
>> On 27.10.2022 17:26, Augusto Stoffel wrote:
>>> One thing to consider here is compatibility with savehist-mode and the
>>> like.  I guess the only simple way to achieve this is with one global
>>> variable per project.
>> I suppose interning a symbol with the project root directory in its
>> name could work.
> The project root directory or the project name?  I'd say the latter.

We don't have a "project name" generic yet. There is a bug open.

And even so, unrelated projects could declare equal names, couldn't they?

> If you have two copies of the same project in two different places
> (which I often have -- a local copy and one in a remote compute
> machine), then the shell and compilation buffers are shared.  One can
> argue whether this is good or bad (I find it okay, with some caveats),
> but for the sake of consistency the history variable should also be
> shared among copies of the same project.

I imagined the project names would be used in the project selector 
instead of their root directories, not in addition to them. So they'd 
have to be unique even across "copied projects".

But you can bring up that idea in bug#48747.

>> Passing it to completing-read will unavoidably result in relative
>> names still, though. But it won't be so much of a problem anymore.
> If we settle for one history variable per project, then I'd say yes, the
> entries should be relative file names.

Ok.

>>> And then there would be the issue of when to
>>> garbage-collect the saved save pertaining to old projects.
>> Inside project--remove-from-project-list? Either one specific project,
>> or doing a full scan of obarray.
> Okay, if "garbage collection" of projects is already a thing, then we
> just have to extend it to contemplate the history variable.

There is no "garbage collection" point exactly, but we could pivot to 
making one.

The main reason I didn't do that is scanning the full project list with 
file-readable-p check will likely become slow quickly enough, especially 
in the presence of remote projects.

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

[Dmitry Gutov]

On 27.10.2022 18:52, Eli Zaretskii wrote:
>> Date: Thu, 27 Oct 2022 17:21:24 +0300
>> Cc:arstoffel@gmail.com,58447@debbugs.gnu.org
>> From: Dmitry Gutov<dgutov@yandex.ru>
>>
>>>> I like both of these ideas, except I'm not sure how to implement the
>>>> project-specific part better.
>>>>
>>>> I suppose we won't be passing the history var to completing-read anymore?
>>> I thought about a project-specific history variable.
>> ...one global variable per project?
> Yes, I think so.  Are there any downsides that I overlooked?

The idea of having a multitude of similar variables gave me a pause. But 
it's not a deal-breaker, I suppose.

Also a couple of questions we're talking about in the other branch.

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

[Augusto Stoffel]

On Thu, 27 Oct 2022 at 19:10, Dmitry Gutov wrote:

> The idea of having a multitude of similar variables gave me a
> pause. But it's not a deal-breaker, I suppose.
>
> Also a couple of questions we're talking about in the other branch.

I think the idea of per-project histories is an interesting feature that
requires some careful design, but note that there is a bug in
project-find-file that should ideally be fixed for Emacs 29 (namely, it
pollutes the find-file history by adding non-absolute file names to it).

The patch in my original message fixes this and shouldn't complicate a
future implementation of the per-project histories.

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

[Dmitry Gutov]

On 27.10.2022 19:37, Augusto Stoffel wrote:
> The patch in my original message fixes this and shouldn't complicate a
> future implementation of the per-project histories.

OK. Are you sure that it doesn't add the relative name to the history still?

AFAIU as long as hist is passed to project--completing-read-strict,  the 
completing-read call in there will alter the history first.

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

[Augusto Stoffel]

On Thu, 27 Oct 2022 at 19:48, Dmitry Gutov wrote:

> On 27.10.2022 19:37, Augusto Stoffel wrote:
>> The patch in my original message fixes this and shouldn't complicate a
>> future implementation of the per-project histories.
>
> OK. Are you sure that it doesn't add the relative name to the history still?
>
> AFAIU as long as hist is passed to project--completing-read-strict,
> the completing-read call in there will alter the history first.

Note that history-add-new-input is bound to nil at the appropriate
place in the patch.

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

[Dmitry Gutov]

On 27.10.2022 19:53, Augusto Stoffel wrote:
> On Thu, 27 Oct 2022 at 19:48, Dmitry Gutov wrote:
> 
>> On 27.10.2022 19:37, Augusto Stoffel wrote:
>>> The patch in my original message fixes this and shouldn't complicate a
>>> future implementation of the per-project histories.
>> OK. Are you sure that it doesn't add the relative name to the history still?
>>
>> AFAIU as long as hist is passed to project--completing-read-strict,
>> the completing-read call in there will alter the history first.
> Note that history-add-new-input is bound to nil at the appropriate
> place in the patch.

Ah, ok.

But how is completing-read using the history var? Only reading past 
inputs? Can it handle the absolute values there?

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

[Augusto Stoffel]

On Thu, 27 Oct 2022 at 20:34, Dmitry Gutov wrote:

> On 27.10.2022 19:53, Augusto Stoffel wrote:
>> On Thu, 27 Oct 2022 at 19:48, Dmitry Gutov wrote:
>> 
>>> On 27.10.2022 19:37, Augusto Stoffel wrote:
>>>> The patch in my original message fixes this and shouldn't complicate a
>>>> future implementation of the per-project histories.
>>> OK. Are you sure that it doesn't add the relative name to the history still?
>>>
>>> AFAIU as long as hist is passed to project--completing-read-strict,
>>> the completing-read call in there will alter the history first.
>> Note that history-add-new-input is bound to nil at the appropriate
>> place in the patch.
>
> Ah, ok.
>
> But how is completing-read using the history var? Only reading past
> inputs?

Yes, completing-read will make past inputs available for selection but
not updated the history variable.  Then, a couple lines later in the
patch, we update the history manually:

    (when (and hist history-add-new-input)
      (add-to-history hist absname))

> Can it handle the absolute values there?

The completing read reads a file name, the relname; if it's relative,
then it's expanded to an absolute name (the absname).  If for whatever
reason the relname was already absolute, then absname will be equal to
relname (what else could you do, right?)

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

[Dmitry Gutov]

On 27.10.2022 20:48, Augusto Stoffel wrote:
>> Can it handle the absolute values there?
> The completing read reads a file name, the relname; if it's relative,
> then it's expanded to an absolute name (the absname).  If for whatever
> reason the relname was already absolute, then absname will be equal to
> relname (what else could you do, right?)

The completion table won't match absolute names. But okay, require-match 
is nil there, so any arbitrary input will be accepted.

But what if that absolute name comes from a different project?

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

[Augusto Stoffel]

On Thu, 27 Oct 2022 at 21:05, Dmitry Gutov wrote:

> On 27.10.2022 20:48, Augusto Stoffel wrote:
>>> Can it handle the absolute values there?
>> The completing read reads a file name, the relname; if it's relative,
>> then it's expanded to an absolute name (the absname).  If for whatever
>> reason the relname was already absolute, then absname will be equal to
>> relname (what else could you do, right?)
>
> The completion table won't match absolute names. But okay,
> require-match is nil there, so any arbitrary input will be accepted.

Right.

> But what if that absolute name comes from a different project?

In my patch it will just find that file.  Alternatively we could throw
an error, but I don't see a point in doing that.

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

[Juri Linkov]

> I think the idea of per-project histories is an interesting feature that
> requires some careful design, but note that there is a bug in
> project-find-file that should ideally be fixed for Emacs 29 (namely, it
> pollutes the find-file history by adding non-absolute file names to it).
>
> The patch in my original message fixes this and shouldn't complicate a
> future implementation of the per-project histories.

Would be nice to fix this.  The problem that I have is that when a file
visited by 'C-x p f non-absolute/filename' needs to be revisited later,
then the quickest way is with 'C-x C-f M-p', but currently it fails
with non-absolute file names.

But please note that the same problem exists in both ways:
when trying to revisit an absolute file names previously visited
by 'C-x C-f', then 'C-x p f' fails on absolute file names.

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

[Dmitry Gutov]

On 27.10.2022 21:12, Augusto Stoffel wrote:
>> But what if that absolute name comes from a different project?
> In my patch it will just find that file.  Alternatively we could throw
> an error, but I don't see a point in doing that.

OK, if that is the behavior you want, I don't mind.

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

[Dmitry Gutov]

Hi Juri,

On 27.10.2022 22:41, Juri Linkov wrote:
> But please note that the same problem exists in both ways:
> when trying to revisit an absolute file names previously visited
> by 'C-x C-f', then 'C-x p f' fails on absolute file names.

Does Augusto's patch from the first message make things better for you?

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

[Juri Linkov]

>> But please note that the same problem exists in both ways:
>> when trying to revisit an absolute file names previously visited
>> by 'C-x C-f', then 'C-x p f' fails on absolute file names.
>
> Does Augusto's patch from the first message make things better for you?

Augusto's patch makes things better, but fails to read absolute file names.
To fix this, project--read-file-cpd-relative also need to accept absolute
file names.

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

[Dmitry Gutov]

On 29.10.2022 20:45, Juri Linkov wrote:
>>> But please note that the same problem exists in both ways:
>>> when trying to revisit an absolute file names previously visited
>>> by 'C-x C-f', then 'C-x p f' fails on absolute file names.
>> Does Augusto's patch from the first message make things better for you?
> Augusto's patch makes things better, but fails to read absolute file names.
> To fix this, project--read-file-cpd-relative also need to accept absolute
> file names.

Augusto, what do you say to that?

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

[Augusto Stoffel]

On Sun, 30 Oct 2022 at 21:42, Dmitry Gutov wrote:

> On 29.10.2022 20:45, Juri Linkov wrote:
>>>> But please note that the same problem exists in both ways:
>>>> when trying to revisit an absolute file names previously visited
>>>> by 'C-x C-f', then 'C-x p f' fails on absolute file names.
>>> Does Augusto's patch from the first message make things better for you?
>> Augusto's patch makes things better, but fails to read absolute file names.
>> To fix this, project--read-file-cpd-relative also need to accept absolute
>> file names.
>
> Augusto, what do you say to that?

I'm not quite sure what Juri meant here.  If from some project I do

  C-x p f /tmp RET RET

then Emacs opens the /tmp dired buffer (RET twice because REQUIRE-MATCH
is 'confirm), as I think it should.

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

[Juri Linkov]

> I'm not quite sure what Juri meant here.  If from some project I do
>
>   C-x p f /tmp RET RET
>
> then Emacs opens the /tmp dired buffer (RET twice because REQUIRE-MATCH
> is 'confirm), as I think it should.

Actually, that [Confirm] part is not quite a hassle, so your patch
already handles that in both ways, thanks for this.

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

[Dmitry Gutov]

On 01.11.2022 19:30, Juri Linkov wrote:
>> I'm not quite sure what Juri meant here.  If from some project I do
>>
>>    C-x p f /tmp RET RET
>>
>> then Emacs opens the /tmp dired buffer (RET twice because REQUIRE-MATCH
>> is 'confirm), as I think it should.
> Actually, that [Confirm] part is not quite a hassle, so your patch
> already handles that in both ways, thanks for this.

Thanks for verifying.

LGTM then.

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

[Augusto Stoffel]

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

On Wed,  2 Nov 2022 at 00:14, Dmitry Gutov wrote:

> On 01.11.2022 19:30, Juri Linkov wrote:
>>> I'm not quite sure what Juri meant here.  If from some project I do
>>>
>>>    C-x p f /tmp RET RET
>>>
>>> then Emacs opens the /tmp dired buffer (RET twice because REQUIRE-MATCH
>>> is 'confirm), as I think it should.
>> Actually, that [Confirm] part is not quite a hassle, so your patch
>> already handles that in both ways, thanks for this.
>
> Thanks for verifying.
>
> LGTM then.

FTR, someone would have to install the patch on my behalf.  I've
attached it again for your convenience.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-In-project-find-file-and-the-like-add-absolute-file-.patch --]
[-- Type: text/x-patch, Size: 1724 bytes --]

From 699732687f559c7984e76310971094f251423d5d Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Tue, 11 Oct 2022 20:21:34 +0200
Subject: [PATCH] In project-find-file and the like, add absolute file name to
 history

* lisp/progmodes/project.el (project--read-file-cpd-relative): Add
absolute file name to history.
---
 lisp/progmodes/project.el | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index ee94d0d85d..95087ae6a8 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -931,11 +931,15 @@ project--read-file-cpd-relative
          (_ (when included-cpd
               (setq substrings (cons "./" substrings))))
          (new-collection (project--file-completion-table substrings))
-         (res (project--completing-read-strict prompt
-                                               new-collection
-                                               predicate
-                                               hist mb-default)))
-    (concat common-parent-directory res)))
+         (relname (let ((history-add-new-input nil))
+                    (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 absname))
+    absname))
 
 (defun project--read-file-absolute (prompt
                                     all-files &optional predicate
-- 
2.37.3

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

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: Juri Linkov <juri@linkov.net>,  Eli Zaretskii <eliz@gnu.org>,
>   58447@debbugs.gnu.org
> Date: Fri, 18 Nov 2022 11:28:07 -0500
> 
> On Wed,  2 Nov 2022 at 00:14, Dmitry Gutov wrote:
> 
> > On 01.11.2022 19:30, Juri Linkov wrote:
> >>> I'm not quite sure what Juri meant here.  If from some project I do
> >>>
> >>>    C-x p f /tmp RET RET
> >>>
> >>> then Emacs opens the /tmp dired buffer (RET twice because REQUIRE-MATCH
> >>> is 'confirm), as I think it should.
> >> Actually, that [Confirm] part is not quite a hassle, so your patch
> >> already handles that in both ways, thanks for this.
> >
> > Thanks for verifying.
> >
> > LGTM then.
> 
> FTR, someone would have to install the patch on my behalf.  I've
> attached it again for your convenience.

Thanks.  Dmitry, should this go in?

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

[Dmitry Gutov]

On 18/11/22 18:28, Augusto Stoffel wrote:
> On Wed,  2 Nov 2022 at 00:14, Dmitry Gutov wrote:
> 
>> On 01.11.2022 19:30, Juri Linkov wrote:
>>>> I'm not quite sure what Juri meant here.  If from some project I do
>>>>
>>>>     C-x p f /tmp RET RET
>>>>
>>>> then Emacs opens the /tmp dired buffer (RET twice because REQUIRE-MATCH
>>>> is 'confirm), as I think it should.
>>> Actually, that [Confirm] part is not quite a hassle, so your patch
>>> already handles that in both ways, thanks for this.
>> Thanks for verifying.
>>
>> LGTM then.
> FTR, someone would have to install the patch on my behalf.  I've
> attached it again for your convenience.

Sorry, I figured you had commit access.

Now pushed, thanks.

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

[Augusto Stoffel]

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

On Sat, 26 Nov 2022 at 04:28, Dmitry Gutov wrote:

> Sorry, I figured you had commit access.
>
> Now pushed, thanks.

It has been brought to my attention that I should have used
`abbreviate-file-name' for file name history items.  Unfortunately we
need to push a small fix.  Hopefully this can go in Emacs 29.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-In-project-find-file-add-abbreviated-file-names-to-h.patch --]
[-- Type: text/x-patch, Size: 988 bytes --]

From 19957ae6b2db0c40de37c7c290a7113b0b33e6b7 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Mon, 28 Nov 2022 23:50:41 +0100
Subject: [PATCH] In project-find-file, add abbreviated file names to history

* lisp/progmodes/project.el (project--read-file-cpd-relative): Use
'abbreviate-file-name'.
---
 lisp/progmodes/project.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 5b8648031f..3d567b42a0 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -954,7 +954,7 @@ project--read-file-cpd-relative
                                                      hist mb-default)))
          (absname (expand-file-name relname common-parent-directory)))
     (when (and hist history-add-new-input)
-      (add-to-history hist absname))
+      (add-to-history hist (abbreviate-file-name absname)))
     absname))
 
 (defun project--read-file-absolute (prompt
-- 
2.38.1

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

[Dmitry Gutov]

On 29/11/2022 00:58, Augusto Stoffel wrote:
> On Sat, 26 Nov 2022 at 04:28, Dmitry Gutov wrote:
> 
>> Sorry, I figured you had commit access.
>>
>> Now pushed, thanks.
> It has been brought to my attention that I should have used
> `abbreviate-file-name' for file name history items.  Unfortunately we
> need to push a small fix.  Hopefully this can go in Emacs 29.

Looks indeed like a bugfix to me.

Pushed to emacs-29, thanks.

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

[Juri Linkov]

>> I'm not quite sure what Juri meant here.  If from some project I do
>>
>>   C-x p f /tmp RET RET
>>
>> then Emacs opens the /tmp dired buffer (RET twice because REQUIRE-MATCH
>> is 'confirm), as I think it should.
>
> Actually, that [Confirm] part is not quite a hassle, so your patch
> already handles that in both ways, thanks for this.

After stumbling on this confirmation snag every time while using 'C-x p f M-p',
I have to admit this is still a hassle that needs to be avoided in some way.

One solution would be to write a confirmation function instead of using
the symbol 'confirm' for completing-read in project--completing-read-strict.
Such a function could check if the filename is absolute and not require
confirmation in this case.

Another solution would be to add absolute filenames to the history
'file-name-history' used by 'C-x C-f'.  Then add relative filenames
to another history e.g. 'project-file-name-history' that will be used
only by 'C-x p f'.

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

[Dmitry Gutov]

On 09/12/2022 09:38, Juri Linkov wrote:
> One solution would be to write a confirmation function instead of using
> the symbol 'confirm' for completing-read in project--completing-read-strict.
> Such a function could check if the filename is absolute

And exists, I suppose?

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

[Juri Linkov]

>> One solution would be to write a confirmation function instead of using
>> the symbol 'confirm' for completing-read in project--completing-read-strict.
>> Such a function could check if the filename is absolute
>
> And exists, I suppose?

I'd prefer to have separate histories: with absolute filenames for C-x C-f,
and project-relative filenames for C-x p f.

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

[Dmitry Gutov]

On 10/12/2022 19:27, Juri Linkov wrote:
>>> One solution would be to write a confirmation function instead of using
>>> the symbol 'confirm' for completing-read in project--completing-read-strict.
>>> Such a function could check if the filename is absolute
>>
>> And exists, I suppose?
> 
> I'd prefer to have separate histories: with absolute filenames for C-x C-f,
> and project-relative filenames for C-x p f.

Yeah, ok.

I think we just postponed this improvement because we might not manage 
it in time for the 29's release.

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

[Juri Linkov]

>>>> One solution would be to write a confirmation function instead of using
>>>> the symbol 'confirm' for completing-read in project--completing-read-strict.
>>>> Such a function could check if the filename is absolute
>>>
>>> And exists, I suppose?
>> I'd prefer to have separate histories: with absolute filenames for C-x
>> C-f,
>> and project-relative filenames for C-x p f.
>
> Yeah, ok.
>
> I think we just postponed this improvement because we might not manage it
> in time for the 29's release.

Unfortunately, this makes the emacs-29 release unusable.

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

[Dmitry Gutov]

On 11/12/2022 19:57, Juri Linkov wrote:
>>>>> One solution would be to write a confirmation function instead of using
>>>>> the symbol 'confirm' for completing-read in project--completing-read-strict.
>>>>> Such a function could check if the filename is absolute
>>>> And exists, I suppose?
>>> I'd prefer to have separate histories: with absolute filenames for C-x
>>> C-f,
>>> and project-relative filenames for C-x p f.
>> Yeah, ok.
>>
>> I think we just postponed this improvement because we might not manage it
>> in time for the 29's release.
> Unfortunately, this makes the emacs-29 release unusable.

Would you say the new behavior is worse than what we had before?

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

[Juri Linkov]

>>>>>> One solution would be to write a confirmation function instead of using
>>>>>> the symbol 'confirm' for completing-read in project--completing-read-strict.
>>>>>> Such a function could check if the filename is absolute
>>>>> And exists, I suppose?
>>>> I'd prefer to have separate histories: with absolute filenames for C-x
>>>> C-f,
>>>> and project-relative filenames for C-x p f.
>>> Yeah, ok.
>>>
>>> I think we just postponed this improvement because we might not manage it
>>> in time for the 29's release.
>> Unfortunately, this makes the emacs-29 release unusable.
>
> Would you say the new behavior is worse than what we had before?

Unfortunately, yes.  It turned out that it interferes too much
in normal workflows of using 'C-x p f'.  I suggest to keep
the history of 'C-x p f' with relative filenames intact,
and add absolute filenames to 'file-name-history' that already
contains absolute filenames.

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

[Augusto Stoffel]

>>> Unfortunately, this makes the emacs-29 release unusable.
>>
>> Would you say the new behavior is worse than what we had before?
>
> Unfortunately, yes.  It turned out that it interferes too much
> in normal workflows of using 'C-x p f'.  I suggest to keep
> the history of 'C-x p f' with relative filenames intact,
> and add absolute filenames to 'file-name-history' that already
> contains absolute filenames.

Until now, `C-x p f' and `C-x C-f' have always shared the same history
variable, `file-name-history'.  Reverting this change to go back to the
even more broken (IMO) behavior of adding relative file names to the
history of `C-x C-f' et al.

How about computing the history of `C-x p f' dynamically by filtering
the elements of `file-name-history' which belong to the current project?
This is simpler than the other viable solution, to introduce
project-dependent histories.

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

[Juri Linkov]

>>> Would you say the new behavior is worse than what we had before?
>>
>> Unfortunately, yes.  It turned out that it interferes too much
>> in normal workflows of using 'C-x p f'.  I suggest to keep
>> the history of 'C-x p f' with relative filenames intact,
>> and add absolute filenames to 'file-name-history' that already
>> contains absolute filenames.
>
> Until now, `C-x p f' and `C-x C-f' have always shared the same history
> variable, `file-name-history'.  Reverting this change to go back to the
> even more broken (IMO) behavior of adding relative file names to the
> history of `C-x C-f' et al.

I agree that the old behavior was broken, and the new fixes it,
but only for `C-x C-f', at the expense of `C-x p f'.

> How about computing the history of `C-x p f' dynamically by filtering
> the elements of `file-name-history' which belong to the current project?
> This is simpler than the other viable solution, to introduce
> project-dependent histories.

This makes sense.  OTOH, sometimes such a need arises to revisit
the same project-relative file in another directory tree
with a similar file structure.  For example, in the directory
with the Emacs master branch visit 'C-x p f lisp/progmodes/project.el'.
Then revisit the same file in the directory with the emacs-29 branch
with 'C-x p f M-p'.

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

[Augusto Stoffel]

On Tue, 13 Dec 2022 at 19:28, Juri Linkov wrote:

> This makes sense.  OTOH, sometimes such a need arises to revisit
> the same project-relative file in another directory tree
> with a similar file structure.  For example, in the directory
> with the Emacs master branch visit 'C-x p f lisp/progmodes/project.el'.
> Then revisit the same file in the directory with the emacs-29 branch
> with 'C-x p f M-p'.

This is an interesting trick.  I have a somewhat related situation: for
some projects, I edit code locally but I can only run it in a remote
(and bigger) machine.  So I have some commands to go back and forth
between the two copies, sync them up, etc.

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.

Support for "sibling projects" like those would be a interesting but
rather subtle feature that needs to be well thought out.

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

[Dmitry Gutov]

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.

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.

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

[Augusto Stoffel]

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?

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.

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

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

[Dmitry Gutov]

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

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

[Juri Linkov]

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

Does the second patch allow such workflows as re-visiting
the same relative filenames in another directory?

IMHO, the safest fix for emacs-29 would be to add relative filenames
to the separate history.  If someone might want to use
'file-name-history', then a new variable could be added like
'query-replace-from-history-variable'.  Then this variable
could be customized to 'file-name-history', or nil.

It's needed only when 'project-read-file-name-function'
is 'project--read-file-cpd-relative'.

But please also note that its value 'project--read-file-absolute'
is also broken.  It asks confirmation for re-visiting even absolute
filenames, because currently 'C-x p f' adds abbreviated filenames,
but confirmation checks for absolute.

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

[Augusto Stoffel]

On Thu, 15 Dec 2022 at 01:04, Dmitry Gutov wrote:

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

My suggestion was not to filer based on project-files, but rather simply
by file name prefixes.  Which, if I read correctly, is exactly what your
patch v2 does.

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

In fact I think v2 already is a completely satisfactory implementation
the project-local file history feature.

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

[Augusto Stoffel]

On Thu, 15 Dec 2022 at 09:24, Juri Linkov wrote:

>> 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.
>
> Does the second patch allow such workflows as re-visiting
> the same relative filenames in another directory?

I don't think this should be the default behavior at all.  To me it
seems much more likely that a user _does not_ want to see files from
other projects when doing `C-x p f M-p'.

Your workflow seems more of a job the "find related file" feature that
has been discussed recently in the list.

> IMHO, the safest fix for emacs-29 would be to add relative filenames
> to the separate history.  If someone might want to use
> 'file-name-history', then a new variable could be added like
> 'query-replace-from-history-variable'.  Then this variable
> could be customized to 'file-name-history', or nil.

Then let's do the opposite.  We define 'project-file-history-variable'
and let it be nil by default.  In this case, we get the behavior of
Dmitry's patch v2.  If it's not nil, then we assume it's to be used
as-is, with no filtering or removal of project root prefix.  The user
can set it whatever way they please and take care it makes sense.

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

[Dmitry Gutov]

On 15/12/2022 09:24, Juri Linkov wrote:
>> 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.
> 
> Does the second patch allow such workflows as re-visiting
> the same relative filenames in another directory?

Not really; it filters by project root (more or less).

As long as we have global history, it's hard to extract the relative 
names for it, if they belong to a different directory tree.

But the history storage seems to retain text properties. So I suppose 
one option would be to annotate the "relative" part with a text 
property, before adding the string to history. And then look up the text 
property later.

> IMHO, the safest fix for emacs-29 would be to add relative filenames
> to the separate history.  If someone might want to use
> 'file-name-history', then a new variable could be added like
> 'query-replace-from-history-variable'.  Then this variable
> could be customized to 'file-name-history', or nil.
> 
> It's needed only when 'project-read-file-name-function'
> is 'project--read-file-cpd-relative'.

It can work, but having a user option that only works for 
project--read-file-cpd-relative is pretty awkward. Beginning with having 
to choose its name.

If we do this, I'd rather that happens on master, and we choose 
something smaller for emacs-29.

And the text property trick above could both "store" the relative 
history and also keep adding entries to 'C-x C-f'.

> But please also note that its value 'project--read-file-absolute'
> is also broken.  It asks confirmation for re-visiting even absolute
> filenames, because currently 'C-x p f' adds abbreviated filenames,
> but confirmation checks for absolute.

Does the breakage show itself only when you add a history entry using 
one value of project-read-file-name-function and then try to look it up 
when using another value of project-read-file-name-function?

If so, that's probably bearable enough for now (people don't really 
change this value often). Fixing it will require resolving the 
abbreviation situation. And we might as well switch to relative names in 
project-files first.

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

[Dmitry Gutov]

On 15/12/2022 13:07, Augusto Stoffel wrote:
> On Thu, 15 Dec 2022 at 09:24, Juri Linkov wrote:
> 
>>> 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.
>>
>> Does the second patch allow such workflows as re-visiting
>> the same relative filenames in another directory?
> 
> I don't think this should be the default behavior at all.  To me it
> seems much more likely that a user _does not_ want to see files from
> other projects when doing `C-x p f M-p'.

Do you think they will mind too much? At least if the relative names are 
shown, the user will recognize them and won't be too surprised.

> Your workflow seems more of a job the "find related file" feature that
> has been discussed recently in the list.
> 
>> IMHO, the safest fix for emacs-29 would be to add relative filenames
>> to the separate history.  If someone might want to use
>> 'file-name-history', then a new variable could be added like
>> 'query-replace-from-history-variable'.  Then this variable
>> could be customized to 'file-name-history', or nil.
> 
> Then let's do the opposite.  We define 'project-file-history-variable'
> and let it be nil by default.  In this case, we get the behavior of
> Dmitry's patch v2.  If it's not nil, then we assume it's to be used
> as-is, with no filtering or removal of project root prefix.  The user
> can set it whatever way they please and take care it makes sense.

The problem with that is storing relative history in such a variable 
will only work with one of the values of 
project-read-file-name-function. So we'll have two defcustoms which need 
to be changed in concert..?

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

[Dmitry Gutov]

On 15/12/2022 13:07, Augusto Stoffel wrote:
> On Thu, 15 Dec 2022 at 01:04, Dmitry Gutov wrote:
> 
>> 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.
> My suggestion was not to filer based on project-files, but rather simply
> by file name prefixes.  Which, if I read correctly, is exactly what your
> patch v2 does.

Ah yes.

>> 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.
> In fact I think v2 already is a completely satisfactory implementation
> the project-local file history feature.

It seems like there are two incompatible behaviors here and you and Juri 
want. One is to filter by current project, and another to be able to 
reuse previous inputs freely.

If forced to choose, I would be slightly inclined toward the latter 
(which could be implemented using text properties, although the concept 
is a little awkward). But both are reasonable.

I think we should install one of them and then later (on master) add a 
user option to switch to the other behavior.

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

[Juri Linkov]

>> In fact I think v2 already is a completely satisfactory implementation
>> the project-local file history feature.
>
> It seems like there are two incompatible behaviors here and you and Juri
> want. One is to filter by current project, and another to be able to reuse
> previous inputs freely.
>
> If forced to choose, I would be slightly inclined toward the latter (which
> could be implemented using text properties, although the concept is
> a little awkward). But both are reasonable.
>
> I think we should install one of them and then later (on master) add a user
> option to switch to the other behavior.

I agree with Augusto that filtering that implements project-local history
should be the default behavior for emacs-29.  I tried your second patch
and it nicely handles even such sequences as `C-x p f' and `C-x p F'.
(and in the opposite direction from `C-x p F' to `C-x p f' it
filters out external files, that is expected and consistent.)

OTOH, `C-x p f M-p' in another project is not my primary workflow.
But if someone wants to keep a plain history, this could be added
later in master, e.g. by a new value of project-read-file-name-function
and a function that is mostly a copy of project--read-file-cpd-relative.

BTW, in a fresh emacs -Q `C-x p f' raises:
  Debugger entered--Lisp error: (void-function cl-find-if)
But not sure how important is this case since usually `C-x p f'
is invoked on a project's file when project.el with cl is loaded.

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

[Juri Linkov]

>> But please also note that its value 'project--read-file-absolute'
>> is also broken.  It asks confirmation for re-visiting even absolute
>> filenames, because currently 'C-x p f' adds abbreviated filenames,
>> but confirmation checks for absolute.
>
> Does the breakage show itself only when you add a history entry using one
> value of project-read-file-name-function and then try to look it up when
> using another value of project-read-file-name-function?
>
> If so, that's probably bearable enough for now (people don't really change
> this value often). Fixing it will require resolving the abbreviation
> situation. And we might as well switch to relative names in project-files
> first.

Visiting with 'C-x C-f' adds an abbreviated file name to the history.
So it looks strange that 'C-x p f M-p' doesn't understand that it's
absolute even though it's abbreviated.

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

[Dmitry Gutov]

On 15/12/2022 19:24, Juri Linkov wrote:
>>> But please also note that its value 'project--read-file-absolute'
>>> is also broken.  It asks confirmation for re-visiting even absolute
>>> filenames, because currently 'C-x p f' adds abbreviated filenames,
>>> but confirmation checks for absolute.
>> Does the breakage show itself only when you add a history entry using one
>> value of project-read-file-name-function and then try to look it up when
>> using another value of project-read-file-name-function?
>>
>> If so, that's probably bearable enough for now (people don't really change
>> this value often). Fixing it will require resolving the abbreviation
>> situation. And we might as well switch to relative names in project-files
>> first.
> Visiting with 'C-x C-f' adds an abbreviated file name to the history.
> So it looks strange that 'C-x p f M-p' doesn't understand that it's
> absolute even though it's abbreviated.

Fair enough.

Can we do something with it, though, without piping all-files through 
#'expand-file-name?

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

[Dmitry Gutov]

On 15/12/2022 19:21, Juri Linkov wrote:
>>> In fact I think v2 already is a completely satisfactory implementation
>>> the project-local file history feature.
>>
>> It seems like there are two incompatible behaviors here and you and Juri
>> want. One is to filter by current project, and another to be able to reuse
>> previous inputs freely.
>>
>> If forced to choose, I would be slightly inclined toward the latter (which
>> could be implemented using text properties, although the concept is
>> a little awkward). But both are reasonable.
>>
>> I think we should install one of them and then later (on master) add a user
>> option to switch to the other behavior.
> 
> I agree with Augusto that filtering that implements project-local history
> should be the default behavior for emacs-29.  I tried your second patch
> and it nicely handles even such sequences as `C-x p f' and `C-x p F'.
> (and in the opposite direction from `C-x p F' to `C-x p f' it
> filters out external files, that is expected and consistent.)

That sounds like a consensus, then. I've pushed the v2 patch to emacs-29.

> OTOH, `C-x p f M-p' in another project is not my primary workflow.
> But if someone wants to keep a plain history, this could be added
> later in master, e.g. by a new value of project-read-file-name-function
> and a function that is mostly a copy of project--read-file-cpd-relative.
> 
> BTW, in a fresh emacs -Q `C-x p f' raises:
>    Debugger entered--Lisp error: (void-function cl-find-if)
> But not sure how important is this case since usually `C-x p f'
> is invoked on a project's file when project.el with cl is loaded.

...and included a fix for this too. Thanks!

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

[Juri Linkov]

>>>> But please also note that its value 'project--read-file-absolute'
>>>> is also broken.  It asks confirmation for re-visiting even absolute
>>>> filenames, because currently 'C-x p f' adds abbreviated filenames,
>>>> but confirmation checks for absolute.
>>> Does the breakage show itself only when you add a history entry using one
>>> value of project-read-file-name-function and then try to look it up when
>>> using another value of project-read-file-name-function?
>>>
>>> If so, that's probably bearable enough for now (people don't really change
>>> this value often). Fixing it will require resolving the abbreviation
>>> situation. And we might as well switch to relative names in project-files
>>> first.
>> Visiting with 'C-x C-f' adds an abbreviated file name to the history.
>> So it looks strange that 'C-x p f M-p' doesn't understand that it's
>> absolute even though it's abbreviated.
>
> Fair enough.
>
> Can we do something with it, though, without piping all-files through
> #'expand-file-name?

In theory, it should be sufficient to pipe through #'expand-file-name
only the completion candidate to try both with and without
#'expand-file-name.  But in practice this might require
implementing this as the CONFIRM function.

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

[Dmitry Gutov]

On 16/12/2022 09:48, Juri Linkov wrote:
>> Can we do something with it, though, without piping all-files through
>> #'expand-file-name?
> In theory, it should be sufficient to pipe through #'expand-file-name
> only the completion candidate to try both with and without
> #'expand-file-name.  But in practice this might require
> implementing this as the CONFIRM function.

Either that, or indeed modify the completion table inside 
project--file-completion-table.

Not sure which is easier to do and/or more compatible with the different 
completion frameworks out there.

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

[Juri Linkov]

>> I agree with Augusto that filtering that implements project-local history
>> should be the default behavior for emacs-29.  I tried your second patch
>> and it nicely handles even such sequences as `C-x p f' and `C-x p F'.
>> (and in the opposite direction from `C-x p F' to `C-x p f' it
>> filters out external files, that is expected and consistent.)
>
> That sounds like a consensus, then. I've pushed the v2 patch to emacs-29.

Thanks.  One very minor detail: when the history contains the project root
directory then it's confusing to see that M-p does nothing.  It looks like
it has no effect because it inserts an empty string.  Maybe better to
filter out empty strings?

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

[Dmitry Gutov]

On 18/12/2022 10:36, Juri Linkov wrote:
> It looks like
> it has no effect because it inserts an empty string.  Maybe better to
> filter out empty strings?

Sure. Do you want to implement that?

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

[Juri Linkov]

>> It looks like
>> it has no effect because it inserts an empty string.  Maybe better to
>> filter out empty strings?
>
> Sure. Do you want to implement that?

Done in commit 399433cc2b9.

Also noticed that project-find-dir (that uses the same
project--read-file-cpd-relative) needs to filter out
all file names, leaving only directory names in the
pre-processed history.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Augusto Stoffel <arstoffel@gmail.com>,
>   58447@debbugs.gnu.org
> Date: Mon, 19 Dec 2022 19:55:22 +0200
> 
> >> It looks like
> >> it has no effect because it inserts an empty string.  Maybe better to
> >> filter out empty strings?
> >
> > Sure. Do you want to implement that?
> 
> Done in commit 399433cc2b9.

How about making the code there a bit more efficient by computing
(length abbr-cpd) just once instead of twice?

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

[Dmitry Gutov]

On 19/12/2022 19:55, Juri Linkov wrote:
> Also noticed that project-find-dir (that uses the same
> project--read-file-cpd-relative) needs to filter out
> all file names, leaving only directory names in the
> pre-processed history.

That will probably be an improvement, yes.

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

[Juri Linkov]

>> >> It looks like
>> >> it has no effect because it inserts an empty string.  Maybe better to
>> >> filter out empty strings?
>> >
>> > Sure. Do you want to implement that?
>> 
>> Done in commit 399433cc2b9.
>
> How about making the code there a bit more efficient by computing
> (length abbr-cpd) just once instead of twice?

Fixed.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: dgutov@yandex.ru,  arstoffel@gmail.com,  58447@debbugs.gnu.org
> Date: Mon, 19 Dec 2022 21:48:20 +0200
> 
> > How about making the code there a bit more efficient by computing
> > (length abbr-cpd) just once instead of twice?
> 
> Fixed.

Thanks.