bug#50410: Tramp does not honor default file modes in make-directory

bug#50410: Tramp does not honor default file modes in make-directory

[Stephen Gildea]

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

Package: emacs
Version: 28.0.50
Tags: patch
X-Debbugs-CC: michael.albinus@gmx.de


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Tramp: honor default file modes in make-directory --]
[-- Type: text/x-diff, Size: 6148 bytes --]

Tramp: honor default file modes in make-directory

* lisp/net/tramp-sh.el:
* lisp/net/tramp-adb.el:
* lisp/net/tramp-sudoedit.el:
* lisp/net/tramp-gvfs.el: Add support for default file modes to
relevant Tramp back ends for make-directory.
* test/lisp/net/tramp-tests.el (tramp-test13-make-directory-with-file-modes):
New test.

diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
index a2bf0afbf5..e57145e8e7 100644
--- a/lisp/net/tramp-sh.el
+++ b/lisp/net/tramp-sh.el
@@ -2449,8 +2449,9 @@ tramp-sh-handle-make-directory
     (tramp-flush-directory-properties
      v (if parents "/" (file-name-directory localname)))
     (tramp-barf-unless-okay
-     v (format "%s %s"
+     v (format "%s -m %#o %s"
 	       (if parents "mkdir -p" "mkdir")
+	       (default-file-modes)
 	       (tramp-shell-quote-argument localname))
      "Couldn't make directory %s" dir)))
 
