bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

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

Tags: patch


This fixes a periodic issue that happens with multiple Emacs instances
and project.el.

Maybe this isn't the right way to change the behavior of
ask-user-about-lock though; possibly we should add some new defcustom to
customize it.  Happy to do that if that's preferred.


In GNU Emacs 29.1.50 (build 15, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2023-10-25 built on
 igm-qws-u22796a
Repository revision: 5cbca757f620c7b4ca31776711a247b8f266c36e
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.8 (Green Obsidian)

Configured using:
 'configure --config-cache --with-x-toolkit=lucid
 --with-gif=ifavailable'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-project.el-avoid-asking-user-about-project-list-file.patch --]
[-- Type: text/patch, Size: 2573 bytes --]

From ec6caaf9fcb913847278f7183e46d3026c6986fb Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 7 Nov 2023 16:24:26 -0500
Subject: [PATCH] project.el: avoid asking user about project-list-file lock

There are several features which will cause Emacs to frequently call
project-current, and therefore call project-remember-project, and
therefore sometimes call project--write-project-list whenever a new
project is seen.

- project-uniquify-dirname-transform will call this for each new
  buffer

- project-mode-line will call this on mode-line update

- My own private project-watch will call this based on file-notify
  events.

If a user has multiple Emacs instances open using one or more of these
features, it's fairly easy for both of the Emacs instances to see a
new project at the same time.  In that case, they'll both call
project--write-project-list at the same time, which will clash and run
ask-user-about-lock.  This will happen frequently if the user is often
looking at new projects.

There's no correct answer the user can give to ask-user-about-lock:
either way, one of the Emacs instances will have its project-list-file
update lost.  So let's not even bother prompting the user: just do
nothing if project-list-file is currently locked.

In the long run, the update doesn't actually get lost, because the
Emacs instance will probably make the same project-list-file update
again a few seconds later due to a later call to project-current.  So
this doesn't lose anything.

* lisp/progmodes/project.el (project--write-project-list): No-op if
the file is locked.
---
 lisp/progmodes/project.el | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 43c78f8b16b..78aaa75de5f 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1719,7 +1719,13 @@ project--write-project-list
                                 (expand-file-name name)))))
                     project--list)
             (current-buffer)))
