bug#53638: [PATCH] newsticker better path handling and data storage

bug#53638: [PATCH] newsticker better path handling and data storage

[mailj2--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


The way newsticker currently handles paths involves directly concating
"/", which only works for UNIX systems.

Additionally, since newsticker uses `prin1-to-string' for storing
groups and cache, it should be enforced that `print-level' and
`print-length' are set to nil for the operation just as a precaution (it
happened to me that I manually set these once, which resulted in the
newsticker cache being corrupted).

This patch should fix both of these issues.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: newsticker-patch.diff --]
[-- Type: text/x-patch; name=newsticker-patch.diff, Size: 5835 bytes --]

diff --git a/lisp/net/newst-backend.el b/lisp/net/newst-backend.el
index a62a7bd8b7..d4d2892da0 100644
--- a/lisp/net/newst-backend.el
+++ b/lisp/net/newst-backend.el
@@ -1697,11 +1697,13 @@ newsticker--update-process-ids
 ;; ======================================================================
 (defun newsticker--images-dir ()
   "Return directory where feed images are saved."
-  (concat newsticker-dir "/images/"))
+  (concat (file-name-as-directory newsticker-dir)
+          (file-name-as-directory "images")))
 
 (defun newsticker--icons-dir ()
   "Return directory where feed icons are saved."
-  (concat newsticker-dir "/icons/"))
+  (concat (file-name-as-directory newsticker-dir)
+          (file-name-as-directory "icons")))
 
 (defun newsticker--image-get (feed-name filename directory url)
   "Get image for FEED-NAME by returning FILENAME from DIRECTORY.
@@ -2114,7 +2116,8 @@ newsticker--cache-get-feed
 
 (defun newsticker--cache-dir ()
   "Return directory for saving cache data."
-  (concat newsticker-dir "/feeds"))
+  (concat (file-name-as-directory newsticker-dir)
+          (file-name-as-directory "feeds")))
 
 (defun newsticker--cache-save ()
   "Save cache data for all feeds."
@@ -2125,11 +2128,15 @@ newsticker--cache-save
 
 (defun newsticker--cache-save-feed (feed)
   "Save cache data for FEED."
-  (let ((dir (concat (newsticker--cache-dir) "/" (symbol-name (car feed)))))
+  (let ((dir (concat (newsticker--cache-dir)
+                     (symbol-name (car feed))))
+        (print-level nil)
+        (print-length nil))
     (unless (file-directory-p dir)
       (make-directory dir t))
     (let ((coding-system-for-write 'utf-8))
-      (with-temp-file (concat dir "/data")
+      (with-temp-file (concat (file-name-as-directory dir)
+                              "data")
         (insert ";; -*- coding: utf-8 -*-\n")
         (insert (prin1-to-string (cdr feed)))))))
 
@@ -2141,7 +2148,9 @@ newsticker--cache-read
 
 (defun newsticker--cache-read-feed (feed-name)
   "Read cache data for feed named FEED-NAME."
-  (let ((file-name (concat (newsticker--cache-dir) "/" feed-name "/data"))
+  (let ((file-name (concat (newsticker--cache-dir)
+                           feed-name
+                           "data"))
         (coding-system-for-read 'utf-8))
     (when (file-exists-p file-name)
       (with-temp-buffer
@@ -2334,14 +2343,17 @@ newsticker-download-images
   "Download the first image.
 If FEEDNAME equals \"imagefeed\" download the first image URL
 found in the description=contents of ITEM to the directory
-\"~/tmp/newsticker/FEEDNAME/TITLE\" where TITLE is the title of
+`temporary-file-directory'/newsticker/FEEDNAME/TITLE where TITLE is the title of
 the item."
   (when (string= feedname "imagefeed")
     (let ((title (newsticker--title item))
           (desc (newsticker--desc item)))
       (when (string-match "<img src=\"\\(http://[^ \"]+\\)\"" desc)
         (let ((url (substring desc (match-beginning 1) (match-end 1)))
-              (temp-dir (concat "~/tmp/newsticker/" feedname "/" title))
+              (temp-dir (concat (file-name-as-directory temporary-file-directory)
+                                (file-name-as-directory "newsticker")
+                                (file-name-as-directory feedname)
+                                title))
               (org-dir default-directory))
           (unless (file-directory-p temp-dir)
             (make-directory temp-dir t))
@@ -2355,7 +2367,8 @@ newsticker-download-images
 
 (defun newsticker-download-enclosures (feedname item)
   "In all feeds download the enclosed object of the news ITEM.
-The object is saved to the directory \"~/tmp/newsticker/FEEDNAME/TITLE\", which
+The object is saved to the directory
+`temporary-file-directory'/newsticker/FEEDNAME/TITLE, which
 is created if it does not exist.  TITLE is the title of the news
 item.  Argument FEEDNAME is ignored.
 This function is suited for adding it to `newsticker-new-item-functions'."
@@ -2363,7 +2376,10 @@ newsticker-download-enclosures
         (enclosure (newsticker--enclosure item)))
     (when enclosure
       (let ((url (cdr (assoc 'url enclosure)))
-            (temp-dir (concat "~/tmp/newsticker/" feedname "/" title))
+            (temp-dir (concat (file-name-as-directory temporary-file-directory)
+                              (file-name-as-directory "newsticker")
+                              (file-name-as-directory feedname)
+                              title))
             (org-dir default-directory))
         (unless (file-directory-p temp-dir)
           (make-directory temp-dir t))
diff --git a/lisp/net/newst-treeview.el b/lisp/net/newst-treeview.el
index 80d9fd1cef..f7cf153900 100644
--- a/lisp/net/newst-treeview.el
+++ b/lisp/net/newst-treeview.el
@@ -1257,7 +1257,10 @@ newsticker-treeview-save
   "Save treeview group settings."
   (interactive)
   (let ((coding-system-for-write 'utf-8)
-        (buf (find-file-noselect (concat newsticker-dir "/groups"))))
+        (buf (find-file-noselect (concat (file-name-as-directory newsticker-dir)
+                                         "groups")))
+        (print-length nil)
+        (print-level nil))
     (when buf
       (with-current-buffer buf
         (setq buffer-undo-list t)
@@ -1270,7 +1273,8 @@ newsticker-treeview-save
 (defun newsticker--treeview-load ()
   "Load treeview settings."
   (let* ((coding-system-for-read 'utf-8)
-         (filename (concat newsticker-dir "/groups"))
+         (filename (concat (file-name-as-directory newsticker-dir)
+                           "groups"))
          (buf (and (file-exists-p filename)
                    (find-file-noselect filename))))
     (when buf

bug#53638: [PATCH] newsticker better path handling and data storage

[Robert Pluim]

>>>>> On Sun, 30 Jan 2022 19:47:03 +0000, mailj2--- via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

    mailj2> The way newsticker currently handles paths involves directly concating
    mailj2> "/", which only works for UNIX systems.

emacs-28 has `file-name-concat' for this very purpose, perhaps you
could respin to use that instead.

    mailj2> Additionally, since newsticker uses `prin1-to-string' for storing
    mailj2> groups and cache, it should be enforced that `print-level' and
    mailj2> `print-length' are set to nil for the operation just as a precaution (it
    mailj2> happened to me that I manually set these once, which resulted in the
    mailj2> newsticker cache being corrupted).

OK. It would be good to have a ChangeLog style commit message as well,
see CONTRIBUTE for details. 'C-x 4 a' can help with writing it.

Robert
-- 

bug#53638: [PATCH] newsticker better path handling and data storage

[Eli Zaretskii]

> Date: Sun, 30 Jan 2022 19:47:03 +0000
> From: mailj2--- via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> The way newsticker currently handles paths involves directly concating
> "/", which only works for UNIX systems.

The canonical way of doing that is to use expand-file-name.

bug#53638: [PATCH] newsticker better path handling and data storage

[Lars Ingebrigtsen]

mailj2@protonmail.com writes:

> The way newsticker currently handles paths involves directly concating
> "/", which only works for UNIX systems.

Can you rework the patch to use `expand-file-name', which is how
file names are supposed to be constructed?

> Additionally, since newsticker uses `prin1-to-string' for storing
> groups and cache, it should be enforced that `print-level' and
> `print-length' are set to nil for the operation just as a precaution (it
> happened to me that I manually set these once, which resulted in the
> newsticker cache being corrupted).

That part of the patch looks good.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53638: [PATCH] newsticker better path handling and data storage

[mailj2--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

"Lars Ingebrigtsen" <larsi@gnus.org> writes:

> Can you rework the patch to use
> `expand-file-name', which is how file names are supposed to be constructed?

I've refactored it to use `expand-file-name' and `file-name-concat'. It
appears the current convention (correct me if I'm wrong) is to use
`expand-file-name' for joining only one path and one directory, and
`file-name-concat' for multiple together.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: newsticker-patch-2.diff --]
[-- Type: text/x-patch; name=newsticker-patch-2.diff, Size: 5941 bytes --]

diff --git a/lisp/net/newst-backend.el b/lisp/net/newst-backend.el
index a62a7bd8b7..d3610a2130 100644
--- a/lisp/net/newst-backend.el
+++ b/lisp/net/newst-backend.el
@@ -1697,11 +1697,13 @@ newsticker--update-process-ids
 ;; ======================================================================
 (defun newsticker--images-dir ()
   "Return directory where feed images are saved."
-  (concat newsticker-dir "/images/"))
+  (file-name-as-directory
+    (expand-file-name "images" newsticker-dir)))
 
 (defun newsticker--icons-dir ()
   "Return directory where feed icons are saved."
-  (concat newsticker-dir "/icons/"))
+  (file-name-as-directory
+    (expand-file-name "icons" newsticker-dir)))
 
 (defun newsticker--image-get (feed-name filename directory url)
   "Get image for FEED-NAME by returning FILENAME from DIRECTORY.
@@ -2114,7 +2116,8 @@ newsticker--cache-get-feed
 
 (defun newsticker--cache-dir ()
   "Return directory for saving cache data."
-  (concat newsticker-dir "/feeds"))
+  (file-name-as-directory
+    (expand-file-name "feeds" newsticker-dir)))
 
 (defun newsticker--cache-save ()
   "Save cache data for all feeds."
@@ -2125,11 +2128,15 @@ newsticker--cache-save
 
 (defun newsticker--cache-save-feed (feed)
   "Save cache data for FEED."
-  (let ((dir (concat (newsticker--cache-dir) "/" (symbol-name (car feed)))))
+  (let ((dir (file-name-as-directory
+               (expand-file-name (symbol-name (car feed))
+                                 (newsticker--cache-dir))))
+        (print-level nil)
+        (print-length nil))
     (unless (file-directory-p dir)
       (make-directory dir t))
     (let ((coding-system-for-write 'utf-8))
-      (with-temp-file (concat dir "/data")
+      (with-temp-file (expand-file-name "data" dir)
         (insert ";; -*- coding: utf-8 -*-\n")
         (insert (prin1-to-string (cdr feed)))))))
 
@@ -2141,7 +2148,9 @@ newsticker--cache-read
 
 (defun newsticker--cache-read-feed (feed-name)
   "Read cache data for feed named FEED-NAME."
-  (let ((file-name (concat (newsticker--cache-dir) "/" feed-name "/data"))
+  (let ((file-name (file-name-concat (newsticker--cache-dir)
+                                     feed-name
+                                     "data"))
         (coding-system-for-read 'utf-8))
     (when (file-exists-p file-name)
       (with-temp-buffer
@@ -2334,14 +2343,18 @@ newsticker-download-images
   "Download the first image.
 If FEEDNAME equals \"imagefeed\" download the first image URL
 found in the description=contents of ITEM to the directory
-\"~/tmp/newsticker/FEEDNAME/TITLE\" where TITLE is the title of
+`temporary-file-directory'/newsticker/FEEDNAME/TITLE where TITLE is the title of
 the item."
   (when (string= feedname "imagefeed")
     (let ((title (newsticker--title item))
           (desc (newsticker--desc item)))
       (when (string-match "<img src=\"\\(http://[^ \"]+\\)\"" desc)
         (let ((url (substring desc (match-beginning 1) (match-end 1)))
-              (temp-dir (concat "~/tmp/newsticker/" feedname "/" title))
+               (temp-dir (file-name-as-directory
+                           (file-name-concat (file-name-as-directory temporary-file-directory)
+                                             (file-name-as-directory "newsticker")
+                                             (file-name-as-directory feedname)
+                                             title)))
               (org-dir default-directory))
           (unless (file-directory-p temp-dir)
             (make-directory temp-dir t))
@@ -2355,7 +2368,8 @@ newsticker-download-images
 
 (defun newsticker-download-enclosures (feedname item)
   "In all feeds download the enclosed object of the news ITEM.
-The object is saved to the directory \"~/tmp/newsticker/FEEDNAME/TITLE\", which
+The object is saved to the directory
+`temporary-file-directory'/newsticker/FEEDNAME/TITLE, which
 is created if it does not exist.  TITLE is the title of the news
 item.  Argument FEEDNAME is ignored.
 This function is suited for adding it to `newsticker-new-item-functions'."
@@ -2363,7 +2377,11 @@ newsticker-download-enclosures
         (enclosure (newsticker--enclosure item)))
     (when enclosure
       (let ((url (cdr (assoc 'url enclosure)))
-            (temp-dir (concat "~/tmp/newsticker/" feedname "/" title))
+            (temp-dir (file-name-as-directory
+                        (file-name-concat temporary-file-directory
+                                          "newsticker"
+                                          feedname
+                                          title)))
             (org-dir default-directory))
         (unless (file-directory-p temp-dir)
           (make-directory temp-dir t))
diff --git a/lisp/net/newst-treeview.el b/lisp/net/newst-treeview.el
index 80d9fd1cef..f90a03873f 100644
--- a/lisp/net/newst-treeview.el
+++ b/lisp/net/newst-treeview.el
@@ -1257,7 +1257,10 @@ newsticker-treeview-save
   "Save treeview group settings."
   (interactive)
   (let ((coding-system-for-write 'utf-8)
-        (buf (find-file-noselect (concat newsticker-dir "/groups"))))
+        (buf (find-file-noselect (expand-file-name "groups"
+                                                    newsticker-dir)))
+        (print-length nil)
+        (print-level nil))
     (when buf
       (with-current-buffer buf
         (setq buffer-undo-list t)
@@ -1270,7 +1273,8 @@ newsticker-treeview-save
 (defun newsticker--treeview-load ()
   "Load treeview settings."
   (let* ((coding-system-for-read 'utf-8)
-         (filename (concat newsticker-dir "/groups"))
+         (filename (concat (file-name-as-directory newsticker-dir)
+                           "groups"))
          (buf (and (file-exists-p filename)
                    (find-file-noselect filename))))
     (when buf

bug#53638: [PATCH] newsticker better path handling and data storage

[Eli Zaretskii]

> Cc: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 31 Jan 2022 20:09:58 +0000
> From: mailj2--- via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> "Lars Ingebrigtsen" <larsi@gnus.org> writes:
> 
> > Can you rework the patch to use
> > `expand-file-name', which is how file names are supposed to be constructed?
> 
> I've refactored it to use `expand-file-name' and `file-name-concat'. It
> appears the current convention (correct me if I'm wrong) is to use
> `expand-file-name' for joining only one path and one directory, and
> `file-name-concat' for multiple together.

No, you can chain calls to expand-file-name as you need:

  (expand-file-name "foo" (expand-file-name "bar" my-directory))

bug#53638: [PATCH] newsticker better path handling and data

[Xavier Capaldi]

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

Not sure if this bug report/patch is blocked but I faced the
`prin1-to-string` issue which is fixed by the patch here. I've attached
an updated patch that uses chained calls to `expand-file-name` instead
of `file-name-concat` since that was the last issue mentioned with the
patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: newsticker-patch-3.diff --]
[-- Type: text/x-patch, Size: 5474 bytes --]

diff --git a/lisp/net/newst-backend.el b/lisp/net/newst-backend.el
index a62a7bd8b7..d3610a2130 100644
--- a/lisp/net/newst-backend.el
+++ b/lisp/net/newst-backend.el
@@ -1697,11 +1697,13 @@ newsticker--update-process-ids
 ;; ======================================================================
 (defun newsticker--images-dir ()
   "Return directory where feed images are saved."
-  (concat newsticker-dir "/images/"))
+  (file-name-as-directory
+    (expand-file-name "images" newsticker-dir)))
 
 (defun newsticker--icons-dir ()
   "Return directory where feed icons are saved."
-  (concat newsticker-dir "/icons/"))
+  (file-name-as-directory
+    (expand-file-name "icons" newsticker-dir)))
 
 (defun newsticker--image-get (feed-name filename directory url)
   "Get image for FEED-NAME by returning FILENAME from DIRECTORY.
@@ -2114,7 +2116,8 @@ newsticker--cache-get-feed
 
 (defun newsticker--cache-dir ()
   "Return directory for saving cache data."
-  (concat newsticker-dir "/feeds"))
+  (file-name-as-directory
+    (expand-file-name "feeds" newsticker-dir)))
 
 (defun newsticker--cache-save ()
   "Save cache data for all feeds."
@@ -2125,11 +2128,15 @@ newsticker--cache-save
 
 (defun newsticker--cache-save-feed (feed)
   "Save cache data for FEED."
-  (let ((dir (concat (newsticker--cache-dir) "/" (symbol-name (car feed)))))
+  (let ((dir (file-name-as-directory
+               (expand-file-name (symbol-name (car feed))
+                                 (newsticker--cache-dir))))
+        (print-level nil)
+        (print-length nil))
     (unless (file-directory-p dir)
       (make-directory dir t))
     (let ((coding-system-for-write 'utf-8))
-      (with-temp-file (concat dir "/data")
+      (with-temp-file (expand-file-name "data" dir)
         (insert ";; -*- coding: utf-8 -*-\n")
         (insert (prin1-to-string (cdr feed)))))))
 
@@ -2141,7 +2148,7 @@ newsticker--cache-read
 
 (defun newsticker--cache-read-feed (feed-name)
   "Read cache data for feed named FEED-NAME."
-  (let ((file-name (concat (newsticker--cache-dir) "/" feed-name "/data"))
+  (let ((file-name (expand-file-name "data" (expand-file-name feed-name (newsticker--cache-dir)))
         (coding-system-for-read 'utf-8))
     (when (file-exists-p file-name)
       (with-temp-buffer
@@ -2334,14 +2343,15 @@ newsticker-download-images
   "Download the first image.
 If FEEDNAME equals \"imagefeed\" download the first image URL
 found in the description=contents of ITEM to the directory
-\"~/tmp/newsticker/FEEDNAME/TITLE\" where TITLE is the title of
+`temporary-file-directory'/newsticker/FEEDNAME/TITLE where TITLE is the title of
 the item."
   (when (string= feedname "imagefeed")
     (let ((title (newsticker--title item))
           (desc (newsticker--desc item)))
       (when (string-match "<img src=\"\\(http://[^ \"]+\\)\"" desc)
         (let ((url (substring desc (match-beginning 1) (match-end 1)))
-              (temp-dir (concat "~/tmp/newsticker/" feedname "/" title))
+		(temp-dir (file-name-as-directory
+                           (expand-file-name title (expand-file-name feedname (expand-file-name "newsticker" temporary-file-directory)))))
               (org-dir default-directory))
           (unless (file-directory-p temp-dir)
             (make-directory temp-dir t))
@@ -2355,7 +2368,8 @@ newsticker-download-images
 
 (defun newsticker-download-enclosures (feedname item)
   "In all feeds download the enclosed object of the news ITEM.
-The object is saved to the directory \"~/tmp/newsticker/FEEDNAME/TITLE\", which
+The object is saved to the directory
+`temporary-file-directory'/newsticker/FEEDNAME/TITLE, which
 is created if it does not exist.  TITLE is the title of the news
 item.  Argument FEEDNAME is ignored.
 This function is suited for adding it to `newsticker-new-item-functions'."
@@ -2363,7 +2377,8 @@ newsticker-download-enclosures
         (enclosure (newsticker--enclosure item)))
     (when enclosure
       (let ((url (cdr (assoc 'url enclosure)))
-            (temp-dir (concat "~/tmp/newsticker/" feedname "/" title))
+            (temp-dir (file-name-as-directory
+                        (expand-file-name title (expand-file-name feedname (expand-file-name "newsticker" temporary-file-directory)))))
             (org-dir default-directory))
         (unless (file-directory-p temp-dir)
           (make-directory temp-dir t))
diff --git a/lisp/net/newst-treeview.el b/lisp/net/newst-treeview.el
index 80d9fd1cef..f90a03873f 100644
--- a/lisp/net/newst-treeview.el
+++ b/lisp/net/newst-treeview.el
@@ -1257,7 +1257,10 @@ newsticker-treeview-save
   "Save treeview group settings."
   (interactive)
   (let ((coding-system-for-write 'utf-8)
-        (buf (find-file-noselect (concat newsticker-dir "/groups"))))
+        (buf (find-file-noselect (expand-file-name "groups"
+                                                    newsticker-dir)))
+        (print-length nil)
+        (print-level nil))
     (when buf
       (with-current-buffer buf
         (setq buffer-undo-list t)
@@ -1270,7 +1273,8 @@ newsticker-treeview-save
 (defun newsticker--treeview-load ()
   "Load treeview settings."
   (let* ((coding-system-for-read 'utf-8)
-         (filename (concat newsticker-dir "/groups"))
+         (filename (concat (file-name-as-directory newsticker-dir)
+                           "groups"))
          (buf (and (file-exists-p filename)
                    (find-file-noselect filename))))
     (when buf

bug#53638: [PATCH] newsticker better path handling and data

[Lars Ingebrigtsen]

Xavier Capaldi <xcapaldi@scribo.biz> writes:

> Not sure if this bug report/patch is blocked but I faced the
> `prin1-to-string` issue which is fixed by the patch here. I've attached
> an updated patch that uses chained calls to `expand-file-name` instead
> of `file-name-concat` since that was the last issue mentioned with the
> patch.

Thanks.  I've now applied the patch (with a number of changes) and
pushed to Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no