Re: master 7b50ed5: Ask if dir and subdir dired buffers be killed when deleting dir

Re: master 7b50ed5: Ask if dir and subdir dired buffers be killed when deleting dir

[Basil L. Contovounesios]

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

tsdh@gnu.org (Tassilo Horn) writes:

> branch: master
> commit 7b50ed553faa6de6d51bf07d12d106ca61ab3ac4
> Author: Tassilo Horn <tsdh@gnu.org>
> Commit: Tassilo Horn <tsdh@gnu.org>
[...]
>     (dired-in-this-tree-p): Make obsolete in favor of file-in-directory-p
>     which actually does what the name suggest whereas dired-in-this-tree-p
>     is just string-matching on filenames which will fail with symlinks
>     filenames including ./ or ../.

There are still a few uses of dired-in-this-tree-p in dired-aux.el,
which give rise to byte-compilation warnings.  Is the attached patch
TRT, or are there any subtle differences between the functions that
still need to be addressed?

Thanks,

-- 
Basil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Remove-obsolete-uses-of-dired-in-this-tree-p.patch --]
[-- Type: text/x-diff, Size: 3871 bytes --]

From e3e91263438d378047c1c3e37faa8a815d182683 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sat, 12 Jun 2021 12:39:35 +0100
Subject: [PATCH] Remove obsolete uses of dired-in-this-tree-p

* lisp/dired.el (dired-in-this-tree-p): Promote commentary to
docstring.  Replace make-obsolete with equivalent declare form.