-      (write-region nil nil filename nil 'silent))))
+      ;; If project-list-file is locked by some other Emacs, fail to
+      ;; write rather than prompting the user.
+      (ignore-error file-locked
+        (cl-letf (((symbol-function 'ask-user-about-lock)
+                   (lambda (file opponent)
+                     (signal 'file-locked (list file opponent)))))
+          (write-region nil nil filename nil 'silent))))))
 
 ;;;###autoload
 (defun project-remember-project (pr &optional no-write)
-- 
2.39.3

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 07/11/2023 23:28, Spencer Baugh wrote:
> - project-mode-line will call this on mode-line update

Hopefully this will never result in writes to disk made more often than 
once per user command, or buffer switch, etc.

> -      (write-region nil nil filename nil 'silent))))
> +      ;; If project-list-file is locked by some other Emacs, fail to
> +      ;; write rather than prompting the user.
> +      (ignore-error file-locked
> +        (cl-letf (((symbol-function 'ask-user-about-lock)
> +                   (lambda (file opponent)
> +                     (signal 'file-locked (list file opponent)))))
> +          (write-region nil nil filename nil 'silent))))))

I wonder if all cl-letf uses like this will survive native compilation, 
for example. Or will break over time due to internal changes in the 
function.

Anyway, maybe an implementation like this (totally untested)?

Or the warning could be skipped entirely.

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index a6426c08840..e544dfefa73 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1719,7 +1719,9 @@ project--write-project-list
                                  (expand-file-name name)))))
                      project--list)
              (current-buffer)))
-      (write-region nil nil filename nil 'silent))))
+      (let ((noninteractive t))
+        (with-demoted-errors "Failed to save file list: %S"
+          (write-region nil nil filename nil 'silent))))))

  ;;;###autoload
  (defun project-remember-project (pr &optional no-write)

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> Date: Wed, 8 Nov 2023 02:24:13 +0200
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> Anyway, maybe an implementation like this (totally untested)?
> 
> Or the warning could be skipped entirely.
> 
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index a6426c08840..e544dfefa73 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -1719,7 +1719,9 @@ project--write-project-list
>                                   (expand-file-name name)))))
>                       project--list)
>               (current-buffer)))
> -      (write-region nil nil filename nil 'silent))))
> +      (let ((noninteractive t))
> +        (with-demoted-errors "Failed to save file list: %S"
> +          (write-region nil nil filename nil 'silent))))))

Isn't the cure worse than the disease? we are in effect disregarding
all errors that prevent us from saving the file.  If those errors are
not important, why save the list to the file at all?

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 08/11/2023 14:29, Eli Zaretskii wrote:
>> Date: Wed, 8 Nov 2023 02:24:13 +0200
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> Anyway, maybe an implementation like this (totally untested)?
>>
>> Or the warning could be skipped entirely.
>>
>> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>> index a6426c08840..e544dfefa73 100644
>> --- a/lisp/progmodes/project.el
>> +++ b/lisp/progmodes/project.el
>> @@ -1719,7 +1719,9 @@ project--write-project-list
>>                                    (expand-file-name name)))))
>>                        project--list)
>>                (current-buffer)))
>> -      (write-region nil nil filename nil 'silent))))
>> +      (let ((noninteractive t))
>> +        (with-demoted-errors "Failed to save file list: %S"
>> +          (write-region nil nil filename nil 'silent))))))
> Isn't the cure worse than the disease? we are in effect disregarding
> all errors that prevent us from saving the file.

If ask-user-about-lock signaled a specific error rather than just (error 
...), we could catch only that one.

 > If those errors are
 > not important, why save the list to the file at all?

It's not hugely important that the list is up-to-date at all times. I 
haven't encountered the conflicts myself, though. Maybe some other 
solution could be preferable.

Michael's suggestion sounds pretty efficient, for example.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Dmitry Gutov <dmitry@gutov.dev> writes:
> On 07/11/2023 23:28, Spencer Baugh wrote:
>> - project-mode-line will call this on mode-line update
>
> Hopefully this will never result in writes to disk made more often
> than once per user command, or buffer switch, etc.
>
>> -      (write-region nil nil filename nil 'silent))))
>> +      ;; If project-list-file is locked by some other Emacs, fail to
>> +      ;; write rather than prompting the user.
>> +      (ignore-error file-locked
>> +        (cl-letf (((symbol-function 'ask-user-about-lock)
>> +                   (lambda (file opponent)
>> +                     (signal 'file-locked (list file opponent)))))
>> +          (write-region nil nil filename nil 'silent))))))
>
> I wonder if all cl-letf uses like this will survive native
> compilation, for example. Or will break over time due to internal
> changes in the function.
>
> Anyway, maybe an implementation like this (totally untested)?
>
> Or the warning could be skipped entirely.
>
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index a6426c08840..e544dfefa73 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -1719,7 +1719,9 @@ project--write-project-list
>                                  (expand-file-name name)))))
>                      project--list)
>              (current-buffer)))
> -      (write-region nil nil filename nil 'silent))))
> +      (let ((noninteractive t))
> +        (with-demoted-errors "Failed to save file list: %S"
> +          (write-region nil nil filename nil 'silent))))))
>
>  ;;;###autoload
>  (defun project-remember-project (pr &optional no-write)

Good idea using noninteractive.  I agree that should signal file-locked,
so we can handle it.  That seems like the most elegant solution.

However, interestingly, this actually seems to crash Emacs!  Not sure
why yet.

Reproduction:

1. Open ~/file and edit it without saving (so Emacs takes the lock)
2. in a separate emacs -Q, run with M-:
(let ((noninteractive t)) (write-region nil nil "~/file"))
3. Notice the separate emacs -Q immediately crashes!

This is really a separate bug, but since we're talking about
noninteractively handling lock conflicts, we might as well solve it here
- or decide if it really should be solved.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Michael Albinus]

Spencer Baugh <sbaugh@janestreet.com> writes:

Hi Spencer,

> If a user has multiple Emacs instances open using one or more of these
> features, it's fairly easy for both of the Emacs instances to see a
> new project at the same time.  In that case, they'll both call
> project--write-project-list at the same time, which will clash and run
> ask-user-about-lock.  This will happen frequently if the user is often
> looking at new projects.
>
> There's no correct answer the user can give to ask-user-about-lock:
> either way, one of the Emacs instances will have its project-list-file
> update lost.  So let's not even bother prompting the user: just do
> nothing if project-list-file is currently locked.

If you don't care about locking project-list-file, let-bind
create-lockfiles to nil while writing project-list-file.

Best regards, Michael.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Michael Albinus <michael.albinus@gmx.de> writes:
> Spencer Baugh <sbaugh@janestreet.com> writes:
>
> Hi Spencer,
>
>> If a user has multiple Emacs instances open using one or more of these
>> features, it's fairly easy for both of the Emacs instances to see a
>> new project at the same time.  In that case, they'll both call
>> project--write-project-list at the same time, which will clash and run
>> ask-user-about-lock.  This will happen frequently if the user is often
>> looking at new projects.
>>
>> There's no correct answer the user can give to ask-user-about-lock:
>> either way, one of the Emacs instances will have its project-list-file
>> update lost.  So let's not even bother prompting the user: just do
>> nothing if project-list-file is currently locked.
>
> If you don't care about locking project-list-file, let-bind
> create-lockfiles to nil while writing project-list-file.

Hm, I think that could result in two writes clobbering each other in a
way that creates an invalidly formatted file.  This would happen if
e.g. there were two concurrent write-regions that got interleaved like:

1. write-region(A) starts and writes first half
2. write-region(B) starts and completes
3. write-region(A) writes second half

Then it would be some combination of A and B, which might not be validly
formatted.  From a brief look at the write-region implementation, this
is possible.  (The thing that would make this impossible is if
write-region wrote into a tempfile and then renamed it on top of the
destination.  Instead write-region just writes directly into the file,
opening it with O_TRUNC.)

It's not very likely, but if the lock is conflicting at all and the
project list is large then this will probably happen eventually.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Michael Albinus]

Spencer Baugh <sbaugh@janestreet.com> writes:

Hi Spencer,

>> If you don't care about locking project-list-file, let-bind
>> create-lockfiles to nil while writing project-list-file.
>
> Hm, I think that could result in two writes clobbering each other in a
> way that creates an invalidly formatted file.

Alternatively, you could check the status via file-locked-p prior
calling write-region.

I don't use project.el, so I don't know what's the best. Overwriting
ask-user-about-lock seems to be a bad choice, IMHO.

Best regards, Michael.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> Cc: 66993@debbugs.gnu.org
> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Wed, 08 Nov 2023 16:44:43 +0100
> 
> Overwriting ask-user-about-lock seems to be a bad choice, IMHO.

Agreed.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Tue, 07 Nov 2023 16:28:04 -0500
> 
> There are several features which will cause Emacs to frequently call
> project-current, and therefore call project-remember-project, and
> therefore sometimes call project--write-project-list whenever a new
> project is seen.

Why does project-current immediately writes the list to the file?  Why
cannot it keep the information in memory and write it only when the
session ends, or at some random rare opportunity?

> - project-uniquify-dirname-transform will call this for each new
>   buffer
> 
> - project-mode-line will call this on mode-line update
> 
> - My own private project-watch will call this based on file-notify
>   events.

Ouch! this doesn't sound like a clean implementation to me.  Writing
something to a file from mode-line update? or when a new buffer is
created?  Why cannot we come up with a cleaner implementation?

> There's no correct answer the user can give to ask-user-about-lock:
> either way, one of the Emacs instances will have its project-list-file
> update lost.  So let's not even bother prompting the user: just do
> nothing if project-list-file is currently locked.

Sorry, that cannot be TRT.  We cannot second-guess what the user wants
to do in this case, and claim we are always right.

> In the long run, the update doesn't actually get lost, because the
> Emacs instance will probably make the same project-list-file update
> again a few seconds later due to a later call to project-current.  So
> this doesn't lose anything.

One more reason not to write the changes upon each change.  Or at
least detect that the file is locked, and schedule update at a later
time, or something.  Ideally, we should only write the file at exit.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 08/11/2023 14:22, Eli Zaretskii wrote:
>> From: Spencer Baugh<sbaugh@janestreet.com>
>> Date: Tue, 07 Nov 2023 16:28:04 -0500
>>
>> There are several features which will cause Emacs to frequently call
>> project-current, and therefore call project-remember-project, and
>> therefore sometimes call project--write-project-list whenever a new
>> project is seen.
> Why does project-current immediately writes the list to the file?  Why
> cannot it keep the information in memory and write it only when the
> session ends, or at some random rare opportunity?

It could indeed be written from kill-emacs-hook, or just the next time 
an opportunity presents.

With the latter approach, though, it would be handier if the 
lock-prompter signaled a specific error we could catch to reschedule saving.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> Date: Wed, 8 Nov 2023 15:26:11 +0200
> Cc: 66993@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 08/11/2023 14:22, Eli Zaretskii wrote:
> >> From: Spencer Baugh<sbaugh@janestreet.com>
> >> Date: Tue, 07 Nov 2023 16:28:04 -0500
> >>
> >> There are several features which will cause Emacs to frequently call
> >> project-current, and therefore call project-remember-project, and
> >> therefore sometimes call project--write-project-list whenever a new
> >> project is seen.
> > Why does project-current immediately writes the list to the file?  Why
> > cannot it keep the information in memory and write it only when the
> > session ends, or at some random rare opportunity?
> 
> It could indeed be written from kill-emacs-hook, or just the next time 
> an opportunity presents.
> 
> With the latter approach, though, it would be handier if the 
> lock-prompter signaled a specific error we could catch to reschedule saving.

Why do you need an error when you can use file-locked-p to check up
front that the file is locked?

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 08/11/2023 15:50, Eli Zaretskii wrote:
> Why do you need an error when you can use file-locked-p to check up
> front that the file is locked?

IIUC the problem comes due to concurrent writes from concurrent writes 
from parallel Emacs instances.

Simply checking whether the file is locked before writing, without 
trying to obtain the lock, is unlikely to be a reliable solution 
(another instance might lock it right after we checked).

Anyway, these writes must be very fast and relatively infrequent. So I'm 
surprised that this has came up, personally.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> Date: Wed, 8 Nov 2023 15:56:11 +0200
> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 08/11/2023 15:50, Eli Zaretskii wrote:
> > Why do you need an error when you can use file-locked-p to check up
> > front that the file is locked?
> 
> IIUC the problem comes due to concurrent writes from concurrent writes 
> from parallel Emacs instances.
> 
> Simply checking whether the file is locked before writing, without 
> trying to obtain the lock, is unlikely to be a reliable solution 
> (another instance might lock it right after we checked).
> 
> Anyway, these writes must be very fast and relatively infrequent. So I'm 
> surprised that this has came up, personally.

If that's the case, just catch the error and retry, several times,
perhaps with variable delays.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Wed, 8 Nov 2023 15:56:11 +0200
>> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
>> From: Dmitry Gutov <dmitry@gutov.dev>
>> 
>> On 08/11/2023 15:50, Eli Zaretskii wrote:
>> > Why do you need an error when you can use file-locked-p to check up
>> > front that the file is locked?
>> 
>> IIUC the problem comes due to concurrent writes from concurrent writes 
>> from parallel Emacs instances.
>> 
>> Simply checking whether the file is locked before writing, without 
>> trying to obtain the lock, is unlikely to be a reliable solution 
>> (another instance might lock it right after we checked).
>> 
>> Anyway, these writes must be very fast and relatively infrequent. So I'm 
>> surprised that this has came up, personally.
>
> If that's the case, just catch the error and retry, several times,
> perhaps with variable delays.

Perhaps, but it would be nice to do that by catching specifically
file-locked rather than catching all errors.  Which is something which
can't be done right now, other than with the cl-flet approach in my
original approach.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: Dmitry Gutov <dmitry@gutov.dev>,  66993@debbugs.gnu.org
> Date: Wed, 08 Nov 2023 10:41:15 -0500
> 
> > If that's the case, just catch the error and retry, several times,
> > perhaps with variable delays.
> 
> Perhaps, but it would be nice to do that by catching specifically
> file-locked rather than catching all errors.

Not sure I follow: userlock.el signals a very specific error, doesn't
it?  Or what am I missing?

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: Dmitry Gutov <dmitry@gutov.dev>,  66993@debbugs.gnu.org
>> Date: Wed, 08 Nov 2023 10:41:15 -0500
>> 
>> > If that's the case, just catch the error and retry, several times,
>> > perhaps with variable delays.
>> 
>> Perhaps, but it would be nice to do that by catching specifically
>> file-locked rather than catching all errors.
>
> Not sure I follow: userlock.el signals a very specific error, doesn't
> it?  Or what am I missing?

By that you mean file-locked?  It only signals file-locked if it first
prompts the user, which is what we want to avoid.  If noninteractive=t,
then it just signals error:

(if noninteractive (error "Cannot resolve lock conflict in batch mode"))

Possibly we should just change that to signal file-locked too.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: dmitry@gutov.dev,  66993@debbugs.gnu.org
> Date: Wed, 08 Nov 2023 12:05:38 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >
> > Not sure I follow: userlock.el signals a very specific error, doesn't
> > it?  Or what am I missing?
> 
> By that you mean file-locked?  It only signals file-locked if it first
> prompts the user, which is what we want to avoid.  If noninteractive=t,
> then it just signals error:
> 
> (if noninteractive (error "Cannot resolve lock conflict in batch mode"))

And that is not specific enough?

And why the noninteractive=t case is relevant here, btw?

> Possibly we should just change that to signal file-locked too.

Or something else.  If needed.

In any case, these are minor details, the main point is that this
error can be caught.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: dmitry@gutov.dev,  66993@debbugs.gnu.org
>> Date: Wed, 08 Nov 2023 12:05:38 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> >
>> > Not sure I follow: userlock.el signals a very specific error, doesn't
>> > it?  Or what am I missing?
>> 
>> By that you mean file-locked?  It only signals file-locked if it first
>> prompts the user, which is what we want to avoid.  If noninteractive=t,
>> then it just signals error:
>> 
>> (if noninteractive (error "Cannot resolve lock conflict in batch mode"))
>
> And that is not specific enough?

Are you suggesting that we should condition-case and check the string
inside the error is "Cannot resolve lock conflict in batch mode"?
Otherwise, if we just catch all errors, this will also catch and
suppress errors for failures to write to the file, which would be bad.

> And why the noninteractive=t case is relevant here, btw?

Because we don't want to prompt the user, we just want to signal an
error if there's a lock conflict.

>> Possibly we should just change that to signal file-locked too.
>
> Or something else.  If needed.
>
> In any case, these are minor details, the main point is that this
> error can be caught.

Well... the other issue is that Emacs crashes if you bind
noninteractive=t and then hit a lock conflict.  e.g.:

1. Open ~/file and edit it without saving (so Emacs takes the lock)
2. in a separate emacs -Q, run with M-:
(let ((noninteractive t)) (write-region nil nil "~/file"))
3. The emacs -Q crashes

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: dmitry@gutov.dev,  66993@debbugs.gnu.org
> Date: Wed, 08 Nov 2023 15:43:53 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> (if noninteractive (error "Cannot resolve lock conflict in batch mode"))
> >
> > And that is not specific enough?
> 
> Are you suggesting that we should condition-case and check the string
> inside the error is "Cannot resolve lock conflict in batch mode"?

That's one way, yes.  Another one is to use define-error to define a
new error type for this case.

> > And why the noninteractive=t case is relevant here, btw?
> 
> Because we don't want to prompt the user, we just want to signal an
> error if there's a lock conflict.

??? Is project-current always used in a non-interactive context?  I
don't think so.  When some interactive program calls it,
noninteractive will be nil, and what userlock.el does in that case is
not what you describe.  And if you are saying that some program binds
noninteractive to a non-nil value to avoid asking the file-locked
question, then with the error-catching as discussed above in place,
that program won't need to do that anymore, right?  (Also see below
for why this binding is problematic in a more general sense.)

> Well... the other issue is that Emacs crashes if you bind
> noninteractive=t and then hit a lock conflict.  e.g.:
> 
> 1. Open ~/file and edit it without saving (so Emacs takes the lock)
> 2. in a separate emacs -Q, run with M-:
> (let ((noninteractive t)) (write-region nil nil "~/file"))
> 3. The emacs -Q crashes

(Didn't you just say it's a separate problem?)

It doesn't crash here, it exits with exit code -1.  Which is a direct
consequence of binding noninteractive non-nil: signaling an error in
that case shows the backtrace on stderr and exits.  "Kids, don't try
that at home!"

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: dmitry@gutov.dev,  66993@debbugs.gnu.org
>> Date: Wed, 08 Nov 2023 15:43:53 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> (if noninteractive (error "Cannot resolve lock conflict in batch mode"))
>> >
>> > And that is not specific enough?
>> 
>> Are you suggesting that we should condition-case and check the string
>> inside the error is "Cannot resolve lock conflict in batch mode"?
>
> That's one way, yes.  Another one is to use define-error to define a
> new error type for this case.

Instead of defining a new error type, how about just signaling
file-locked instead?  e.g. the following patch:

diff --git a/lisp/userlock.el b/lisp/userlock.el
index 61f061d3e54..e4d23c56249 100644
--- a/lisp/userlock.el
+++ b/lisp/userlock.el
@@ -67,7 +67,7 @@ ask-user-about-lock
         (message (substitute-command-keys
                   "%s locked by %s: (\\`s', \\`q', \\`p', \\`?')? ")
                  short-file short-opponent)
-	(if noninteractive (error "Cannot resolve lock conflict in batch mode"))
+	(if noninteractive (signal 'file-locked (list file opponent)))
 	(let ((tem (let ((inhibit-quit t)
 			 (cursor-in-echo-area t))
 		     (prog1 (downcase (read-char))

Including also a documentation update to explain that when
noninteractive=t, a file lock conflict always signals file-locked
instead of prompting the user.

>> > And why the noninteractive=t case is relevant here, btw?
>> 
>> Because we don't want to prompt the user, we just want to signal an
>> error if there's a lock conflict.
>
> ??? Is project-current always used in a non-interactive context?  I
> don't think so.  When some interactive program calls it,
> noninteractive will be nil, and what userlock.el does in that case is
> not what you describe.

Right.

> And if you are saying that some program binds noninteractive to a
> non-nil value to avoid asking the file-locked question, then with the
> error-catching as discussed above in place, that program won't need to
> do that anymore, right?  (Also see below for why this binding is
> problematic in a more general sense.)

Yes, that's what I'm saying.  That's all correct.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: dmitry@gutov.dev,  66993@debbugs.gnu.org
> Date: Thu, 09 Nov 2023 11:38:26 -0500
> 
> > That's one way, yes.  Another one is to use define-error to define a
> > new error type for this case.
> 
> Instead of defining a new error type, how about just signaling
> file-locked instead?  e.g. the following patch:
> 
> diff --git a/lisp/userlock.el b/lisp/userlock.el
> index 61f061d3e54..e4d23c56249 100644
> --- a/lisp/userlock.el
> +++ b/lisp/userlock.el
> @@ -67,7 +67,7 @@ ask-user-about-lock
>          (message (substitute-command-keys
>                    "%s locked by %s: (\\`s', \\`q', \\`p', \\`?')? ")
>                   short-file short-opponent)
> -	(if noninteractive (error "Cannot resolve lock conflict in batch mode"))
> +	(if noninteractive (signal 'file-locked (list file opponent)))
>  	(let ((tem (let ((inhibit-quit t)
>  			 (cursor-in-echo-area t))
>  		     (prog1 (downcase (read-char))

Please show the result of this change on what Emacs prints in batch
mode when this error is signaled.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: dmitry@gutov.dev,  66993@debbugs.gnu.org
>> Date: Thu, 09 Nov 2023 11:38:26 -0500
>> 
>> > That's one way, yes.  Another one is to use define-error to define a
>> > new error type for this case.
>> 
>> Instead of defining a new error type, how about just signaling
>> file-locked instead?  e.g. the following patch:
>> 
>> diff --git a/lisp/userlock.el b/lisp/userlock.el
>> index 61f061d3e54..e4d23c56249 100644
>> --- a/lisp/userlock.el
>> +++ b/lisp/userlock.el
>> @@ -67,7 +67,7 @@ ask-user-about-lock
>>          (message (substitute-command-keys
>>                    "%s locked by %s: (\\`s', \\`q', \\`p', \\`?')? ")
>>                   short-file short-opponent)
>> -	(if noninteractive (error "Cannot resolve lock conflict in batch mode"))
>> +	(if noninteractive (signal 'file-locked (list file opponent)))
>>  	(let ((tem (let ((inhibit-quit t)
>>  			 (cursor-in-echo-area t))
>>  		     (prog1 (downcase (read-char))
>
> Please show the result of this change on what Emacs prints in batch
> mode when this error is signaled.

Before:

$ ./src/emacs -Q --batch --eval '(write-region "foo" nil "~/file")'
/home/sbaugh/file locked by sbaugh@igm-qw... (pid 3781848): (s, q, p, ?)? 

Error: error ("Cannot resolve lock conflict in batch mode")
  mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode 0x957865a77aef0ee>))
  debug-early-backtrace()
  debug-early(error (error "Cannot resolve lock conflict in batch mode"))
  signal(error ("Cannot resolve lock conflict in batch mode"))
  error("Cannot resolve lock conflict in batch mode")
  ask-user-about-lock("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)")
  write-region("foo" nil "~/file")
  eval((write-region "foo" nil "~/file") t)
  command-line-1(("--eval" "(write-region \"foo\" nil \"~/file\")"))
  command-line()
  normal-top-level()
Cannot resolve lock conflict in batch mode


After:

$ ./src/emacs -Q --batch --eval '(write-region "foo" nil "~/file")'
/home/sbaugh/file locked by sbaugh@igm-qw... (pid 3781848): (s, q, p, ?)? 

Error: file-locked ("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)")
  mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode 0x179d0e5a77aef0e7>))
  debug-early-backtrace()
  debug-early(error (file-locked "/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)"))
  signal(file-locked ("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)"))
  ask-user-about-lock("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)")
  write-region("foo" nil "~/file")
  eval((write-region "foo" nil "~/file") t)
  command-line-1(("--eval" "(write-region \"foo\" nil \"~/file\")"))
  command-line()
  normal-top-level()
/home/sbaugh/file: sbaugh@igm-qws-u22796a (pid 3781848)

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: dmitry@gutov.dev,  66993@debbugs.gnu.org
> Date: Thu, 09 Nov 2023 13:01:09 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > Please show the result of this change on what Emacs prints in batch
> > mode when this error is signaled.
> 
> Before:
> 
> $ ./src/emacs -Q --batch --eval '(write-region "foo" nil "~/file")'
> /home/sbaugh/file locked by sbaugh@igm-qw... (pid 3781848): (s, q, p, ?)? 
> 
> Error: error ("Cannot resolve lock conflict in batch mode")
>   mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode 0x957865a77aef0ee>))
>   debug-early-backtrace()
>   debug-early(error (error "Cannot resolve lock conflict in batch mode"))
>   signal(error ("Cannot resolve lock conflict in batch mode"))
>   error("Cannot resolve lock conflict in batch mode")
>   ask-user-about-lock("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)")
>   write-region("foo" nil "~/file")
>   eval((write-region "foo" nil "~/file") t)
>   command-line-1(("--eval" "(write-region \"foo\" nil \"~/file\")"))
>   command-line()
>   normal-top-level()
> Cannot resolve lock conflict in batch mode
> 
> 
> After:
> 
> $ ./src/emacs -Q --batch --eval '(write-region "foo" nil "~/file")'
> /home/sbaugh/file locked by sbaugh@igm-qw... (pid 3781848): (s, q, p, ?)? 
> 
> Error: file-locked ("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)")
>   mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode 0x179d0e5a77aef0e7>))
>   debug-early-backtrace()
>   debug-early(error (file-locked "/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)"))
>   signal(file-locked ("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)"))
>   ask-user-about-lock("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)")
>   write-region("foo" nil "~/file")
>   eval((write-region "foo" nil "~/file") t)
>   command-line-1(("--eval" "(write-region \"foo\" nil \"~/file\")"))
>   command-line()
>   normal-top-level()
> /home/sbaugh/file: sbaugh@igm-qws-u22796a (pid 3781848)

Thanks, that's what I thought: this loses information.  The "Cannot
resolve lock conflict in batch mode" part is important, since it
explains the "file-locked" part.  So please include the missing text
in the list passed to 'signal' as its DATA argument, so as not to lose
this explanation.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

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

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: dmitry@gutov.dev,  66993@debbugs.gnu.org
>> Date: Thu, 09 Nov 2023 13:01:09 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> > Please show the result of this change on what Emacs prints in batch
>> > mode when this error is signaled.
>> 
>> Before:
>> 
>> $ ./src/emacs -Q --batch --eval '(write-region "foo" nil "~/file")'
>> /home/sbaugh/file locked by sbaugh@igm-qw... (pid 3781848): (s, q, p, ?)? 
>> 
>> Error: error ("Cannot resolve lock conflict in batch mode")
>>   mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode 0x957865a77aef0ee>))
>>   debug-early-backtrace()
>>   debug-early(error (error "Cannot resolve lock conflict in batch mode"))
>>   signal(error ("Cannot resolve lock conflict in batch mode"))
>>   error("Cannot resolve lock conflict in batch mode")
>>   ask-user-about-lock("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)")
>>   write-region("foo" nil "~/file")
>>   eval((write-region "foo" nil "~/file") t)
>>   command-line-1(("--eval" "(write-region \"foo\" nil \"~/file\")"))
>>   command-line()
>>   normal-top-level()
>> Cannot resolve lock conflict in batch mode
>> 
>> 
>> After:
>> 
>> $ ./src/emacs -Q --batch --eval '(write-region "foo" nil "~/file")'
>> /home/sbaugh/file locked by sbaugh@igm-qw... (pid 3781848): (s, q, p, ?)? 
>> 
>> Error: file-locked ("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)")
>>   mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode 0x179d0e5a77aef0e7>))
>>   debug-early-backtrace()
>>   debug-early(error (file-locked "/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)"))
>>   signal(file-locked ("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)"))
>>   ask-user-about-lock("/home/sbaugh/file" "sbaugh@igm-qws-u22796a (pid 3781848)")
>>   write-region("foo" nil "~/file")
>>   eval((write-region "foo" nil "~/file") t)
>>   command-line-1(("--eval" "(write-region \"foo\" nil \"~/file\")"))
>>   command-line()
>>   normal-top-level()
>> /home/sbaugh/file: sbaugh@igm-qws-u22796a (pid 3781848)
>
> Thanks, that's what I thought: this loses information.  The "Cannot
> resolve lock conflict in batch mode" part is important, since it
> explains the "file-locked" part.  So please include the missing text
> in the list passed to 'signal' as its DATA argument, so as not to lose
> this explanation.

OK, how about this?  Which makes Emacs print:

Error: file-locked ("/home/sbaugh/file" "sbaugh@earth (pid 1852838)" "Cannot resolve lock conflict in batch mode")
  mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode -0x1f22147a251783b>))
  debug-early-backtrace()
  debug-early(error (file-locked "/home/sbaugh/file" "sbaugh@earth (pid 1852838)" "Cannot resolve lock conflict in batch mode"))
  signal(file-locked ("/home/sbaugh/file" "sbaugh@earth (pid 1852838)" "Cannot resolve lock conflict in batch mode"))
  ask-user-about-lock("/home/sbaugh/file" "sbaugh@earth (pid 1852838)")
  write-region("foo" nil "~/file")
  eval((write-region "foo" nil "~/file") t)
  command-line-1(("--eval" "(write-region \"foo\" nil \"~/file\")"))
  command-line()
  normal-top-level()
/home/sbaugh/file: sbaugh@earth (pid 1852838), Cannot resolve lock conflict in batch mode


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Signal-file-locked-on-lock-conflict-with-noninteract.patch --]
[-- Type: text/x-diff, Size: 1776 bytes --]

From 9f94d8975406a3f4bdaa97ed8bd6a08dbf8e3d9b Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Fri, 10 Nov 2023 07:20:09 -0500
Subject: [PATCH] Signal file-locked on lock conflict with noninteractive=t

Previously we would signal a generic error on lock conflict when
noninteractive=t.  That meant that non-interactively handling a lock
conflict would require catching all errors and checking the string in
DATA.

Now we just signal file-locked instead, which matches the interactive
behavior when the user says "q" at the prompt.

Also, when noninteractive, we signal before we write the prompt about
the lock conflict.  That prompt usually gets in the way of
noninteractively handling and suppress lock conflict errors.  The
signal data contains all the necessary information, we don't need to
write a separate message for noninteractive.

* lisp/userlock.el (ask-user-about-lock): Signal file-locked on
noninteractive lock conflict.  (bug#66993)
---
 lisp/userlock.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/userlock.el b/lisp/userlock.el
index 4623608f1db..91d5b7308dd 100644
--- a/lisp/userlock.el
+++ b/lisp/userlock.el
@@ -64,10 +64,11 @@ ask-user-about-lock
 			  (match-string 0 opponent)))
 	      opponent))
       (while (null answer)
+	(when noninteractive
+          (signal 'file-locked (list file opponent "Cannot resolve lock conflict in batch mode")))
         (message (substitute-command-keys
                   "%s locked by %s: (\\`s', \\`q', \\`p', \\`?')? ")
                  short-file short-opponent)
-	(if noninteractive (error "Cannot resolve lock conflict in batch mode"))
 	(let ((tem (let ((inhibit-quit t)
 			 (cursor-in-echo-area t))
 		     (prog1 (downcase (read-char))
-- 
2.39.2

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Fri, 10 Nov 2023 12:48:36 +0000 (UTC)
> Cc: Spencer Baugh <sbaugh@janestreet.com>, dmitry@gutov.dev,
> 	66993@debbugs.gnu.org
> 
> > Thanks, that's what I thought: this loses information.  The "Cannot
> > resolve lock conflict in batch mode" part is important, since it
> > explains the "file-locked" part.  So please include the missing text
> > in the list passed to 'signal' as its DATA argument, so as not to lose
> > this explanation.
> 
> OK, how about this?  Which makes Emacs print:
> 
> Error: file-locked ("/home/sbaugh/file" "sbaugh@earth (pid 1852838)" "Cannot resolve lock conflict in batch mode")
>   mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode -0x1f22147a251783b>))
>   debug-early-backtrace()
>   debug-early(error (file-locked "/home/sbaugh/file" "sbaugh@earth (pid 1852838)" "Cannot resolve lock conflict in batch mode"))
>   signal(file-locked ("/home/sbaugh/file" "sbaugh@earth (pid 1852838)" "Cannot resolve lock conflict in batch mode"))
>   ask-user-about-lock("/home/sbaugh/file" "sbaugh@earth (pid 1852838)")
>   write-region("foo" nil "~/file")
>   eval((write-region "foo" nil "~/file") t)
>   command-line-1(("--eval" "(write-region \"foo\" nil \"~/file\")"))
>   command-line()
>   normal-top-level()
> /home/sbaugh/file: sbaugh@earth (pid 1852838), Cannot resolve lock conflict in batch mode

Thanks, installed on master, and closing the bug.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 08/11/2023 14:22, Eli Zaretskii wrote:
>>> From: Spencer Baugh<sbaugh@janestreet.com>
>>> Date: Tue, 07 Nov 2023 16:28:04 -0500
>>>
>>> There are several features which will cause Emacs to frequently call
>>> project-current, and therefore call project-remember-project, and
>>> therefore sometimes call project--write-project-list whenever a new
>>> project is seen.
>> Why does project-current immediately writes the list to the file?  Why
>> cannot it keep the information in memory and write it only when the
>> session ends, or at some random rare opportunity?
>
> It could indeed be written from kill-emacs-hook, or just the next time
> an opportunity presents.

I like the idea of writing the file from kill-emacs-hook.  That seems
cleaner than writing the file at random times.

Alternatively, I suppose if we had a periodic timer that writes the
file, that timer could also do the somewhat useful maintenance work of
calling project-forget-zombie-projects.  Or maybe we could just call
that from kill-emacs-hook too?  Just a thought.

Separately: Currently, even without any locking issues, if there are
multiple Emacsen then project-list-file just contains the project--list
of the last one to write.  So they're constantly clobbering each others'
added and removed projects.  If we do the writes more rarely, then we
could try to read project-list-file first and add or remove the projects
that were added or removed during the life of this Emacs instance,
instead of just blindly writing out project--list.  Then if there are
multiple Emacsen around, their changes to project--list won't clobber
each other, they'll just be cleanly merged.  Does that sound reasonable?

Either way, I can implement this.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  66993@debbugs.gnu.org
> Date: Wed, 08 Nov 2023 10:36:12 -0500
> 
> Alternatively, I suppose if we had a periodic timer that writes the
> file, that timer could also do the somewhat useful maintenance work of
> calling project-forget-zombie-projects.  Or maybe we could just call
> that from kill-emacs-hook too?  Just a thought.

There's midnight.el for that kind of jobs.

> Separately: Currently, even without any locking issues, if there are
> multiple Emacsen then project-list-file just contains the project--list
> of the last one to write.  So they're constantly clobbering each others'
> added and removed projects.  If we do the writes more rarely, then we
> could try to read project-list-file first and add or remove the projects
> that were added or removed during the life of this Emacs instance,
> instead of just blindly writing out project--list.  Then if there are
> multiple Emacsen around, their changes to project--list won't clobber
> each other, they'll just be cleanly merged.  Does that sound reasonable?

I thought these writes were already just adding projects, not
clobbering the list.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 08/11/2023 18:32, Eli Zaretskii wrote:
> I thought these writes were already just adding projects, not
> clobbering the list.

The file is re-written in full every time e.g. to remove the previous 
mentions of a project that should now be moved to the top.

And also for simplicity.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> Date: Wed, 8 Nov 2023 23:04:50 +0200
> Cc: 66993@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 08/11/2023 18:32, Eli Zaretskii wrote:
> > I thought these writes were already just adding projects, not
> > clobbering the list.
> 
> The file is re-written in full every time e.g. to remove the previous 
> mentions of a project that should now be moved to the top.

I don't understand how this resolves the concern about clobbering.  If
each Emacs session just writes its list of projects to the file,
without refreshing the list due to additions done by another session,
they will still clobber each other's lists, no?  This could be avoided
if the writer would read the file before rewriting it.

Can you tell why the list of projects is saved on a file that is
global among all sessions of the same user?  Would it make sense to
make the file specific to an Emacs process?

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 09/11/2023 08:37, Eli Zaretskii wrote:
>> Date: Wed, 8 Nov 2023 23:04:50 +0200
>> Cc:66993@debbugs.gnu.org
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> On 08/11/2023 18:32, Eli Zaretskii wrote:
>>> I thought these writes were already just adding projects, not
>>> clobbering the list.
>> The file is re-written in full every time e.g. to remove the previous
>> mentions of a project that should now be moved to the top.
> I don't understand how this resolves the concern about clobbering.  If
> each Emacs session just writes its list of projects to the file,
> without refreshing the list due to additions done by another session,
> they will still clobber each other's lists, no?  This could be avoided
> if the writer would read the file before rewriting it.

Not resolves. Explains.

> Can you tell why the list of projects is saved on a file that is
> global among all sessions of the same user?  Would it make sense to
> make the file specific to an Emacs process?

Specific, meaning in a file name e.g. suffixed with pid?

And what happens after the restart? The list needs to be recovered.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> Date: Thu, 9 Nov 2023 13:05:09 +0200
> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> > Can you tell why the list of projects is saved on a file that is
> > global among all sessions of the same user?  Would it make sense to
> > make the file specific to an Emacs process?
> 
> Specific, meaning in a file name e.g. suffixed with pid?

For example, yes.

> And what happens after the restart? The list needs to be recovered.

Why does it need to be recovered?  Isn't it built on-the-fly anyway?
Is this just some kind of optimization?

I guess I don't understand well enough why this file exists and how it
is used?

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 09/11/2023 13:27, Eli Zaretskii wrote:
>> Date: Thu, 9 Nov 2023 13:05:09 +0200
>> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>>> Can you tell why the list of projects is saved on a file that is
>>> global among all sessions of the same user?  Would it make sense to
>>> make the file specific to an Emacs process?
>>
>> Specific, meaning in a file name e.g. suffixed with pid?
> 
> For example, yes.
> 
>> And what happens after the restart? The list needs to be recovered.
> 
> Why does it need to be recovered?  Isn't it built on-the-fly anyway?
> Is this just some kind of optimization?

If we didn't need to read it after restart, there would be no point in 
writing the list to disk.

> I guess I don't understand well enough why this file exists and how it
> is used?

Same reason as for savehist-file.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> Date: Thu, 9 Nov 2023 13:33:29 +0200
> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 09/11/2023 13:27, Eli Zaretskii wrote:
> >> Date: Thu, 9 Nov 2023 13:05:09 +0200
> >> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
> >> From: Dmitry Gutov <dmitry@gutov.dev>
> >>
> >> And what happens after the restart? The list needs to be recovered.
> > 
> > Why does it need to be recovered?  Isn't it built on-the-fly anyway?
> > Is this just some kind of optimization?
> 
> If we didn't need to read it after restart, there would be no point in 
> writing the list to disk.
> 
> > I guess I don't understand well enough why this file exists and how it
> > is used?
> 
> Same reason as for savehist-file.

It's a history of projects in the last session?  Then it's one more
reason to write the file only at exit, I guess?

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Thu, 9 Nov 2023 13:33:29 +0200
>> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
>> From: Dmitry Gutov <dmitry@gutov.dev>
>> 
>> On 09/11/2023 13:27, Eli Zaretskii wrote:
>> >> Date: Thu, 9 Nov 2023 13:05:09 +0200
>> >> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
>> >> From: Dmitry Gutov <dmitry@gutov.dev>
>> >>
>> >> And what happens after the restart? The list needs to be recovered.
>> > 
>> > Why does it need to be recovered?  Isn't it built on-the-fly anyway?
>> > Is this just some kind of optimization?
>> 
>> If we didn't need to read it after restart, there would be no point in 
>> writing the list to disk.
>> 
>> > I guess I don't understand well enough why this file exists and how it
>> > is used?
>> 
>> Same reason as for savehist-file.
>
> It's a history of projects in the last session?  Then it's one more
> reason to write the file only at exit, I guess?

This comparison to savehist-file actually makes me think: Why don't we
just make project--list into a history variable, and persist it like
savehist?

It would already be quite nice to have a history variable
project-history for project-prompter, I've been thinking about
implementing that.  It would be convenient for:

- switching repeatedly between two projects

- running a project command accidentally outside a project and wanting
to run it in the most recently used project.

But then, if we have project-history, could the project file just be a
persisted project-history?

If we had this model, project-remember-project would be basically
(push project 'project-history)

project-forget-zombie-projects would naturally not be needed, because
any zombie projects wouldn't be visited as part of the project-history,
and so if the history was limited in size, they'd eventually just drop
off the end.

project-switch-project would prompt for anything on project-history.

I think it simplifies things quite a lot!

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

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

Spencer Baugh <sbaugh@janestreet.com> writes:
> Eli Zaretskii <eliz@gnu.org> writes:
>>> Date: Thu, 9 Nov 2023 13:33:29 +0200
>>> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
>>> From: Dmitry Gutov <dmitry@gutov.dev>
>>> 
>>> On 09/11/2023 13:27, Eli Zaretskii wrote:
>>> >> Date: Thu, 9 Nov 2023 13:05:09 +0200
>>> >> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
>>> >> From: Dmitry Gutov <dmitry@gutov.dev>
>>> >>
>>> >> And what happens after the restart? The list needs to be recovered.
>>> > 
>>> > Why does it need to be recovered?  Isn't it built on-the-fly anyway?
>>> > Is this just some kind of optimization?
>>> 
>>> If we didn't need to read it after restart, there would be no point in 
>>> writing the list to disk.
>>> 
>>> > I guess I don't understand well enough why this file exists and how it
>>> > is used?
>>> 
>>> Same reason as for savehist-file.
>>
>> It's a history of projects in the last session?  Then it's one more
>> reason to write the file only at exit, I guess?
>
> This comparison to savehist-file actually makes me think: Why don't we
> just make project--list into a history variable, and persist it like
> savehist?
>
> It would already be quite nice to have a history variable
> project-history for project-prompter, I've been thinking about
> implementing that.  It would be convenient for:
>
> - switching repeatedly between two projects
>
> - running a project command accidentally outside a project and wanting
> to run it in the most recently used project.
>
> But then, if we have project-history, could the project file just be a
> persisted project-history?
>
> If we had this model, project-remember-project would be basically
> (push project 'project-history)
>
> project-forget-zombie-projects would naturally not be needed, because
> any zombie projects wouldn't be visited as part of the project-history,
> and so if the history was limited in size, they'd eventually just drop
> off the end.
>
> project-switch-project would prompt for anything on project-history.
>
> I think it simplifies things quite a lot!

Here's a patch which implements this.  It drops compatibility with the
existing project-file, just to demonstrate the simplicity of this
approach.

project--list is the new history variable.  Using it is mostly trivial;
a small bit of work is necessary in project-prompt-project-name to get
project-names for the history, but that worked out great.  Everything
else just seems to work.

And with savehist-mode enabled, project--list just gets stored
automatically without any code in project.el.

I can implement a backwards-compatible version if this seems like a
reasonable direction to go in.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Incompatibly-change-project-list-to-be-a-project-his.patch --]
[-- Type: text/x-patch, Size: 8994 bytes --]

From 86714ac9c82967e9d93e32d9bd172311fc4aed00 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Wed, 15 Nov 2023 15:44:03 -0500
Subject: [PATCH] Incompatibly change project--list to be a project history
 variable

With savehist-mode enabled, this Just Works.
---
 lisp/progmodes/project.el | 114 +++++++++++---------------------------
 1 file changed, 33 insertions(+), 81 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 95db9d0ef4c..59404e5729d 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1671,70 +1671,16 @@ project-kill-buffers
 \f
 ;;; Project list
 
-(defcustom project-list-file (locate-user-emacs-file "projects")
-  "File in which to save the list of known projects."
-  :type 'file
-  :version "28.1"
-  :group 'project)
-
-(defvar project--list 'unset
-  "List structure containing root directories of known projects.
-With some possible metadata (to be decided).")
-
-(defun project--read-project-list ()
-  "Initialize `project--list' using contents of `project-list-file'."
-  (let ((filename project-list-file))
-    (setq project--list
-          (when (file-exists-p filename)
-            (with-temp-buffer
-              (insert-file-contents filename)
-              (mapcar
-               (lambda (elem)
-                 (let ((name (car elem)))
-                   (list (if (file-remote-p name) name
-                           (abbreviate-file-name name)))))
-               (read (current-buffer))))))
-    (unless (seq-every-p
-             (lambda (elt) (stringp (car-safe elt)))
-             project--list)
-      (warn "Contents of %s are in wrong format, resetting"
-            project-list-file)
-      (setq project--list nil))))
-
-(defun project--ensure-read-project-list ()
-  "Initialize `project--list' if it isn't already initialized."
-  (when (eq project--list 'unset)
-    (project--read-project-list)))
-
-(defun project--write-project-list ()
-  "Save `project--list' in `project-list-file'."
-  (let ((filename project-list-file))
-    (with-temp-buffer
-      (insert ";;; -*- lisp-data -*-\n")
-      (let ((print-length nil)
-            (print-level nil))
-        (pp (mapcar (lambda (elem)
-                      (let ((name (car elem)))
-                        (list (if (file-remote-p name) name
-                                (expand-file-name name)))))
-                    project--list)
-            (current-buffer)))
-      (write-region nil nil filename nil 'silent))))
+(defvar project--list nil
+  "List of root directories of recently-accessed projects.")
 
 ;;;###autoload
-(defun project-remember-project (pr &optional no-write)
-  "Add project PR to the front of the project list.
-Save the result in `project-list-file' if the list of projects
-has changed, and NO-WRITE is nil."
-  (project--ensure-read-project-list)
+(defun project-remember-project (pr &optional _no-write)
+  "Add project PR to the front of the project list."
   (let ((dir (abbreviate-file-name (project-root pr))))
-    (unless (equal (caar project--list) dir)
-      (dolist (ent project--list)
-        (when (equal dir (car ent))
-          (setq project--list (delq ent project--list))))
-      (push (list dir) project--list)
-      (unless no-write
-        (project--write-project-list)))))
+    (unless (equal (car project--list) dir)
+      (setq project--list (delq dir project--list))
+      (push dir project--list))))
 
 (defun project--remove-from-project-list (project-root report-message)
   "Remove directory PROJECT-ROOT of a missing project from the project list.
@@ -1742,11 +1688,9 @@ project--remove-from-project-list
 result in `project-list-file'.  Announce the project's removal
 from the list using REPORT-MESSAGE, which is a format string
 passed to `message' as its first argument."
-  (project--ensure-read-project-list)
-  (when-let ((ent (assoc (abbreviate-file-name project-root) project--list)))
-    (setq project--list (delq ent project--list))
-    (message report-message project-root)
-    (project--write-project-list)))
+  (let ((dir (abbreviate-file-name project-root)))
+    (setq project--list (delq dir project--list))
+    (message report-message project-root)))
 
 ;;;###autoload
 (defun project-forget-project (project-root)
@@ -1762,7 +1706,6 @@ project-prompt-project-dir
 The project is chosen among projects known from the project list,
 see `project-list-file'.
 It's also possible to enter an arbitrary directory not in the list."
-  (project--ensure-read-project-list)
   (let* ((dir-choice "... (choose a dir)")
          (choices
           ;; XXX: Just using this for the category (for the substring
@@ -1772,43 +1715,55 @@ project-prompt-project-dir
          (pr-dir ""))
     (while (equal pr-dir "")
       ;; If the user simply pressed RET, do this again until they don't.
-      (setq pr-dir (completing-read "Select project: " choices nil t)))
+      (setq pr-dir (completing-read "Select project: " choices nil t nil 'project--list)))
     (if (equal pr-dir dir-choice)
         (read-directory-name "Select directory: " default-directory nil t)
       pr-dir)))
 
+(defvar project--name-history)
+
 (defun project-prompt-project-name ()
   "Prompt the user for a project, by name, that is one of the known project roots.
 The project is chosen among projects known from the project list,
 see `project-list-file'.
 It's also possible to enter an arbitrary directory not in the list."
   (let* ((dir-choice "... (choose a dir)")
+         project--name-history
          (choices
           (let (ret)
-            (dolist (dir (project-known-project-roots))
+            ;; Iterate in reverse order so project--name-history is in
+            ;; the correct order.
+            (dolist (dir (reverse project--list))
               ;; we filter out directories that no longer map to a project,
               ;; since they don't have a clean project-name.
-              (if-let (proj (project--find-in-directory dir))
-                  (push (cons (project-name proj) proj) ret)))
+              (when-let (proj (project--find-in-directory dir))
+                (let ((name (project-name proj)))
+                  (push name project--name-history)
+                  (push (cons name proj) ret))))
             ret))
          ;; XXX: Just using this for the category (for the substring
          ;; completion style).
          (table (project--file-completion-table
-                 (reverse (cons dir-choice choices))))
+                 (cons dir-choice choices)))
          (pr-name ""))
+    (setq project--name-history (delete-consecutive-dups project--name-history))
     (while (equal pr-name "")
       ;; If the user simply pressed RET, do this again until they don't.
-      (setq pr-name (completing-read "Select project: " table nil t)))
+      (setq pr-name
+            (completing-read "Select project: " table nil t nil 'project--name-history)))
     (if (equal pr-name dir-choice)
         (read-directory-name "Select directory: " default-directory nil t)
-      (let ((proj (assoc pr-name choices)))
-        (if (stringp proj) proj (project-root (cdr proj)))))))
+      (let* ((proj (assoc pr-name choices))
+             (ret (project-root (cdr proj))))
+        ;; Record this return value in history, since
+        ;; project--name-history is purely local.
+        (push ret project--list)
+        ret))))
 
 ;;;###autoload
 (defun project-known-project-roots ()
   "Return the list of root directories of all known projects."
-  (project--ensure-read-project-list)
-  (mapcar #'car project--list))
+  project--list)
 
 ;;;###autoload
 (defun project-execute-extended-command ()
@@ -1865,14 +1820,13 @@ project-remember-projects-under
 the progress.  The function returns the number of detected
 projects."
   (interactive "DDirectory: \nP")
-  (project--ensure-read-project-list)
   (let ((dirs (if recursive
                   (directory-files-recursively dir "" t)
                 (directory-files dir t)))
         (known (make-hash-table :size (* 2 (length project--list))
                                 :test #'equal))
         (count 0))
-    (dolist (project (mapcar #'car project--list))
+    (dolist (project project--list)
       (puthash project t known))
     (dolist (subdir dirs)
       (when-let (((file-directory-p subdir))
@@ -1885,7 +1839,6 @@ project-remember-projects-under
         (setq count (1+ count))))
     (if (zerop count)
         (message "No projects were found")
-      (project--write-project-list)
       (message "%d project%s were found"
                count (if (= count 1) "" "s")))
     count))
@@ -1916,7 +1869,6 @@ project-forget-projects-under
           (setq count (1+ count)))))
     (if (zerop count)
         (message "No projects were forgotten")
-      (project--write-project-list)
       (message "%d project%s were forgotten"
                count (if (= count 1) "" "s")))
     count))
-- 
2.39.3

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 15/11/2023 22:49, Spencer Baugh wrote:
> project--list is the new history variable.  Using it is mostly trivial;
> a small bit of work is necessary in project-prompt-project-name to get
> project-names for the history, but that worked out great.  Everything
> else just seems to work.
> 
> And with savehist-mode enabled, project--list just gets stored
> automatically without any code in project.el.
> 
> I can implement a backwards-compatible version if this seems like a
> reasonable direction to go in.

I haven't tried using it, but there's indeed a lot to like about this patch.

What happens if savehist-mode is nil, though? Which it is by default. 
For users with this setup the project history will just disappear.

What kind of backward compatibility did you have in mind?

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[sbaugh]

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

Dmitry Gutov <dmitry@gutov.dev> writes:
> On 15/11/2023 22:49, Spencer Baugh wrote:
>> project--list is the new history variable.  Using it is mostly trivial;
>> a small bit of work is necessary in project-prompt-project-name to get
>> project-names for the history, but that worked out great.  Everything
>> else just seems to work.
>> And with savehist-mode enabled, project--list just gets stored
>> automatically without any code in project.el.
>> I can implement a backwards-compatible version if this seems like a
>> reasonable direction to go in.
>
> I haven't tried using it, but there's indeed a lot to like about this patch.
>
> What happens if savehist-mode is nil, though? Which it is by
> default. For users with this setup the project history will just
> disappear.

Indeed.

> What kind of backward compatibility did you have in mind?

I was thinking about either keeping our code for saving to
project-list-file around in some obsoleted form, or using a subset of
the savehist code to save only project--list when it's not otherwise
enabled.

But actually, maybe it's time that we just enable savehist by default.
I'll try that first - I mailed emacs-devel about it.  Then we could
delete our project-list-file code once project.el eventually starts
requiring Emacs 30.

Regardless, how about the below patch which changes the default behavior
to only write project-list-file when Emacs exits?  I think this is a
useful first step no matter what else we do, since it matches the
behavior of savehist better.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Only-save-project-list-file-on-exit-by-default.patch --]
[-- Type: text/x-patch, Size: 5750 bytes --]

From 1e2ca2abf98f2589db384dc56e68ed4b8e73f1a3 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Sat, 18 Nov 2023 15:43:44 +0000
Subject: [PATCH] Only save project-list-file on exit by default.

Saving on most project commands can cause conflicts between Emacs
instances.  Saving on exit only is more reliable, and it matches the
behavior of savehist better, which we might use eventually instead of
our own code.

To allow opting in to the old behavior, add a new defcustom
project-list-file-save defaulting to on-exit.

* lisp/progmodes/project.el (project-list-file-save)
(project--project-list-changed, project--kill-emacs-hook):
Add. (bug#66993)
(kill-emacs-hook): Call project--kill-emacs-hook.
(project-remember-project, project--remove-from-project-list)
(project-remember-projects-under, project-forget-projects-under): Call
project--project-list-changed.
---
 etc/NEWS                  |  5 +++++
 lisp/progmodes/project.el | 45 +++++++++++++++++++++++++++++++--------
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 8324eb7da1e..bbb27183e96 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1062,6 +1062,11 @@ the current project.
 The look of the key prompt in the project switcher has been changed
 slightly.  To get the previous one, set this option to 'brackets'.
 
+*** New user option 'project-list-file-save', defaulting to on-exit.
+By default, 'project-list-file' is now only saved on exit.  To get the
+old behavior of saving whenever the project list changed, set this
+option to 'on-change' or 'on-change-and-exit'.
+
 *** 'project-try-vc' tries harder to find the responsible VCS.
 When 'project-vc-extra-root-markers' is non-nil, and causes
 subdirectory project to be detected which is not a VCS root, we now
diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 0855f76aa13..da2cec6abb2 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1713,6 +1713,21 @@ project--ensure-read-project-list
   (when (eq project--list 'unset)
     (project--read-project-list)))
 
+(defcustom project-list-file-save 'on-exit
+  "When to save `project-list-file'.
+
+If non-nil, the list of known projects is saved in
+`project-list-file'.
+
+If set to `on-change', the list is saved every time it changes.
+If set to `on-exit', the list is saved when Emacs exits.  If set
+to `on-change-and-exit', the list is saved in both cases."
+  :type '(choice (const :tag "Save on change" on-change)
+                 (const :tag "Save when exiting" on-exit)
+                 (const :tag "Save on change and when exiting" on-change-and-exit))
+  :group 'project
+  :version "30.1")
+
 (defun project--write-project-list ()
   "Save `project--list' in `project-list-file'."
   (let ((filename project-list-file))
@@ -1728,11 +1743,22 @@ project--write-project-list
             (current-buffer)))
       (write-region nil nil filename nil 'silent))))
 
+(defun project--project-list-changed ()
+  (when (memq project-list-file-save '(on-change on-change-and-exit))
+    (project--write-project-list)))
+
+(defun project--kill-emacs-hook ()
+  (when (memq project-list-file-save '(on-exit on-change-and-exit))
+    (project--write-project-list)))
+
+(add-hook 'kill-emacs-hook #'project--kill-emacs-hook)
+
 ;;;###autoload
 (defun project-remember-project (pr &optional no-write)
   "Add project PR to the front of the project list.
 Save the result in `project-list-file' if the list of projects
-has changed, and NO-WRITE is nil."
+has changed, `project-list-file-save' is configured to save on
+change, and NO-WRITE is nil."
   (project--ensure-read-project-list)
   (let ((dir (abbreviate-file-name (project-root pr))))
     (unless (equal (caar project--list) dir)
@@ -1741,19 +1767,20 @@ project-remember-project
           (setq project--list (delq ent project--list))))
       (push (list dir) project--list)
       (unless no-write
-        (project--write-project-list)))))
+        (project--project-list-changed)))))
 
 (defun project--remove-from-project-list (project-root report-message)
   "Remove directory PROJECT-ROOT of a missing project from the project list.
-If the directory was in the list before the removal, save the
-result in `project-list-file'.  Announce the project's removal
-from the list using REPORT-MESSAGE, which is a format string
-passed to `message' as its first argument."
+If the directory was in the list before the removal and
+`project-list-file-save' is configured to save on change, save
+the result in `project-list-file'.  Announce the project's
+removal from the list using REPORT-MESSAGE, which is a format
+string passed to `message' as its first argument."
   (project--ensure-read-project-list)
   (when-let ((ent (assoc (abbreviate-file-name project-root) project--list)))
     (setq project--list (delq ent project--list))
     (message report-message project-root)
-    (project--write-project-list)))
+    (project--project-list-changed)))
 
 ;;;###autoload
 (defun project-forget-project (project-root)
@@ -1892,7 +1919,7 @@ project-remember-projects-under
         (setq count (1+ count))))
     (if (zerop count)
         (message "No projects were found")
-      (project--write-project-list)
+      (project--project-list-changed)
       (message "%d project%s were found"
                count (if (= count 1) "" "s")))
     count))
@@ -1923,7 +1950,7 @@ project-forget-projects-under
           (setq count (1+ count)))))
     (if (zerop count)
         (message "No projects were forgotten")
-      (project--write-project-list)
+      (project--project-list-changed)
       (message "%d project%s were forgotten"
                count (if (= count 1) "" "s")))
     count))
-- 
2.42.1

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[sbaugh]

sbaugh@catern.com writes:
> Regardless, how about the below patch which changes the default behavior
> to only write project-list-file when Emacs exits?  I think this is a
> useful first step no matter what else we do, since it matches the
> behavior of savehist better.

(And plus it actually relates to and fixes this bug - I can take the
"use project--list as history" stuff to a different bug later)

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> From: sbaugh@catern.com
> Date: Sat, 18 Nov 2023 15:48:33 +0000 (UTC)
> Cc: Spencer Baugh <sbaugh@janestreet.com>, Eli Zaretskii <eliz@gnu.org>,
> 	66993@debbugs.gnu.org
> 
> > What happens if savehist-mode is nil, though? Which it is by
> > default. For users with this setup the project history will just
> > disappear.
> 
> Indeed.
> 
> > What kind of backward compatibility did you have in mind?
> 
> I was thinking about either keeping our code for saving to
> project-list-file around in some obsoleted form, or using a subset of
> the savehist code to save only project--list when it's not otherwise
> enabled.
> 
> But actually, maybe it's time that we just enable savehist by default.

Even if we decide to do that (and I'm not at all sure we should), how
would that solve the difficulty pointed out by Dmitry?  Even if
savehist is ON by default, the user could turn it OFF, right?

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: sbaugh@catern.com
>> Date: Sat, 18 Nov 2023 15:48:33 +0000 (UTC)
>> Cc: Spencer Baugh <sbaugh@janestreet.com>, Eli Zaretskii <eliz@gnu.org>,
>> 	66993@debbugs.gnu.org
>> 
>> > What happens if savehist-mode is nil, though? Which it is by
>> > default. For users with this setup the project history will just
>> > disappear.
>> 
>> Indeed.
>> 
>> > What kind of backward compatibility did you have in mind?
>> 
>> I was thinking about either keeping our code for saving to
>> project-list-file around in some obsoleted form, or using a subset of
>> the savehist code to save only project--list when it's not otherwise
>> enabled.
>> 
>> But actually, maybe it's time that we just enable savehist by default.
>
> Even if we decide to do that (and I'm not at all sure we should), how
> would that solve the difficulty pointed out by Dmitry?  Even if
> savehist is ON by default, the user could turn it OFF, right?

That's fine, we'd still be preserving backwards-compatibility:
project--list would still be saved by default.  The user could turn it
off, if they want, but that's not a problem - they can turn it off if
they want.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 18 Nov 2023 23:10:43 +0000 (UTC)
> Cc: dmitry@gutov.dev, sbaugh@janestreet.com, 66993@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >> But actually, maybe it's time that we just enable savehist by default.
> >
> > Even if we decide to do that (and I'm not at all sure we should), how
> > would that solve the difficulty pointed out by Dmitry?  Even if
> > savehist is ON by default, the user could turn it OFF, right?
> 
> That's fine, we'd still be preserving backwards-compatibility:
> project--list would still be saved by default.  The user could turn it
> off, if they want, but that's not a problem - they can turn it off if
> they want.

IMNSHO, it is a very bad idea to have one feature turn on and off as a
side effect of another, unrelated feature.  We should not have such
inelegant dependencies.  savehist is a general-purpose feature which
saves the history of important user actions, whereas the history of
projects is a completely different feature from a much more narrow
area of user activities.  They should not depend on one another, not
directly anyway.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@catern.com>
>> Date: Sat, 18 Nov 2023 23:10:43 +0000 (UTC)
>> Cc: dmitry@gutov.dev, sbaugh@janestreet.com, 66993@debbugs.gnu.org
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> >> But actually, maybe it's time that we just enable savehist by default.
>> >
>> > Even if we decide to do that (and I'm not at all sure we should), how
>> > would that solve the difficulty pointed out by Dmitry?  Even if
>> > savehist is ON by default, the user could turn it OFF, right?
>> 
>> That's fine, we'd still be preserving backwards-compatibility:
>> project--list would still be saved by default.  The user could turn it
>> off, if they want, but that's not a problem - they can turn it off if
>> they want.
>
> IMNSHO, it is a very bad idea to have one feature turn on and off as a
> side effect of another, unrelated feature.  We should not have such
> inelegant dependencies.  savehist is a general-purpose feature which
> saves the history of important user actions, whereas the history of
> projects is a completely different feature from a much more narrow
> area of user activities.  They should not depend on one another, not
> directly anyway.

project-prompt-project-dir minibuffer history is is in fact the same
feature as (for example) read-file-name minibuffer history, we just
didn't realize it before and so we unnecessarily implemented separate
logic for persisting it.  If savehist had been on by default when
project.el was added, there would have been no need for the separate
logic.  So it makes sense for them to be unified.

(They can still be turned off independently with savehist configuration,
of course, if the user wants to)

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 18/11/2023 18:26, Eli Zaretskii wrote:
>> From:sbaugh@catern.com
>> Date: Sat, 18 Nov 2023 15:48:33 +0000 (UTC)
>> Cc: Spencer Baugh<sbaugh@janestreet.com>, Eli Zaretskii<eliz@gnu.org>,
>> 	66993@debbugs.gnu.org
>>
>>> What happens if savehist-mode is nil, though? Which it is by
>>> default. For users with this setup the project history will just
>>> disappear.
>> Indeed.
>>
>>> What kind of backward compatibility did you have in mind?
>> I was thinking about either keeping our code for saving to
>> project-list-file around in some obsoleted form, or using a subset of
>> the savehist code to save only project--list when it's not otherwise
>> enabled.
>>
>> But actually, maybe it's time that we just enable savehist by default.
> Even if we decide to do that (and I'm not at all sure we should), how
> would that solve the difficulty pointed out by Dmitry?  Even if
> savehist is ON by default, the user could turn it OFF, right?

I think the working hypothesis is that they are the same feature: if 
project--list is used for saving the previous inputs, it can be covered 
by savehist-mode.

And if used explicitly turns off savehist-mode (after it's been made the 
default), that can mean they're fine with not storing all of those 
histories, including the projects one.

AFAIK project-known-project-roots is also used by some "splash page" 
packages to list the known projects for quick visiting, but that's not 
very different from using file-name-history to show a list of "last 
visited files", which also some code does.

So I was primarily worried about migration -- for those who didn't 
enable savehist-mode yet (perhaps due to not being aware of it), but 
have existing saved project histories.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> Date: Sun, 19 Nov 2023 16:31:22 +0200
> Cc: sbaugh@janestreet.com, 66993@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 18/11/2023 18:26, Eli Zaretskii wrote:
> > Even if we decide to do that (and I'm not at all sure we should), how
> > would that solve the difficulty pointed out by Dmitry?  Even if
> > savehist is ON by default, the user could turn it OFF, right?
> 
> I think the working hypothesis is that they are the same feature: if 
> project--list is used for saving the previous inputs, it can be covered 
> by savehist-mode.

Then maybe I'm missing something: savehist is AFAIU a feature that
saves minibuffer history, not just _any_ history.  Are we talking
about minibuffer history here?  I didn't think so.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 19/11/2023 17:23, Eli Zaretskii wrote:
>> Date: Sun, 19 Nov 2023 16:31:22 +0200
>> Cc:sbaugh@janestreet.com,66993@debbugs.gnu.org
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> On 18/11/2023 18:26, Eli Zaretskii wrote:
>>> Even if we decide to do that (and I'm not at all sure we should), how
>>> would that solve the difficulty pointed out by Dmitry?  Even if
>>> savehist is ON by default, the user could turn it OFF, right?
>> I think the working hypothesis is that they are the same feature: if
>> project--list is used for saving the previous inputs, it can be covered
>> by savehist-mode.
> Then maybe I'm missing something: savehist is AFAIU a feature that
> saves minibuffer history, not just_any_  history.

Perhaps it would be accurate to say that it saves minibuffer histories 
by default, but it can also save other history variables when 
'savehist-additional-variables' is customized.

> Are we talking
> about minibuffer history here?  I didn't think so.

Let me try to list the similarities and differences.

Similar:

- The contents of project--list come from interactions with the user. 
They're always a result of the user invoking a command. Sometimes the 
user will input the project's directory manually in the minibuffer, 
though in many/most cases that addition will be determined automatically 
by our code (and yet would still be added to the "project history" in 
project--list, because that looks like the most helpful approach for 
this list).
- The contents of project--list are, like file-name-history, used during 
completion when the user is asked to choose the project (when it 
couldn't be or wouldn't be selected automatically).

Different:

- When writing project--list to disk, we eliminate duplicate entries, 
both consecutive and distant ones. Again, seems more useful here.
- When file-name-history is used, something else is used as the 
completion table. Whereas project--list *is* the completion table. 
Whether it should be (in certain order) also used in the input history, 
is perhaps a potential future improvement.

So I don't know if project--list is a "minibuffer history", but 
depending on the POV one might decide it's close enough.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Eli Zaretskii]

> Date: Mon, 20 Nov 2023 04:05:58 +0200
> Cc: sbaugh@catern.com, sbaugh@janestreet.com, 66993@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> So I don't know if project--list is a "minibuffer history", but 
> depending on the POV one might decide it's close enough.

I still think they are different enough for project--list to be out of
scope of savehist.  But that's me.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 18/11/2023 17:48, sbaugh@catern.com wrote:
> +(defcustom project-list-file-save 'on-exit
> +  "When to save `project-list-file'.
> +
> +If non-nil, the list of known projects is saved in
> +`project-list-file'.
> +
> +If set to `on-change', the list is saved every time it changes.
> +If set to `on-exit', the list is saved when Emacs exits.  If set
> +to `on-change-and-exit', the list is saved in both cases."
> +  :type '(choice (const :tag "Save on change" on-change)
> +                 (const :tag "Save when exiting" on-exit)
> +                 (const :tag "Save on change and when exiting" on-change-and-exit))
> +  :group 'project
> +  :version "30.1")

I think I'd rather we change the behavior to straight away save on exit, 
no customization needed.

For most users the observable behavior will be the same, but we won't 
have to keep supporting the 'on-change' value later if/when we migrate 
to use savehist-mode instead.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Juri Linkov]

> But actually, maybe it's time that we just enable savehist by default.

Have you considered using define-multisession-variable?
What are pros and cons?

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Juri Linkov <juri@linkov.net> writes:
>> But actually, maybe it's time that we just enable savehist by default.
>
> Have you considered using define-multisession-variable?
> What are pros and cons?

That seems like a good idea too.  I'll try putting together a patch
which uses define-multisession-variable for project--list.

If project--list becomes a minibuffer history variable and also is a
multisession variable, though, perhaps savehist-mode would interact
poorly.  (Then again, maybe instead of enabling savehist mode by
default, we should just define-multisession-variable file-name-history?)

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 19/11/2023 21:38, Spencer Baugh wrote:
> If project--list becomes a minibuffer history variable and also is a
> multisession variable, though, perhaps savehist-mode would interact
> poorly.

I guess that, ultimately, it should be one or the other. Or neither.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 08/11/2023 17:36, Spencer Baugh wrote:
> Separately: Currently, even without any locking issues, if there are
> multiple Emacsen then project-list-file just contains the project--list
> of the last one to write.  So they're constantly clobbering each others'
> added and removed projects.  If we do the writes more rarely, then we
> could try to read project-list-file first and add or remove the projects
> that were added or removed during the life of this Emacs instance,
> instead of just blindly writing out project--list.  Then if there are
> multiple Emacsen around, their changes to project--list won't clobber
> each other, they'll just be cleanly merged.  Does that sound reasonable?
> 
> Either way, I can implement this.

If you're sure that the added utility is worth the complication in the 
code, why not.

Please post some performance measurements for before/after, though.

E.g. how long project--write-project-list takes without re-reading the 
file, and with the new, merging approach. Though it's probably okay.

BTW, that reminds me of the Fish shell's history merging between 
parallel sessions. I think it was automatic in some version, and now 
it's manual again (https://github.com/fish-shell/fish-shell/issues/825).

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 07/11/2023 23:28, Spencer Baugh wrote:
> - project-mode-line will call this on mode-line update
> 
> - My own private project-watch will call this based on file-notify
>    events.
> 
> If a user has multiple Emacs instances open using one or more of these
> features, it's fairly easy for both of the Emacs instances to see a
> new project at the same time.  In that case, they'll both call
> project--write-project-list at the same time, which will clash and run
> ask-user-about-lock.  This will happen frequently if the user is often
> looking at new projects.

Hmmm. What happens if we just show two buffers from different projects 
in the same Emacs instance? Do the mode-line refreshes trigger the 
changes in the list of remembered projects and subsequent writes to disk 
(e.g. alternating the "current" project between these two, potentially 
many times a second)?

If so, we'll need a way to inhibit these updates, at least from the 
mode-line formatter.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Spencer Baugh]

Dmitry Gutov <dmitry@gutov.dev> writes:
> On 07/11/2023 23:28, Spencer Baugh wrote:
>> - project-mode-line will call this on mode-line update
>> - My own private project-watch will call this based on file-notify
>>    events.
>> If a user has multiple Emacs instances open using one or more of
>> these
>> features, it's fairly easy for both of the Emacs instances to see a
>> new project at the same time.  In that case, they'll both call
>> project--write-project-list at the same time, which will clash and run
>> ask-user-about-lock.  This will happen frequently if the user is often
>> looking at new projects.
>
> Hmmm. What happens if we just show two buffers from different projects
> in the same Emacs instance? Do the mode-line refreshes trigger the
> changes in the list of remembered projects and subsequent writes to
> disk (e.g. alternating the "current" project between these two,
> potentially many times a second)?
>
> If so, we'll need a way to inhibit these updates, at least from the
> mode-line formatter.

Oh, I spoke too soon, perhaps - when MAYBE-PROMPT=nil in
project-current, we don't call project--write-project-list.

So the issue won't actually happen with project-mode-line or
project-uniquify-dirname-transform.  It will only happen with either:

- Concurrent calls to regular project commands
  (e.g. project-async-shell-command or project-compile) from two
  Emacsen,
  since those set MAYBE-PROMPT=t

- Concurrent calls to project-remember-project/project-forget-project
  and similar functions.  (This is what my project-watch does, since it
  calls project-remember-project when a new project shows up in a
  directory, and if I'm unlucky that will be at the same time as another
  Emacs instance.)

I guess that makes it less of a critical issue, but it still seems to me
that this is worth fixing.

bug#66993: [PATCH] project.el: avoid asking user about project-list-file lock

[Dmitry Gutov]

On 08/11/2023 17:25, Spencer Baugh wrote:
> - Concurrent calls to regular project commands
>    (e.g. project-async-shell-command or project-compile) from two
>    Emacsen,
>    since those set MAYBE-PROMPT=t

Since they are usually triggered manually, the user will have to try 
hard to launch them at the same time.

> - Concurrent calls to project-remember-project/project-forget-project
>    and similar functions.  (This is what my project-watch does, since it
>    calls project-remember-project when a new project shows up in a
>    directory, and if I'm unlucky that will be at the same time as another
>    Emacs instance.)

Note that if your code triggers the rewrites of the project-list file 
frequently enough so that the conflicts happen, doing more work during 
that time (e.g. reading, re-serializing, and then writing the file) 
might actually cause some disk/CPU usage increase. (Although if the 
conflicts happen due to synchronous firing of inotify watches in 
parallel Emacs instances, then probably not.)

BTW, the merging algorithm to use it is not very obvious (especially if 
we're going to do that in kill-emacs-hook). E.g. if one instance removed 
a project from history and another did not, we probably want to have it 
removed in the "merged" version.

> I guess that makes it less of a critical issue, but it still seems to me
> that this is worth fixing.

I'm glad to hear that the current design is working fine for the 
expected usage scenarios, but indeed, why not make it more resilient. If 
we don't sacrifice too much of something else.