GNU bug report logs - #69606
[PATCH] Ensure default-directory exists when generating diff

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 69606 in the body.
You can then email your comments to 69606 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Ensure default-directory exists when generating diff
Date: Thu, 07 Mar 2024 09:27:48 +0000

[Message part 1 (text/plain, inline)]

When I generate a diff in some directory that I would afterwards delete,
the diff buffer appears to remain in the now non-existent directory
(according to `default-directory'), even if I want to generate a diff
for some other file.  The issue now is that make-process complains that
it cannot set the CWD when starting "diff", and instead fails with a
slightly confusing error message:

  start-process: Setting current directory: No such file or directory, /some/directory/that/doesnt/exist/anymore

This would fix the issue, but I don't think the solution is ideal:

[0001-Ensure-default-directory-exists-when-generating-di.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

Any other ideas?  Perhaps we should always set the default directory to
that 

-- 
	Philip Kaludercic on peregrine

Message #8 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Thu, 07 Mar 2024 11:19:10 +0100

Philip Kaludercic <philipk <at> posteo.net> writes:

Hi Philip,

> diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
> index a64fbc47853..47566dbc40b 100644
> --- a/lisp/vc/diff.el
> +++ b/lisp/vc/diff.el
> @@ -188,7 +188,9 @@ diff-no-select
>                                    (list (or old-alt old)
>                                          (or new-alt new)))))
>  		     " "))
> -	 (thisdir default-directory))
> +	 (thisdir (if (file-exists-p default-directory)
> +                      default-directory
> +                    (expand-file-name "~"))))
>      (with-current-buffer buf
>        (setq buffer-read-only t)
>        (buffer-disable-undo (current-buffer))

I would use temporary-file-directory (or even
small-temporary-file-directory). Spamming the home directory with
(temporary) diff files doesn't sound like a good idea.

> Any other ideas?  Perhaps we should always set the default directory to
> that

Perhaps, but I don't know whether there are undesired side effects, for
example when using relative file names. OTOH, if default-directory is
remote or unwritable, we might have problems anyway.

Best regards, Michael.

Message #11 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Fri, 08 Mar 2024 07:47:29 +0000

Michael Albinus <michael.albinus <at> gmx.de> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
> Hi Philip,
>
>> diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
>> index a64fbc47853..47566dbc40b 100644
>> --- a/lisp/vc/diff.el
>> +++ b/lisp/vc/diff.el
>> @@ -188,7 +188,9 @@ diff-no-select
>>                                    (list (or old-alt old)
>>                                          (or new-alt new)))))
>>  		     " "))
>> -	 (thisdir default-directory))
>> +	 (thisdir (if (file-exists-p default-directory)
>> +                      default-directory
>> +                    (expand-file-name "~"))))
>>      (with-current-buffer buf
>>        (setq buffer-read-only t)
>>        (buffer-disable-undo (current-buffer))
>
> I would use temporary-file-directory (or even
> small-temporary-file-directory). Spamming the home directory with
> (temporary) diff files doesn't sound like a good idea.

If I am not mistaken, the command does not create any files, the output
of diff is written directly into the *diff* buffer.

>> Any other ideas?  Perhaps we should always set the default directory to
>> that
>
> Perhaps, but I don't know whether there are undesired side effects, for
> example when using relative file names. OTOH, if default-directory is
> remote or unwritable, we might have problems anyway.

I don't quite understand how, but it seems that some specific sequence
of commands can start diff in a directory I just deleted, without this
being necessary.  The reason I was thinking about a more general
solution, is that something like

--8<---------------cut here---------------start------------->8---
(let ((default-directory "/this/does/not/exists"))
  (make-process :command '("true")))
--8<---------------cut here---------------end--------------->8---

will always fail, even though "true" doesn't use the current working
directory.  I think it would be useful to have some :fallback option for
these situations, to ensure that if `default-directory' doesn't exist,
any other directory should be used instead.

> Best regards, Michael.

-- 
	Philip Kaludercic on peregrine

Message #14 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Sat, 09 Mar 2024 17:45:56 +0100

Philip Kaludercic <philipk <at> posteo.net> writes:

Hi Philip,

> The reason I was thinking about a more general solution, is that
> something like
>
> (let ((default-directory "/this/does/not/exists"))
>   (make-process :command '("true")))
>
> will always fail, even though "true" doesn't use the current working
> directory.  I think it would be useful to have some :fallback option for
> these situations, to ensure that if `default-directory' doesn't exist,
> any other directory should be used instead.

This is a more general request than just make it work for "diff".

Well, there is a reason that `default-directory' isn't set to something
else behind your back, if it doesn't exist. Processes can use relative
file names as arguments, and it is always better to fail with an error
message instead of doing something unexpected you even don't know about.

Best regards, Michael.

Message #17 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Philip Kaludercic <philipk <at> posteo.net>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Tue, 11 Feb 2025 19:34:07 -0800

Michael Albinus <michael.albinus <at> gmx.de> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
> Hi Philip,
>
>> The reason I was thinking about a more general solution, is that
>> something like
>>
>> (let ((default-directory "/this/does/not/exists"))
>>   (make-process :command '("true")))
>>
>> will always fail, even though "true" doesn't use the current working
>> directory.  I think it would be useful to have some :fallback option for
>> these situations, to ensure that if `default-directory' doesn't exist,
>> any other directory should be used instead.
>
> This is a more general request than just make it work for "diff".
>
> Well, there is a reason that `default-directory' isn't set to something
> else behind your back, if it doesn't exist. Processes can use relative
> file names as arguments, and it is always better to fail with an error
> message instead of doing something unexpected you even don't know about.

I tend to agree with Michael that we shouldn't make our fundamentals to
DWIMy.

But this bug fix is clearly useful, though I'd also agree that we might
as well use a temporary directory for this instead.  I'd also set the
file mode to 600 so that we don't inadvertently leak user data to random
places on the file system.

Philip, could you update your patch along these lines and install?

Message #20 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Michael Albinus <michael.albinus <at> gmx.de>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Sun, 16 Feb 2025 14:07:36 +0000

[Message part 1 (text/plain, inline)]

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Michael Albinus <michael.albinus <at> gmx.de> writes:
>
>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>
>> Hi Philip,
>>
>>> The reason I was thinking about a more general solution, is that
>>> something like
>>>
>>> (let ((default-directory "/this/does/not/exists"))
>>>   (make-process :command '("true")))
>>>
>>> will always fail, even though "true" doesn't use the current working
>>> directory.  I think it would be useful to have some :fallback option for
>>> these situations, to ensure that if `default-directory' doesn't exist,
>>> any other directory should be used instead.
>>
>> This is a more general request than just make it work for "diff".
>>
>> Well, there is a reason that `default-directory' isn't set to something
>> else behind your back, if it doesn't exist. Processes can use relative
>> file names as arguments, and it is always better to fail with an error
>> message instead of doing something unexpected you even don't know about.
>
> I tend to agree with Michael that we shouldn't make our fundamentals to
> DWIMy.
>
> But this bug fix is clearly useful, though I'd also agree that we might
> as well use a temporary directory for this instead.  I'd also set the
> file mode to 600 so that we don't inadvertently leak user data to random
> places on the file system.

Temporary directories are 700 by default anyway, so I don't think we
need any additional provisions (especially seeing as if I am not
mistaken, diffing doesn't create any files), which would just bloat up
the patch.

> Philip, could you update your patch along these lines and install?

Sure, hope this is OK?  If so, I'll push it:

[0001-Ensure-default-directory-exists-when-generating-diff.patch (text/x-diff, attachment)]

Message #23 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Michael Albinus <michael.albinus <at> gmx.de>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Sun, 16 Feb 2025 14:21:19 +0000

Philip Kaludercic <philipk <at> posteo.net> writes:

> From 761ee105380c60bcf410e3f3f6e15af8073b1549 Mon Sep 17 00:00:00 2001
> From: Philip Kaludercic <philipk <at> posteo.net>
> Date: Thu, 7 Mar 2024 10:06:48 +0100
> Subject: [PATCH] Ensure default-directory exists when generating diff
>
> * lisp/vc/diff.el (diff-no-select): Fall back to a fresh temporary
> directory if 'default-directory' points to an invalid path.
> ---
>  lisp/vc/diff.el | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
> index 70497a97d56..1aa4f87d1ea 100644
> --- a/lisp/vc/diff.el
> +++ b/lisp/vc/diff.el
> @@ -188,7 +188,9 @@ diff-no-select
>                                    (list (or old-alt old)
>                                          (or new-alt new)))))
>  		     " "))
> -	 (thisdir default-directory))
> +	 (thisdir (if (file-exists-p default-directory)
> +                      default-directory
> +                    (make-temp-file "emacs-diff" t))))
>      (with-current-buffer buf
>        (setq buffer-read-only t)
>        (buffer-disable-undo (current-buffer))
> --
> 2.47.2

Can we please also remove the temporary directory when we're done?

Message #26 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Michael Albinus <michael.albinus <at> gmx.de>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Sun, 16 Feb 2025 16:17:58 +0000

[Message part 1 (text/plain, inline)]

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> From 761ee105380c60bcf410e3f3f6e15af8073b1549 Mon Sep 17 00:00:00 2001
>> From: Philip Kaludercic <philipk <at> posteo.net>
>> Date: Thu, 7 Mar 2024 10:06:48 +0100
>> Subject: [PATCH] Ensure default-directory exists when generating diff
>>
>> * lisp/vc/diff.el (diff-no-select): Fall back to a fresh temporary
>> directory if 'default-directory' points to an invalid path.
>> ---
>>  lisp/vc/diff.el | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
>> index 70497a97d56..1aa4f87d1ea 100644
>> --- a/lisp/vc/diff.el
>> +++ b/lisp/vc/diff.el
>> @@ -188,7 +188,9 @@ diff-no-select
>>                                    (list (or old-alt old)
>>                                          (or new-alt new)))))
>>  		     " "))
>> -	 (thisdir default-directory))
>> +	 (thisdir (if (file-exists-p default-directory)
>> +                      default-directory
>> +                    (make-temp-file "emacs-diff" t))))
>>      (with-current-buffer buf
>>        (setq buffer-read-only t)
>>        (buffer-disable-undo (current-buffer))
>> --
>> 2.47.2
>
> Can we please also remove the temporary directory when we're done?

Sure, this should do the job:

[Message part 2 (text/plain, inline)]

diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
index 1aa4f87d1ea..bb509d0d4e3 100644
--- a/lisp/vc/diff.el
+++ b/lisp/vc/diff.el
@@ -188,9 +188,10 @@ diff-no-select
                                   (list (or old-alt old)
                                         (or new-alt new)))))
 		     " "))