diff --git a/lisp/net/tramp-adb.el b/lisp/net/tramp-adb.el
index 70dbfdb947..a35ac37a20 100644
--- a/lisp/net/tramp-adb.el
+++ b/lisp/net/tramp-adb.el
@@ -442,7 +442,9 @@ tramp-adb-handle-make-directory
 	  (make-directory par parents))))
     (tramp-flush-directory-properties v localname)
     (unless (or (tramp-adb-send-command-and-check
-		 v (format "mkdir %s" (tramp-shell-quote-argument localname)))
+		 v (format "mkdir -m %#o %s"
+			   (default-file-modes)
+			   (tramp-shell-quote-argument localname)))
 		(and parents (file-directory-p dir)))
       (tramp-error v 'file-error "Couldn't make directory %s" dir))))
 
diff --git a/lisp/net/tramp-sudoedit.el b/lisp/net/tramp-sudoedit.el
index 5895f1d25b..051d145c2a 100644
--- a/lisp/net/tramp-sudoedit.el
+++ b/lisp/net/tramp-sudoedit.el
@@ -597,6 +597,7 @@ tramp-sudoedit-handle-make-directory
      v (if parents "/" (file-name-directory localname)))
     (unless (tramp-sudoedit-send-command
 	     v (if parents '("mkdir" "-p") "mkdir")
+	     "-m" (format "%#o" (default-file-modes))
 	     (tramp-compat-file-name-unquote localname))
       (tramp-error v 'file-error "Couldn't make directory %s" dir))))
 
diff --git a/lisp/net/tramp-gvfs.el b/lisp/net/tramp-gvfs.el
index e4f54cf4c4..eb889bb4f2 100644
--- a/lisp/net/tramp-gvfs.el
+++ b/lisp/net/tramp-gvfs.el
@@ -1574,10 +1574,13 @@ tramp-gvfs-handle-make-directory
 	(when (and parents (not (file-directory-p ldir)))
 	  (make-directory ldir parents))
 	;; Just do it.
-	(unless (or (tramp-gvfs-send-command
-		     v "gvfs-mkdir" (tramp-gvfs-url-file-name dir))
-		    (and parents (file-directory-p dir)))
-	  (tramp-error v 'file-error "Couldn't make directory %s" dir))))))
+	(or (let ((mkdir-succeeded
+		   (tramp-gvfs-send-command
+		    v "gvfs-mkdir" (tramp-gvfs-url-file-name dir))))
+	      (if mkdir-succeeded (set-file-modes dir (default-file-modes)))
+	      mkdir-succeeded)
+	    (and parents (file-directory-p dir))
+	    (tramp-error v 'file-error "Couldn't make directory %s" dir))))))
 
 (defun tramp-gvfs-handle-rename-file
   (filename newname &optional ok-if-already-exists)
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index 9a9684dd73..249ee9e94d 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -2765,28 +2765,38 @@ tramp-test12-rename-file
 	    (ignore-errors (delete-directory source 'recursive))
 	    (ignore-errors (delete-directory target 'recursive))))))))
 
-(ert-deftest tramp-test13-make-directory ()
-  "Check `make-directory'.
-This tests also `file-directory-p' and `file-accessible-directory-p'."
-  (skip-unless (tramp--test-enabled))
-
-  (dolist (quoted (if (tramp--test-expensive-test) '(nil t) '(nil)))
+(defun tramp-test-make-directory-helper (test-default-file-modes-p)
+  "Helper test used by tramp-test13-make-directory* tests."
+  (dolist (quoted (if (and (tramp--test-expensive-test)
+                           (not test-default-file-modes-p))
+                      '(nil t)
+                    '(nil)))
     (let* ((tmp-name1 (tramp--test-make-temp-name nil quoted))
-	   (tmp-name2 (expand-file-name "foo/bar" tmp-name1)))
+	   (tmp-name2 (expand-file-name "foo/bar" tmp-name1))
+	   (unusual-file-mode-1 #o740)
+	   (unusual-file-mode-2 #o710))
       (unwind-protect
 	  (progn
-	    (make-directory tmp-name1)
+	    (with-file-modes unusual-file-mode-1
+	      (make-directory tmp-name1))
 	    (should-error
 	     (make-directory tmp-name1)
 	     :type 'file-already-exists)
 	    (should (file-directory-p tmp-name1))
 	    (should (file-accessible-directory-p tmp-name1))
+	    (and test-default-file-modes-p
+		 (should (equal (format "%#o" unusual-file-mode-1)
+				(format "%#o" (file-modes tmp-name1)))))
 	    (should-error
 	     (make-directory tmp-name2)
 	     :type 'file-error)
-	    (make-directory tmp-name2 'parents)
+	    (with-file-modes unusual-file-mode-2
+	      (make-directory tmp-name2 'parents))
 	    (should (file-directory-p tmp-name2))
 	    (should (file-accessible-directory-p tmp-name2))
+	    (and test-default-file-modes-p
+		 (should (equal (format "%#o" unusual-file-mode-2)
+				(format "%#o" (file-modes tmp-name2)))))
 	    ;; If PARENTS is non-nil, `make-directory' shall not
 	    ;; signal an error when DIR exists already.
 	    (make-directory tmp-name2 'parents))
@@ -2794,6 +2804,21 @@ tramp-test13-make-directory
 	;; Cleanup.
 	(ignore-errors (delete-directory tmp-name1 'recursive))))))
 
+(ert-deftest tramp-test13-make-directory ()
+  "Check `make-directory'.
+This tests also `file-directory-p' and `file-accessible-directory-p'."
+  (skip-unless (tramp--test-enabled))
+  (tramp-test-make-directory-helper nil))
+
+(ert-deftest tramp-test13-make-directory-with-file-modes ()
+  "Check that `make-directory' honors `default-file-modes'.
+This is a separate test from `tramp-test13-make-directory' because
+some backends cannot pass this test.  The \"smb\" backend fails
+unless the SMB server supports \"posix\" extensions.
+The \"adb\" backend fails on the /sdcard filesystem."
+  (skip-unless (tramp--test-enabled))
+  (tramp-test-make-directory-helper t))
+
 (ert-deftest tramp-test14-delete-directory ()
   "Check `delete-directory'."
   (skip-unless (tramp--test-enabled))

bug#50410: Tramp does not honor default file modes in make-directory

[Michael Albinus]

Stephen Gildea <stepheng+emacs@gildea.com> writes:

Hi Stephen,

> Tramp: honor default file modes in make-directory

Thanks for this. As discussed privately, I believe it is a good
extension to Tramp.

I gave it a short try, and it works as expected for the "mock" test
method. Please push to the master branch; perhaps you could also add a
short NEWS entry.

Next days I let run the test suite for the other Tramp methods.

Best regards, Michael.

bug#50410: Tramp does not honor default file modes in make-directory

[Michael Albinus]

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

Hi Stephen,
>
>> Tramp: honor default file modes in make-directory
>
> Thanks for this. As discussed privately, I believe it is a good
> extension to Tramp.
>
> I gave it a short try, and it works as expected for the "mock" test
> method. Please push to the master branch; perhaps you could also add a
> short NEWS entry.
>
> Next days I let run the test suite for the other Tramp methods.

The results are good. In tramp-test13-make-directory-with-file-modes
I have applied the same rules like in tramp-test20-file-modes, it looks now

--8<---------------cut here---------------start------------->8---
(ert-deftest tramp-test13-make-directory-with-file-modes ()
  "Check that `make-directory' honors `default-file-modes'.
This is a separate test from `tramp-test13-make-directory' because
some backends cannot pass this test."
  (skip-unless (tramp--test-enabled))
  (skip-unless
   (or (tramp--test-sh-p) (tramp--test-sshfs-p) (tramp--test-sudoedit-p)
       ;; Not all tramp-gvfs.el methods support changing the file mode.
       (and
	(tramp--test-gvfs-p)
	(string-match-p
	 "ftp" (file-remote-p tramp-test-temporary-file-directory 'method)))))
  (tramp-test-make-directory-helper t))
--8<---------------cut here---------------end--------------->8---

In tramp-adb.el I believe it would work as well, if the Android device
is rooted. But this we don't support.

From my POV, you could push it to master.

Best regards, Michael.

bug#50410: Tramp does not honor default file modes in make-directory

[Stephen Gildea]

Version: 28.1

>   Michael Albinus <michael.albinus@gmx.de> writes:
>   
>   From my POV, you could push it to master.

Done, commit 5ee6583cb2

bug#50410: Tramp does not honor default file modes in make-directory

[Michael Albinus]

Stephen Gildea <stepheng+emacs@gildea.com> writes:

Hi Stephen,

>>   From my POV, you could push it to master.
>
> Done, commit 5ee6583cb2

Thanks!

Now, that we have `tramp--test-supports-file-modes-p', we can go back to
just one test `tramp-test13-make-directory' I believe. I'll play a
little bit with this, and let run further Tramp tests. We'll see.

Best regards, Michael.