* bug#54588: 29.0.50; [PATCH] Don't use `eshell-convert' when not needed / Fix setting umask in Eshell
@ 2022-03-26 22:40 Jim Porter
2022-03-29 14:51 ` Lars Ingebrigtsen
0 siblings, 1 reply; 3+ messages in thread
From: Jim Porter @ 2022-03-26 22:40 UTC (permalink / raw)
To: 54588
[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]
There are two tightly-coupled reasons for this bug: first, I want to
make some future improvements to `eshell-convert', so to prevent any
potential for breakage, I wanted to reduce the number of places that use
it when a simpler alternative exists.
Second, there's an obscure bug in setting the umask in Eshell when you
pass it an actual number (as opposed to a numeric string). From "emacs
-Q --eval '(eshell)'":
~ $ umask
002
~ $ umask 222
Warning: umask changed for all new files created by Emacs.
~ $ umask
222
~ $ umask $(identity #o222)
Warning: umask changed for all new files created by Emacs.
~ $ umask
146
The code is pretty complex, so I'll explain what's happening under the
hood. When calling `umask 222', the "222" is converted to a decimal
number by Eshell and passed to `eshell/umask'; then,
`eshell-eval-using-options' converts that number (again, in decimal)
back to a string. Next, `eshell/umask' calls `eshell-convert' to convert
it *back* to a decimal number. If that worked, it calls
`number-to-string' to convert it to a string again, then turns it into a
character escape sequence like "?\222" and finally calls
`read-from-string' on that to get a number.
The `umask $(identity #o222)' case is similar, except that Eshell
doesn't need to do the initial string-to-number conversion. However,
then `eshell-eval-using-options' gets confused since it converts the
value to a decimal string, throwing off the subsequent conversions.
In my patch, the behavior is changed as follows. First, when calling
`umask 222', the "222" is passed as a string, with no conversion. Next,
`eshell-eval-using-options' is set to preserve arguments, so if you pass
an actual number (as in `umask $(identity #o222)'), it doesn't try to
stringify it. Then, if the argument is a string, `eshell/umask' converts
it to an octal number. Finally, the numeric argument is used to set the
umask.
I also simplified the code for printing the umask a bit. That part just
splits the symbolic and non-symbolic cases up so there's less duplicated
work, plus simplifies the `format' call for the non-symbolic case.
[-- Attachment #2: 0001-Add-tests-for-Eshell-s-umask-command.patch --]
[-- Type: text/plain, Size: 3683 bytes --]
From 99e344d7492384991c35ae3ee909e6e4b8a7f068 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 26 Mar 2022 15:09:51 -0700
Subject: [PATCH 1/2] Add tests for Eshell's umask command
'em-basic-test/umask-set' fails when passing an actual number to the
command, but this is fixed in the subsequent commit.
test/lisp/eshell/em-basic-tests.el: New file.
---
test/lisp/eshell/em-basic-tests.el | 71 ++++++++++++++++++++++++++++++
1 file changed, 71 insertions(+)
create mode 100644 test/lisp/eshell/em-basic-tests.el
diff --git a/test/lisp/eshell/em-basic-tests.el b/test/lisp/eshell/em-basic-tests.el
new file mode 100644
index 0000000000..7a24f8b46c
--- /dev/null
+++ b/test/lisp/eshell/em-basic-tests.el
@@ -0,0 +1,71 @@
+;;; em-basic-tests.el --- em-basic test suite -*- lexical-binding:t -*-
+
+;; Copyright (C) 2022 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs. If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Tests for basic Eshell commands.
+
+;;; Code:
+
+(require 'ert)
+(require 'em-basic)
+
+(require 'eshell-tests-helpers
+ (expand-file-name "eshell-tests-helpers"
+ (file-name-directory (or load-file-name
+ default-directory))))
+
+;;; Tests:
+
+(ert-deftest em-basic-test/umask-print-numeric ()
+ "Test printing umask numerically."
+ (cl-letf (((symbol-function 'default-file-modes) (lambda () #o775)))
+ (should (equal (eshell-test-command-result "umask") "002\n")))
+ (cl-letf (((symbol-function 'default-file-modes) (lambda () #o654)))
+ (should (equal (eshell-test-command-result "umask") "123\n")))
+ ;; Make sure larger numbers don't cause problems.
+ (cl-letf (((symbol-function 'default-file-modes) (lambda () #o1775)))
+ (should (equal (eshell-test-command-result "umask") "002\n"))))
+
+(ert-deftest em-basic-test/umask-read-symbolic ()
+ "Test printing umask symbolically."
+ (cl-letf (((symbol-function 'default-file-modes) (lambda () #o775)))
+ (should (equal (eshell-test-command-result "umask -S")
+ "u=rwx,g=rwx,o=rx\n")))
+ (cl-letf (((symbol-function 'default-file-modes) (lambda () #o654)))
+ (should (equal (eshell-test-command-result "umask -S")
+ "u=wx,g=rx,o=x\n")))
+ ;; Make sure larger numbers don't cause problems.
+ (cl-letf (((symbol-function 'default-file-modes) (lambda () #o1775)))
+ (should (equal (eshell-test-command-result "umask -S")
+ "u=rwx,g=rwx,o=rx\n"))))
+
+(ert-deftest em-basic-test/umask-set ()
+ "Test setting umask."
+ (let ((file-modes 0))
+ (cl-letf (((symbol-function 'set-default-file-modes)
+ (lambda (mode) (setq file-modes mode))))
+ (eshell-test-command-result "umask 002")
+ (should (= file-modes #o775))
+ (eshell-test-command-result "umask 123")
+ (should (= file-modes #o654))
+ (eshell-test-command-result "umask $(identity #o222)")
+ (should (= file-modes #o555)))))
+
+;; em-basic-tests.el ends here
--
2.25.1
[-- Attachment #3: 0002-Don-t-use-eshell-convert-when-all-we-want-is-a-numbe.patch --]
[-- Type: text/plain, Size: 3888 bytes --]
From f529c143fd98510744c362425d9c8d723cfeca82 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 26 Mar 2022 15:12:48 -0700
Subject: [PATCH 2/2] Don't use 'eshell-convert' when all we want is a number
* lisp/eshell/em-hist.el (eshell/history): Use 'string-to-number'
instead of 'eshell-convert'.
* lisp/eshell/em-basic.el (eshell/umask): Simplify implementation and
be more careful about parsing numeric umasks to set.
---
lisp/eshell/em-basic.el | 56 ++++++++++++++++++++---------------------
lisp/eshell/em-hist.el | 2 +-
2 files changed, 28 insertions(+), 30 deletions(-)
diff --git a/lisp/eshell/em-basic.el b/lisp/eshell/em-basic.el
index ba868cee59..448b6787ee 100644
--- a/lisp/eshell/em-basic.el
+++ b/lisp/eshell/em-basic.el
@@ -155,39 +155,37 @@ eshell/umask
"umask" args
'((?S "symbolic" nil symbolic-p "display umask symbolically")
(?h "help" nil nil "display this usage message")
+ :preserve-args
:usage "[-S] [mode]")
- (if (or (not args) symbolic-p)
- (let ((modstr
- (concat "000"
- (format "%o"
- (logand (lognot (default-file-modes))
- 511)))))
- (setq modstr (substring modstr (- (length modstr) 3)))
- (when symbolic-p
- (let ((mode (default-file-modes)))
- (setq modstr
- (format
- "u=%s,g=%s,o=%s"
- (concat (and (= (logand mode 64) 64) "r")
- (and (= (logand mode 128) 128) "w")
- (and (= (logand mode 256) 256) "x"))
- (concat (and (= (logand mode 8) 8) "r")
- (and (= (logand mode 16) 16) "w")
- (and (= (logand mode 32) 32) "x"))
- (concat (and (= (logand mode 1) 1) "r")
- (and (= (logand mode 2) 2) "w")
- (and (= (logand mode 4) 4) "x"))))))
- (eshell-printn modstr))
- (setcar args (eshell-convert (car args)))
- (if (numberp (car args))
- (set-default-file-modes
- (- 511 (car (read-from-string
- (concat "?\\" (number-to-string (car args)))))))
- (error "Setting umask symbolically is not yet implemented"))
+ (cond
+ (symbolic-p
+ (let ((mode (default-file-modes)))
+ (eshell-printn
+ (format "u=%s,g=%s,o=%s"
+ (concat (and (= (logand mode 64) 64) "r")
+ (and (= (logand mode 128) 128) "w")
+ (and (= (logand mode 256) 256) "x"))
+ (concat (and (= (logand mode 8) 8) "r")
+ (and (= (logand mode 16) 16) "w")
+ (and (= (logand mode 32) 32) "x"))
+ (concat (and (= (logand mode 1) 1) "r")
+ (and (= (logand mode 2) 2) "w")
+ (and (= (logand mode 4) 4) "x"))))))
+ ((not args)
+ (eshell-printn (format "%03o" (logand (lognot (default-file-modes))
+ #o777))))
+ (t
+ (when (stringp (car args))
+ (if (string-match "^[0-7]+$" (car args))
+ (setcar args (string-to-number (car args) 8))
+ (error "Setting umask symbolically is not yet implemented")))
+ (set-default-file-modes (- #o777 (car args)))
(eshell-print
- "Warning: umask changed for all new files created by Emacs.\n"))
+ "Warning: umask changed for all new files created by Emacs.\n")))
nil))
+(put 'eshell/umask 'eshell-no-numeric-conversions t)
+
(provide 'em-basic)
;; Local Variables:
diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index 16abf04489..a18127a547 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -341,7 +341,7 @@ eshell/history
(error "No history"))
(let (length file)
(when (and args (string-match "^[0-9]+$" (car args)))
- (setq length (min (eshell-convert (car args))
+ (setq length (min (string-to-number (car args))
(ring-length eshell-history-ring))
args (cdr args)))
(and length
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* bug#54588: 29.0.50; [PATCH] Don't use `eshell-convert' when not needed / Fix setting umask in Eshell
2022-03-26 22:40 bug#54588: 29.0.50; [PATCH] Don't use `eshell-convert' when not needed / Fix setting umask in Eshell Jim Porter
@ 2022-03-29 14:51 ` Lars Ingebrigtsen
2022-03-29 16:30 ` Jim Porter
0 siblings, 1 reply; 3+ messages in thread
From: Lars Ingebrigtsen @ 2022-03-29 14:51 UTC (permalink / raw)
To: Jim Porter; +Cc: 54588
Jim Porter <jporterbugs@gmail.com> writes:
> In my patch, the behavior is changed as follows. First, when calling
> `umask 222', the "222" is passed as a string, with no
> conversion. Next, `eshell-eval-using-options' is set to preserve
> arguments, so if you pass an actual number (as in `umask $(identity
> #o222)'), it doesn't try to stringify it. Then, if the argument is a
> string, `eshell/umask' converts it to an octal number. Finally, the
> numeric argument is used to set the umask.
>
> I also simplified the code for printing the umask a bit. That part
> just splits the symbolic and non-symbolic cases up so there's less
> duplicated work, plus simplifies the `format' call for the
> non-symbolic case.
Makes sense to me; pushed to Emacs 29.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 3+ messages in thread
* bug#54588: 29.0.50; [PATCH] Don't use `eshell-convert' when not needed / Fix setting umask in Eshell
2022-03-29 14:51 ` Lars Ingebrigtsen
@ 2022-03-29 16:30 ` Jim Porter
0 siblings, 0 replies; 3+ messages in thread
From: Jim Porter @ 2022-03-29 16:30 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 54588
On 3/29/2022 7:51 AM, Lars Ingebrigtsen wrote:
> Makes sense to me; pushed to Emacs 29.
Thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-29 16:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-26 22:40 bug#54588: 29.0.50; [PATCH] Don't use `eshell-convert' when not needed / Fix setting umask in Eshell Jim Porter
2022-03-29 14:51 ` Lars Ingebrigtsen
2022-03-29 16:30 ` Jim Porter
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).