* lisp/dired-aux.el (dired-rename-subdir, dired-rename-subdir-1)
(dired-insert-subdir, dired-insert-subdir-validate)
(dired-kill-tree, dired-tree-down): Replace obsolete
dired-in-this-tree-p with its moral equivalent file-in-directory-p.
---
 lisp/dired-aux.el | 12 ++++++------
 lisp/dired.el     |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 3a721cd4d9..7d50b60000 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -1859,7 +1859,7 @@ dired-rename-subdir
     (while blist
       (with-current-buffer (car blist)
 	(if (and buffer-file-name
-		 (dired-in-this-tree-p buffer-file-name expanded-from-dir))
+                 (file-in-directory-p buffer-file-name expanded-from-dir))
 	    (let ((modflag (buffer-modified-p))
 		  (to-file (replace-regexp-in-string
 			    (concat "^" (regexp-quote from-dir))
@@ -1878,7 +1878,7 @@ dired-rename-subdir-1
     (while alist
       (setq elt (car alist)
 	    alist (cdr alist))
-      (if (dired-in-this-tree-p (car elt) expanded-dir)
+      (if (file-in-directory-p (car elt) expanded-dir)
 	  ;; ELT's subdir is affected by the rename
 	  (dired-rename-subdir-2 elt dir to)))
     (if (equal dir default-directory)
@@ -2707,7 +2707,7 @@ dired-insert-subdir
       (setq switches (string-replace "R" "" switches))
       (dolist (cur-ass dired-subdir-alist)
 	(let ((cur-dir (car cur-ass)))
-	  (and (dired-in-this-tree-p cur-dir dirname)
+          (and (file-in-directory-p cur-dir dirname)
 	       (let ((cur-cons (assoc-string cur-dir dired-switches-alist)))
 		 (if cur-cons
 		     (setcdr cur-cons switches)
@@ -2719,7 +2719,7 @@ dired-insert-subdir
 (defun dired-insert-subdir-validate (dirname &optional switches)
   ;; Check that it is valid to insert DIRNAME with SWITCHES.
   ;; Signal an error if invalid (e.g. user typed `i' on `..').
-  (or (dired-in-this-tree-p dirname (expand-file-name default-directory))
+  (or (file-in-directory-p dirname (expand-file-name default-directory))
       (error  "%s: not in this directory tree" dirname))
   (let ((real-switches (or switches dired-subdir-switches)))
     (when real-switches
@@ -2760,7 +2760,7 @@ dired-kill-tree
       (setq dir (car (car s-alist))
 	    s-alist (cdr s-alist))
       (and (or kill-root (not (string-equal dir dirname)))
-	   (dired-in-this-tree-p dir dirname)
+           (file-in-directory-p dir dirname)
 	   (dired-goto-subdir dir)
 	   (setq m-alist (nconc (dired-kill-subdir remember-marks) m-alist))))
     m-alist))
@@ -2992,7 +2992,7 @@ dired-tree-down
       (while rest
 	(setq elt (car rest)
 	      rest (cdr rest))
-	(if (dired-in-this-tree-p (directory-file-name (car elt)) dir)
+        (if (file-in-directory-p (directory-file-name (car elt)) dir)
 	    (setq rest nil
 		  pos (dired-goto-subdir (car elt))))))
     (if pos
diff --git a/lisp/dired.el b/lisp/dired.el
index bb428e2198..8c7bff31cb 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2912,10 +2912,10 @@ dired-unadvertise
 ;;; utility functions
 
 (defun dired-in-this-tree-p (file dir)
-  ;;"Is FILE part of the directory tree starting at DIR?"
+  "Return non-nil if FILE is part of the directory tree starting at DIR."
+  (declare (obsolete file-in-directory-p "28.1"))
   (let (case-fold-search)
     (string-match-p (concat "^" (regexp-quote dir)) file)))
-(make-obsolete 'dired-in-this-tree-p 'file-in-directory-p "28.1")
 (define-obsolete-function-alias 'dired-in-this-tree
   'dired-in-this-tree-p "27.1")
 
-- 
2.30.2

Re: master 7b50ed5: Ask if dir and subdir dired buffers be killed when deleting dir

[Tassilo Horn]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

Hi Basil,

>> branch: master
>> commit 7b50ed553faa6de6d51bf07d12d106ca61ab3ac4
>> Author: Tassilo Horn <tsdh@gnu.org>
>> Commit: Tassilo Horn <tsdh@gnu.org>
> [...]
>>     (dired-in-this-tree-p): Make obsolete in favor of file-in-directory-p
>>     which actually does what the name suggest whereas dired-in-this-tree-p
>>     is just string-matching on filenames which will fail with symlinks
>>     filenames including ./ or ../.
>
> There are still a few uses of dired-in-this-tree-p in dired-aux.el,
> which give rise to byte-compilation warnings.

Oh, I somehow missed those.

> Is the attached patch TRT, or are there any subtle differences between
> the functions that still need to be addressed?

Yes, I think they are equivalent in all cases dired cares about and
file-in-directory-p stays correct also in cases where
dired-in-this-tree-p will give a wrong answer.

But sorry, I've read your mail on my phone without noticing the patch.
I've done basically the same right now before actually reading your mail
again in a proper MUA.

I haven't done this:

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/dired.el b/lisp/dired.el
index bb428e2198..8c7bff31cb 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2912,10 +2912,10 @@ dired-unadvertise
 ;;; utility functions
 
 (defun dired-in-this-tree-p (file dir)
-  ;;"Is FILE part of the directory tree starting at DIR?"
+  "Return non-nil if FILE is part of the directory tree starting at DIR."
+  (declare (obsolete file-in-directory-p "28.1"))
   (let (case-fold-search)
     (string-match-p (concat "^" (regexp-quote dir)) file)))
-(make-obsolete 'dired-in-this-tree-p 'file-in-directory-p "28.1")
 (define-obsolete-function-alias 'dired-in-this-tree
   'dired-in-this-tree-p "27.1")
--8<---------------cut here---------------end--------------->8---

Can you tell me the difference between this and `make-obsolete'?  And
will that also trigger a warning during byte-compilation of code using
the function or only when it is actually called?  Feel free to do that
change if you think it is better.

Bye,
Tassilo

Re: master 7b50ed5: Ask if dir and subdir dired buffers be killed when deleting dir

[Basil L. Contovounesios]

Tassilo Horn <tsdh@gnu.org> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:

> But sorry, I've read your mail on my phone without noticing the patch.
> I've done basically the same right now before actually reading your mail
> again in a proper MUA.

Thanks, that's all that matters :).

> I haven't done this:
>
> diff --git a/lisp/dired.el b/lisp/dired.el
> index bb428e2198..8c7bff31cb 100644
> --- a/lisp/dired.el
> +++ b/lisp/dired.el
> @@ -2912,10 +2912,10 @@ dired-unadvertise
>  ;;; utility functions
>  
>  (defun dired-in-this-tree-p (file dir)
> -  ;;"Is FILE part of the directory tree starting at DIR?"
> +  "Return non-nil if FILE is part of the directory tree starting at DIR."
> +  (declare (obsolete file-in-directory-p "28.1"))
>    (let (case-fold-search)
>      (string-match-p (concat "^" (regexp-quote dir)) file)))
> -(make-obsolete 'dired-in-this-tree-p 'file-in-directory-p "28.1")
>  (define-obsolete-function-alias 'dired-in-this-tree
>    'dired-in-this-tree-p "27.1")
>
> Can you tell me the difference between this and `make-obsolete'?

The only difference AFAIK is that the former is translated into the
latter; see defun-declarations-alist, in particular
byte-run--set-obsolete.  You can verify this by invoking
'M-x pp-macroexpand-last-sexp' with point after the relevant defun.

So it's just an aesthetic preference of mine for keeping function
properties within the function's definition if possible, as it feels
more atomic and declarative.

> And will that also trigger a warning during byte-compilation of code
> using the function or only when it is actually called?

The same byte-compilation warnings should be omitted as with
make-obsolete.

> Feel free to do that change if you think it is better.

Not a big deal; it was just an opportunistic change ;).

Thanks,

-- 
Basil