-	 (thisdir (if (file-exists-p default-directory)
-                      default-directory
-                    (make-temp-file "emacs-diff" t))))
+         (use-temp-dir (not (file-exists-p default-directory)))
+         (thisdir (if use-temp-dir
+                      (make-temp-file "emacs-diff" t)
+                    default-directory)))
     (with-current-buffer buf
       (setq buffer-read-only t)
       (buffer-disable-undo (current-buffer))
@@ -220,6 +221,8 @@ diff-no-select
 	   (call-process shell-file-name nil buf nil
 			 shell-command-switch command)
            old-alt new-alt))))
+    (when use-temp-dir
+      (delete-directory thisdir t))
     buf))
 
 (defun diff-process-filter (proc string)

[Message part 3 (text/plain, inline)]

Or would it be better to keep the directory around and re-use it?

Message #29 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Sun, 16 Feb 2025 19:31:23 +0100

Philip Kaludercic <philipk <at> posteo.net> writes:

> Or would it be better to keep the directory around and re-use it?

Pls don't.

I don't understand why you can't use temporary-file-directory. Did I
miss this point in the discussion?

Best regards, Michael.

Message #32 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Sun, 16 Feb 2025 21:18:45 +0000

[Message part 1 (text/plain, inline)]

Michael Albinus <michael.albinus <at> gmx.de> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Or would it be better to keep the directory around and re-use it?
>
> Pls don't.
>
> I don't understand why you can't use temporary-file-directory. Did I
> miss this point in the discussion?
>
> Best regards, Michael.

No, you are absolutely right, that is the better idea!

[0001-Ensure-default-directory-exists-when-generating-diff.patch (text/x-diff, attachment)]

Message #35 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Mon, 17 Feb 2025 12:02:53 +0100

[Message part 1 (text/plain, inline)]

Philip Kaludercic <philipk <at> posteo.net> writes:

Hi Philipp,

>> I don't understand why you can't use temporary-file-directory. Did I
>> miss this point in the discussion?
>
> No, you are absolutely right, that is the better idea!

Thanks. However, why not do it more simple, like appended?

Best regards, Michael.

[Message part 2 (text/x-patch, attachment)]

Message #38 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Mon, 17 Feb 2025 21:00:35 +0000

Michael Albinus <michael.albinus <at> gmx.de> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
> Hi Philipp,
>
>>> I don't understand why you can't use temporary-file-directory. Did I
>>> miss this point in the discussion?
>>
>> No, you are absolutely right, that is the better idea!
>
> Thanks. However, why not do it more simple, like appended?

Is it safe to always use a temporary directory?  I would have expected
there to be edge-cases when using TRAMP where that might matter, and we
should only fall back if really necessary?

> Best regards, Michael.
>
> diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
> index 70497a97d56..875deb68724 100644
> --- a/lisp/vc/diff.el
> +++ b/lisp/vc/diff.el
> @@ -187,8 +187,7 @@ diff-no-select
>  					       (prin1-to-string new))))
>                                    (list (or old-alt old)
>                                          (or new-alt new)))))
> -		     " "))
> -	 (thisdir default-directory))
> +		     " ")))
>      (with-current-buffer buf
>        (setq buffer-read-only t)
>        (buffer-disable-undo (current-buffer))
> @@ -199,25 +198,26 @@ diff-no-select
>        (setq-local revert-buffer-function
>                    (lambda (_ignore-auto _noconfirm)
>                      (diff-no-select old new switches no-async (current-buffer))))
> -      (setq default-directory thisdir)
> +      (setq default-directory temporary-file-directory)
>        (setq diff-default-directory default-directory)
>        (let ((inhibit-read-only t))
>  	(insert command "\n"))
> -      (if (and (not no-async) (fboundp 'make-process))
> -	  (let ((proc (start-process "Diff" buf shell-file-name
> -                                     shell-command-switch command)))
> -	    (set-process-filter proc #'diff-process-filter)
> -            (set-process-sentinel
> -             proc (lambda (proc _msg)
> -                    (with-current-buffer (process-buffer proc)
> -                      (diff-sentinel (process-exit-status proc)
> -                                     old-alt new-alt)))))
> -	;; Async processes aren't available.
> -	(let ((inhibit-read-only t))
> -	  (diff-sentinel
> -	   (call-process shell-file-name nil buf nil
> -			 shell-command-switch command)
> -           old-alt new-alt))))
> +      (with-file-modes #o600
> +        (if (and (not no-async) (fboundp 'make-process))
> +	    (let ((proc (start-process "Diff" buf shell-file-name
> +                                       shell-command-switch command)))
> +	      (set-process-filter proc #'diff-process-filter)
> +              (set-process-sentinel
> +               proc (lambda (proc _msg)
> +                      (with-current-buffer (process-buffer proc)
> +                        (diff-sentinel (process-exit-status proc)
> +                                       old-alt new-alt)))))
> +	  ;; Async processes aren't available.
> +	  (let ((inhibit-read-only t))
> +	    (diff-sentinel
> +	     (call-process shell-file-name nil buf nil
> +			   shell-command-switch command)
> +             old-alt new-alt)))))
>      buf))
>
>  (defun diff-process-filter (proc string)

Message #41 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Tue, 18 Feb 2025 08:11:45 +0100

Philip Kaludercic <philipk <at> posteo.net> writes:

Hi Philipp,

> Is it safe to always use a temporary directory?  I would have expected
> there to be edge-cases when using TRAMP where that might matter, and we
> should only fall back if really necessary?

There is no problem wrt temporary-file-directory and Tramp.

Best regards, Michael.

Message #44 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Thu, 20 Feb 2025 20:30:28 +0000

Michael Albinus <michael.albinus <at> gmx.de> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
> Hi Philipp,
>
>> Is it safe to always use a temporary directory?  I would have expected
>> there to be edge-cases when using TRAMP where that might matter, and we
>> should only fall back if really necessary?
>
> There is no problem wrt temporary-file-directory and Tramp.

Then we should go with that approach.  Do you want to push the change?

> Best regards, Michael.

Message #47 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Fri, 21 Feb 2025 12:18:56 +0100

Philip Kaludercic <philipk <at> posteo.net> writes:

Hi Philipp,
>>
>>> Is it safe to always use a temporary directory?  I would have expected
>>> there to be edge-cases when using TRAMP where that might matter, and we
>>> should only fall back if really necessary?
>>
>> There is no problem wrt temporary-file-directory and Tramp.
>
> Then we should go with that approach.  Do you want to push the change?

I thought you do :-) But no problem, I can do it.

The question is how we bring it to Emacs 30. Pushing to the emacs-30
branch (perhaps too late due to the release candidate). Or pushing to
master, and ask Srefan to add this to the to-be-merged-from-master-after-the-release-list.

Best regards, Michael.

Message #50 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>,
 Philip Kaludercic <philipk <at> posteo.net>
Cc: 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Fri, 21 Feb 2025 11:31:40 +0000

Michael Albinus <michael.albinus <at> gmx.de> writes:

> The question is how we bring it to Emacs 30. Pushing to the emacs-30
> branch (perhaps too late due to the release candidate). Or pushing to
> master, and ask Srefan to add this to the to-be-merged-from-master-after-the-release-list.

The latter is probably best at this point, thanks.

Message #55 received at 69606-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Philip Kaludercic <philipk <at> posteo.net>, 69606-done <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Fri, 21 Feb 2025 14:49:17 +0100

Version: 31.1

Stefan Kangas <stefankangas <at> gmail.com> writes:

>> The question is how we bring it to Emacs 30. Pushing to the emacs-30
>> branch (perhaps too late due to the release candidate). Or pushing to
>> master, and ask Srefan to add this to the to-be-merged-from-master-after-the-release-list.
>
> The latter is probably best at this point, thanks.

Pushed to master, closing the bug.

Best regards, Michael.

Message #58 received at 69606-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Philip Kaludercic <philipk <at> posteo.net>, 69606-done <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Fri, 21 Feb 2025 17:45:48 +0000

Michael Albinus <michael.albinus <at> gmx.de> writes:

> Version: 31.1
>
> Stefan Kangas <stefankangas <at> gmail.com> writes:
>
>>> The question is how we bring it to Emacs 30. Pushing to the emacs-30
>>> branch (perhaps too late due to the release candidate). Or pushing to
>>> master, and ask Srefan to add this to the to-be-merged-from-master-after-the-release-list.
>>
>> The latter is probably best at this point, thanks.
>
> Pushed to master, closing the bug.

Thanks.  I've cherry-picked it to a local branch that I will push to
emacs-30 once Emacs 30.1 is released.

Message #61 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Mon, 24 Feb 2025 19:24:34 +0200

>>>> Is it safe to always use a temporary directory?  I would have expected
>>>> there to be edge-cases when using TRAMP where that might matter, and we
>>>> should only fall back if really necessary?
>>>
>>> There is no problem wrt temporary-file-directory and Tramp.
>>
>> Then we should go with that approach.  Do you want to push the change?
>
> I thought you do :-) But no problem, I can do it.

It was completely unexpected surprise that now diff is broken
since 'C-x C-j' goes into an unrelated directory, but not
going from the diff buffer to the dired where it was created.

I didn't have the initial problem since using

  (add-hook 'diff-mode-hook 'rename-uniquely)

(I know that this pollutes with temp buffers in org-src-font-lock-fontify-block
that has ‘(get-buffer-create (format " *org-src-fontification:%s*" lang-mode))’
because it renames internal buffers, so they can't be reused.  But this is
not a problem.)

But even with the default settings of reusing the same diff buffer,
why change the default directory when it exists?  Why not to check
if the dir still exists like Philip proposed initially?

Message #64 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Tue, 25 Feb 2025 14:44:35 +0100

Juri Linkov <juri <at> linkov.net> writes:

Hi Juri,

> But even with the default settings of reusing the same diff buffer,
> why change the default directory when it exists?  Why not to check
> if the dir still exists like Philip proposed initially?

*diff* is an ephemeral buffer, there is no documented value of its
`default-directory'. I guess you describe a side-effect you're using.

And I cannot reproduce your problem. If I am in a dired buffer, and type
"=" (dired-diff), I get a *diff* buffer with the expected
`default-directory'. 'C-x C-j' works as expected for me.

Could you pls show a recipe, which highlights what is broken for you?

Best regards, Michael.

Message #67 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Juri Linkov <juri <at> linkov.net>
Cc: Michael Albinus <michael.albinus <at> gmx.de>,
 Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Wed, 26 Feb 2025 16:49:52 +0000

Juri Linkov <juri <at> linkov.net> writes:

>>>>> Is it safe to always use a temporary directory?  I would have expected
>>>>> there to be edge-cases when using TRAMP where that might matter, and we
>>>>> should only fall back if really necessary?
>>>>
>>>> There is no problem wrt temporary-file-directory and Tramp.
>>>
>>> Then we should go with that approach.  Do you want to push the change?
>>
>> I thought you do :-) But no problem, I can do it.
>
> It was completely unexpected surprise that now diff is broken
> since 'C-x C-j' goes into an unrelated directory, but not
> going from the diff buffer to the dired where it was created.
>
> I didn't have the initial problem since using
>
>   (add-hook 'diff-mode-hook 'rename-uniquely)
>
> (I know that this pollutes with temp buffers in org-src-font-lock-fontify-block
> that has ‘(get-buffer-create (format " *org-src-fontification:%s*" lang-mode))’
> because it renames internal buffers, so they can't be reused.  But this is
> not a problem.)
>
> But even with the default settings of reusing the same diff buffer,
> why change the default directory when it exists?  Why not to check
> if the dir still exists like Philip proposed initially?

I have also run into this issue when trying to save a diff buffer, and
it not appearing in the expected place but under /tmp/.

Message #70 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Wed, 26 Feb 2025 19:52:47 +0100

[Message part 1 (text/plain, inline)]

Philip Kaludercic <philipk <at> posteo.net> writes:

Hi Philip,

>> But even with the default settings of reusing the same diff buffer,
>> why change the default directory when it exists?  Why not to check
>> if the dir still exists like Philip proposed initially?
>
> I have also run into this issue when trying to save a diff buffer, and
> it not appearing in the expected place but under /tmp/.

As I said the other message, I cannot reproduce. I'm not a diff user (I
use ediff), so I need a recipe for triggering the issue.

