bug#70271: 30.0.50; [PATCH] * lisp/ls-lisp.el (ls-lisp--sanitize-switches): support more

bug#70271: 30.0.50; [PATCH] * lisp/ls-lisp.el (ls-lisp--sanitize-switches): support more

[Lin Sun]

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

Hi,

The dired ignored the options like "--time=ctime" in the variable
`dired-listing-switches' on Windows OS.

 I traced the code and found dired will use the functions in the
`ls-lisp.el' instead of calling the real command "/usr/bin/ls", and
the "ls-lisp.el" didn't process the "--time=ctime" option.

The attached PATCH will convert the "--time=ctime" to "-c"... in the
"ls-lisp.el", and then the dired will display the desired time in its
time column.

Here is the verification code (on Windows OS), try it one line and
close the dired buffer then try next line.

(dired "/tmp/aa" "-al --sort=time") ; as "-al -t"

(dired "/tmp/aa" "-al --time=ctime") ; as "al -c"

(dired "/tmp/aa" "-al --time=atime") ; as "al -u"

Please help review the changes and approve them. Thanks

[-- Attachment #2: 0001-lisp-ls-lisp.el-ls-lisp-sanitize-switches-support-mo.patch --]
[-- Type: text/x-patch, Size: 1222 bytes --]

From 2044be09357c46af13ed341bafbcc5737bd40c42 Mon Sep 17 00:00:00 2001
From: Lin Sun <sunlin7@hotmail.com>
Date: Mon, 8 Apr 2024 06:59:21 +0000
Subject: [PATCH] * lisp/ls-lisp.el (ls-lisp--sanitize-switches): support more
 time options

---
 lisp/ls-lisp.el | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lisp/ls-lisp.el b/lisp/ls-lisp.el
index d09b53b1cc..ae4a43797f 100644
--- a/lisp/ls-lisp.el
+++ b/lisp/ls-lisp.el
@@ -873,6 +873,7 @@ ls-lisp--sanitize-switches
   (let ((lsflags '(("-a" . "--all")
                    ("-A" . "--almost-all")
                    ("-B" . "--ignore-backups")
+                   ("-c" . "--time=ctime")
                    ("-C" . "--color")
                    ("-F" . "--classify")
                    ("-G" . "--no-group")
@@ -883,7 +884,9 @@ ls-lisp--sanitize-switches
                    ("-r" . "--reverse")
                    ("-R" . "--recursive")
                    ("-s" . "--size")
+                   ("-t" . "--sort=time")
                    ("-S" . "--sort.*[ \\\t]")
+                   ("-u" . "--time=atime")
                    (""   . "--group-directories-first")
                    (""   . "--author")
                    (""   . "--escape")
-- 
2.20.5

bug#70271: 30.0.50; [PATCH] * lisp/ls-lisp.el (ls-lisp--sanitize-switches): support more

[Eli Zaretskii]

> From: Lin Sun <sunlin7.mail@gmail.com>
> Date: Mon, 8 Apr 2024 05:10:22 +0000
> 
> The dired ignored the options like "--time=ctime" in the variable
> `dired-listing-switches' on Windows OS.
> 
>  I traced the code and found dired will use the functions in the
> `ls-lisp.el' instead of calling the real command "/usr/bin/ls", and
> the "ls-lisp.el" didn't process the "--time=ctime" option.
> 
> The attached PATCH will convert the "--time=ctime" to "-c"... in the
> "ls-lisp.el", and then the dired will display the desired time in its
> time column.
> 
> Here is the verification code (on Windows OS), try it one line and
> close the dired buffer then try next line.
> 
> (dired "/tmp/aa" "-al --sort=time") ; as "-al -t"
> 
> (dired "/tmp/aa" "-al --time=ctime") ; as "al -c"
> 
> (dired "/tmp/aa" "-al --time=atime") ; as "al -u"
> 
> Please help review the changes and approve them. Thanks

Thanks, but how about adding some tests for this?

bug#70271: 30.0.50; [PATCH] * lisp/ls-lisp.el (ls-lisp--sanitize-switches): support more

[Lin Sun]

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

On Mon, Apr 8, 2024 at 11:25 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Lin Sun <sunlin7.mail@gmail.com>
> > Date: Mon, 8 Apr 2024 05:10:22 +0000
> >
> > The dired ignored the options like "--time=ctime" in the variable
> > `dired-listing-switches' on Windows OS.
> >
> >  I traced the code and found dired will use the functions in the
> > `ls-lisp.el' instead of calling the real command "/usr/bin/ls", and
> > the "ls-lisp.el" didn't process the "--time=ctime" option.
> >
> > The attached PATCH will convert the "--time=ctime" to "-c"... in the
> > "ls-lisp.el", and then the dired will display the desired time in its
> > time column.
> >
> > Here is the verification code (on Windows OS), try it one line and
> > close the dired buffer then try next line.
> >
> > (dired "/tmp/aa" "-al --sort=time") ; as "-al -t"
> >
> > (dired "/tmp/aa" "-al --time=ctime") ; as "al -c"
> >
> > (dired "/tmp/aa" "-al --time=atime") ; as "al -u"
> >
> > Please help review the changes and approve them. Thanks
>
> Thanks, but how about adding some tests for this?

Sure, I had added the test case for this ticket.

Changes are attached in the PATCH.

Please help review again. Thanks.

[-- Attachment #2: 0001-Support-more-time-options-for-ls-lisp-sanitize-switc.patch --]
[-- Type: text/x-patch, Size: 3261 bytes --]

From 94410ce1d5ff93da4fb1df96abdb70c65141ff7d Mon Sep 17 00:00:00 2001
From: Lin Sun <sunlin7@hotmail.com>
Date: Mon, 8 Apr 2024 06:59:21 +0000
Subject: [PATCH] Support more time options for `ls-lisp--sanitize-switches'
 (bug#70271)

* lisp/ls-lisp.el (ls-lisp--sanitize-switches): more time options
* test/lisp/ls-lisp-tests.el: test case for bug#70271
---
 lisp/ls-lisp.el            |  3 +++
 test/lisp/ls-lisp-tests.el | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/lisp/ls-lisp.el b/lisp/ls-lisp.el
index d09b53b1cc..ae4a43797f 100644
--- a/lisp/ls-lisp.el
+++ b/lisp/ls-lisp.el
@@ -873,6 +873,7 @@ ls-lisp--sanitize-switches
   (let ((lsflags '(("-a" . "--all")
                    ("-A" . "--almost-all")
                    ("-B" . "--ignore-backups")
+                   ("-c" . "--time=ctime")
                    ("-C" . "--color")
                    ("-F" . "--classify")
                    ("-G" . "--no-group")
@@ -883,7 +884,9 @@ ls-lisp--sanitize-switches
                    ("-r" . "--reverse")
                    ("-R" . "--recursive")
                    ("-s" . "--size")
+                   ("-t" . "--sort=time")
                    ("-S" . "--sort.*[ \\\t]")
+                   ("-u" . "--time=atime")
                    (""   . "--group-directories-first")
                    (""   . "--author")
                    (""   . "--escape")
diff --git a/test/lisp/ls-lisp-tests.el b/test/lisp/ls-lisp-tests.el
index 77046871ea..81e3e163c9 100644
--- a/test/lisp/ls-lisp-tests.el
+++ b/test/lisp/ls-lisp-tests.el
@@ -29,6 +29,42 @@
 (require 'ls-lisp)
 (require 'dired)
 
+(ert-deftest ls-lisp-test-bug70271 ()
+  "Test for https://debbugs.gnu.org/70271 ."
+  (ert-with-temp-file
+   fpath
+   :suffix "bug70271"
+   (let* ((dir (file-name-directory fpath))
+          (attributes (file-attributes fpath))
+          (dired-find-subdir t)
+          ls-lisp-use-insert-directory-program buf ts)
+     (unwind-protect
+         (progn
+           (setq ts (file-attribute-access-time attributes))
+           (with-current-buffer
+               (dired-internal-noselect dir "-la --time=ctime")
+             (setq buf (current-buffer)
+                   str (format-time-string "%H:%M" ts))
+             (should (search-forward-regexp str nil t))
+             (kill-buffer))
+           (setq ts (- (float-time) 60))
+           (set-file-times fpath ts)
+           (with-current-buffer
+               (dired-internal-noselect dir "-la --sort=time")
+             (setq buf (current-buffer)
+                   str (format-time-string "%H:%M" ts))
+             (should (search-forward-regexp str nil t))
+             (kill-buffer))
+           (setq ts (- (float-time) 120))
+           (set-file-times fpath ts)
+           (with-current-buffer
+               (dired-internal-noselect dir "-la --time=atime")
+             (setq buf (current-buffer)
+                   str (format-time-string "%H:%M" ts))
+             (should (search-forward-regexp str nil t))
+             (kill-buffer)))
+       (when (buffer-live-p buf) (kill-buffer buf))))))
+
 (ert-deftest ls-lisp-test-bug27762 ()
   "Test for https://debbugs.gnu.org/27762 ."
   (let* ((dir source-directory)
-- 
2.20.5

bug#70271: 30.0.50; [PATCH] * lisp/ls-lisp.el (ls-lisp--sanitize-switches): support more

[Eli Zaretskii]

> From: Lin Sun <sunlin7.mail@gmail.com>
> Date: Mon, 8 Apr 2024 20:43:54 +0000
> Cc: 70271@debbugs.gnu.org
> 
> On Mon, Apr 8, 2024 at 11:25 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > From: Lin Sun <sunlin7.mail@gmail.com>
> > > Date: Mon, 8 Apr 2024 05:10:22 +0000
> > >
> > > The dired ignored the options like "--time=ctime" in the variable
> > > `dired-listing-switches' on Windows OS.
> > >
> > >  I traced the code and found dired will use the functions in the
> > > `ls-lisp.el' instead of calling the real command "/usr/bin/ls", and
> > > the "ls-lisp.el" didn't process the "--time=ctime" option.
> > >
> > > The attached PATCH will convert the "--time=ctime" to "-c"... in the
> > > "ls-lisp.el", and then the dired will display the desired time in its
> > > time column.
> > >
> > > Here is the verification code (on Windows OS), try it one line and
> > > close the dired buffer then try next line.
> > >
> > > (dired "/tmp/aa" "-al --sort=time") ; as "-al -t"
> > >
> > > (dired "/tmp/aa" "-al --time=ctime") ; as "al -c"
> > >
> > > (dired "/tmp/aa" "-al --time=atime") ; as "al -u"
> > >
> > > Please help review the changes and approve them. Thanks
> >
> > Thanks, but how about adding some tests for this?
> 
> Sure, I had added the test case for this ticket.
> 
> Changes are attached in the PATCH.

Thanks, installed on master, and closing the bug.

The test had a small bug, I fixed it.

Also, please look at how I modified the commit log message, and in the
future try following our conventions more closely (capitalization of
log entries, specifying function names, etc.)

bug#70271: 30.0.50; [PATCH] * lisp/ls-lisp.el (ls-lisp--sanitize-switches): support more

[Lin Sun]

On Tue, Apr 9, 2024 at 3:24 AM Eli Zaretskii <eliz@gnu.org> wrote:

> Thanks, installed on master, and closing the bug.
> The test had a small bug, I fixed it.
> Also, please look at how I modified the commit log message, and in the
> future try following our conventions more closely (capitalization of
> log entries, specifying function names, etc.)

Thank you so much, appreciate it! And will follow code/log rules in
future patches.