However, what about the patch below?

Best regards, Michael.

[Message part 2 (text/x-patch, attachment)]

Message #73 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Wed, 26 Feb 2025 19:25:47 +0000

Michael Albinus <michael.albinus <at> gmx.de> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
> Hi Philip,
>
>>> But even with the default settings of reusing the same diff buffer,
>>> why change the default directory when it exists?  Why not to check
>>> if the dir still exists like Philip proposed initially?
>>
>> I have also run into this issue when trying to save a diff buffer, and
>> it not appearing in the expected place but under /tmp/.
>
> As I said the other message, I cannot reproduce. I'm not a diff user (I
> use ediff), so I need a recipe for triggering the issue.

In my case I compared two files in Dired using `dired-diff' and then
`default-directory' in the resulting buffer is "/tmp", not the same
directory as I had open using Dired.

> However, what about the patch below?
>
> Best regards, Michael.
>
> diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
> index 875deb68724..c8a1b7c0efa 100644
> --- a/lisp/vc/diff.el
> +++ b/lisp/vc/diff.el
> @@ -187,7 +187,8 @@ diff-no-select
>  					       (prin1-to-string new))))
>                                    (list (or old-alt old)
>                                          (or new-alt new)))))
> -		     " ")))
> +		     " "))
> +	 (thisdir default-directory))
>      (with-current-buffer buf
>        (setq buffer-read-only t)
>        (buffer-disable-undo (current-buffer))
> @@ -198,14 +199,15 @@ diff-no-select
>        (setq-local revert-buffer-function
>                    (lambda (_ignore-auto _noconfirm)
>                      (diff-no-select old new switches no-async (current-buffer))))
> -      (setq default-directory temporary-file-directory)
> +      (setq default-directory thisdir)
>        (setq diff-default-directory default-directory)
>        (let ((inhibit-read-only t))
>  	(insert command "\n"))
>        (with-file-modes #o600
>          (if (and (not no-async) (fboundp 'make-process))
> -	    (let ((proc (start-process "Diff" buf shell-file-name
> -                                       shell-command-switch command)))
> +	    (let* ((default-directory temporary-file-directory)
> +                   (proc (start-process "Diff" buf shell-file-name
> +                                        shell-command-switch command)))
>  	      (set-process-filter proc #'diff-process-filter)
>                (set-process-sentinel
>                 proc (lambda (proc _msg)
> @@ -213,7 +215,8 @@ diff-no-select
>                          (diff-sentinel (process-exit-status proc)
>                                         old-alt new-alt)))))
>  	  ;; Async processes aren't available.
> -	  (let ((inhibit-read-only t))
> +	  (let* ((default-directory temporary-file-directory)
> +                 (inhibit-read-only t))
>  	    (diff-sentinel
>  	     (call-process shell-file-name nil buf nil
>  			   shell-command-switch command)

I didn't try it, but it looks like it should fix the issue.

Message #76 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Wed, 26 Feb 2025 21:11:44 +0100

Philip Kaludercic <philipk <at> posteo.net> writes:

Hi Philip,

>> However, what about the patch below?

> I didn't try it, but it looks like it should fix the issue.

Since I don't use diff myself, it would be great if somebody veryfies
it. I don't want to push another fix to diff.el blindly.

Best regards, Michael.

Message #79 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Thu, 27 Feb 2025 09:48:36 +0200

>>> However, what about the patch below?
>
>> I didn't try it, but it looks like it should fix the issue.
>
> Since I don't use diff myself, it would be great if somebody veryfies
> it. I don't want to push another fix to diff.el blindly.

Thanks, I tried your patch, and confirm that the problem is fixed.
The steps to verify were just '=' (dired-diff) and 'C-x C-j' (dired-jump).

Message #82 received at 69606 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>, 69606 <at> debbugs.gnu.org
Subject: Re: bug#69606: [PATCH] Ensure default-directory exists when
 generating diff
Date: Fri, 28 Feb 2025 18:08:49 +0100

Juri Linkov <juri <at> linkov.net> writes:

Hi Juri,

>>>> However, what about the patch below?
>>
>>> I didn't try it, but it looks like it should fix the issue.
>>
>> Since I don't use diff myself, it would be great if somebody veryfies
>> it. I don't want to push another fix to diff.el blindly.
>
> Thanks, I tried your patch, and confirm that the problem is fixed.

Thanks for the feedback. I've pushed the change to the emacs-30 branch.

> The steps to verify were just '=' (dired-diff) and 'C-x C-j' (dired-jump).

That's exactly what I did. No idea, why the problem didn't appear to me.

Best regards, Michael.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.