* bug#69517: [PATCH] Make gnus cache work with group names having '/' @ 2024-03-03 1:52 James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-09 8:42 ` Eli Zaretskii 0 siblings, 1 reply; 12+ messages in thread From: James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-03 1:52 UTC (permalink / raw) To: 69517 [-- Attachment #1: Type: text/plain, Size: 1110 bytes --] Tags: patch Reproduction steps: - Setup Gnus with any group name having a slash ('/') such as "[Gmail]/Drafts" or an Atom feed (they usually have slashes) using the patch in bug#66188. - Press '*' on a message in the group. - Do (info "(gnus) Creating a Virtual Server") - Open the above from the Server buffer; RET on the new group fails. A patch is attached. I couldn't find the problematic commit or its original branch (where it was a consolidated merge from) but 'gnus-use-long-file-names' is apparently not meant for backends: it can't even be customized with that 'not-cache' option. I think this is the right way to solve it: the other lines removed in this patch are even older, but they were never being called due to the above reason. In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.33, cairo version 1.16.0) of 2024-02-20 built on user-Inspiron-15-5518 Repository revision: 466800591c2bd674b0b967205147d7519da7e1a4 Repository branch: nnatom-master Windowing system distributor 'The X.Org Foundation', version 11.0.12101004 System Description: Ubuntu 22.04.3 LTS [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Make-gnus-cache-work-with-group-names-having.patch --] [-- Type: text/patch, Size: 1521 bytes --] From e0b4a71bf241d6faaa46bea58a4688f1e9c932b8 Mon Sep 17 00:00:00 2001 From: James Thomas <jimjoe@gmx.net> Date: Sun, 3 Mar 2024 06:58:49 +0530 Subject: [PATCH] Make gnus cache work with group names having '/' Replace the incorrect `gnus-use-long-file-name` with `nnmail-use-long-file-names`. * lisp/gnus/gnus-cache.el (gnus-cache-file-name): --- lisp/gnus/gnus-cache.el | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/lisp/gnus/gnus-cache.el b/lisp/gnus/gnus-cache.el index 961219eee8f..d174ee9a9f4 100644 --- a/lisp/gnus/gnus-cache.el +++ b/lisp/gnus/gnus-cache.el @@ -448,16 +448,12 @@ gnus-cache-file-name (file-name-as-directory (expand-file-name (nnheader-translate-file-chars - (if (gnus-use-long-file-name 'not-cache) - group - (let ((group (nnheader-replace-duplicate-chars-in-string - (nnheader-replace-chars-in-string group ?/ ?_) - ?. ?_))) - ;; Translate the first colon into a slash. - (when (string-match ":" group) - (setq group (concat (substring group 0 (match-beginning 0)) - "/" (substring group (match-end 0))))) - (nnheader-replace-chars-in-string group ?. ?/))) + (let ((group (nnheader-replace-duplicate-chars-in-string + (nnheader-replace-chars-in-string group ?/ ?_) + ?. ?_))) + (if (not nnmail-use-long-file-names) + (nnheader-replace-chars-in-string group ?. ?/) + group)) t) gnus-cache-directory)))) -- 2.34.1 [-- Attachment #3: Type: text/plain, Size: 4 bytes --] -- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-03 1:52 bug#69517: [PATCH] Make gnus cache work with group names having '/' James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-09 8:42 ` Eli Zaretskii 2024-03-09 21:56 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 12+ messages in thread From: Eli Zaretskii @ 2024-03-09 8:42 UTC (permalink / raw) To: James Thomas, Eric Abrahamsen; +Cc: 69517 > Date: Sun, 03 Mar 2024 07:22:29 +0530 > From: James Thomas via "Bug reports for GNU Emacs, > the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> > > Tags: patch > > Reproduction steps: > > - Setup Gnus with any group name having a slash ('/') such as > "[Gmail]/Drafts" or an Atom feed (they usually have slashes) using the > patch in bug#66188. > > - Press '*' on a message in the group. > > - Do (info "(gnus) Creating a Virtual Server") > > - Open the above from the Server buffer; RET on the new group fails. > > A patch is attached. I couldn't find the problematic commit or its > original branch (where it was a consolidated merge from) but > 'gnus-use-long-file-names' is apparently not meant for backends: it > can't even be customized with that 'not-cache' option. I think this is > the right way to solve it: the other lines removed in this patch are > even older, but they were never being called due to the above reason. Eric, could you please review the patch and install if it's okay? Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-09 8:42 ` Eli Zaretskii @ 2024-03-09 21:56 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-10 5:27 ` Eric Abrahamsen 0 siblings, 1 reply; 12+ messages in thread From: James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-09 21:56 UTC (permalink / raw) To: 69517; +Cc: Eric Abrahamsen, Eli Zaretskii Eli Zaretskii wrote: >> Date: Sun, 03 Mar 2024 07:22:29 +0530 >> From: James Thomas via "Bug reports for GNU Emacs, >> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> >> >> Tags: patch >> >> Reproduction steps: >> >> - Setup Gnus with any group name having a slash ('/') such as >> "[Gmail]/Drafts" or an Atom feed (they usually have slashes) using the >> patch in bug#66188. >> >> - Press '*' on a message in the group. >> >> - Do (info "(gnus) Creating a Virtual Server") >> >> - Open the above from the Server buffer; RET on the new group fails. >> >> A patch is attached. I couldn't find the problematic commit or its >> original branch (where it was a consolidated merge from) but >> 'gnus-use-long-file-names' is apparently not meant for backends: it >> can't even be customized with that 'not-cache' option. I think this is >> the right way to solve it: the other lines removed in this patch are >> even older, but they were never being called due to the above reason. > > Eric, could you please review the patch and install if it's okay? > > Thanks. There's a small caveat after applying this: Before this patch, cache entries would've effectively ignored 'nnmail-use-long-file-names' (nil by default) and used long names. After this patch new entries would honor it, resulting in an extra directory tree for the same group. But only the original one would be opened (due to [[./lisp/gnus/nnmail.el::;; If this direc]]). To fix it, one would have to copy all the files in from the original directory into the new one (and retain the active file entry). Or of course, change the above variable (but that could have other implications depending upon one's configuration). -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-09 21:56 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-10 5:27 ` Eric Abrahamsen 2024-03-10 18:33 ` Eric Abrahamsen 2024-03-10 21:32 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 2 replies; 12+ messages in thread From: Eric Abrahamsen @ 2024-03-10 5:27 UTC (permalink / raw) To: James Thomas; +Cc: Eli Zaretskii, 69517 James Thomas <jimjoe@gmx.net> writes: > Eli Zaretskii wrote: > >>> Date: Sun, 03 Mar 2024 07:22:29 +0530 >>> From: James Thomas via "Bug reports for GNU Emacs, >>> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> >>> >>> Tags: patch >>> >>> Reproduction steps: >>> >>> - Setup Gnus with any group name having a slash ('/') such as >>> "[Gmail]/Drafts" or an Atom feed (they usually have slashes) using the >>> patch in bug#66188. >>> >>> - Press '*' on a message in the group. >>> >>> - Do (info "(gnus) Creating a Virtual Server") >>> >>> - Open the above from the Server buffer; RET on the new group fails. >>> >>> A patch is attached. I couldn't find the problematic commit or its >>> original branch (where it was a consolidated merge from) but >>> 'gnus-use-long-file-names' is apparently not meant for backends: it >>> can't even be customized with that 'not-cache' option. I think this is >>> the right way to solve it: the other lines removed in this patch are >>> even older, but they were never being called due to the above reason. >> >> Eric, could you please review the patch and install if it's okay? >> >> Thanks. > > There's a small caveat after applying this: > > Before this patch, cache entries would've effectively ignored > 'nnmail-use-long-file-names' (nil by default) and used long names. After > this patch new entries would honor it, resulting in an extra directory > tree for the same group. But only the original one would be opened (due > to [[./lisp/gnus/nnmail.el::;; If this direc]]). To fix it, one would > have to copy all the files in from the original directory into the new > one (and retain the active file entry). Or of course, change the above > variable (but that could have other implications depending upon one's > configuration). Thanks for reporting this! Obviously something is wrong here, but I get different results from your reproduction recipe (I'm running master): when I create the cache nnml server and hit RET on it, I get: K 1: nnml:left.right K 1: nnml:left/right I am able to subscribe to both groups. When I return to the *Group* buffer, I can enter the version of the group with the ".", and read the cached article, while attempting to enter the version with the slash gives me "Invalid group (no such directory)". On disk, the "News/cache/active" file starts out with the version with the ".", and after I create the extra cache server it contains both, it looks like: "nnml:left.right" 1 1 y "nnml:left/right" 1 1 y Meanwhile, the directory structure "News/cache/nnml:left/right/1" is created. The "." in the active file comes from `gnus-cache-generate-active`, and that mechanism seems to be working fine, at least in my case -- do you not see that version of the group? To be honest I have no idea what's happening in `gnus-cache-file-name'. I'd be hesitant to remove all that code without understanding it better And I don't think we can argue that that code is "dead" just because it is behind an option value that isn't one of the customization options. Anyway, it would be good to first know why you're not seeing the "." version of the offending filename, and then I guess try to work out what's going on with the file names. It kind of looks like a collision between two different mechanisms -- one that translates "/" to "_" to prevent unwanted filesystem hierarchies, and one that translates between "/" and "." in order to convert newsgroup-name dot hierarchies into filesystem hierarchies. In this case, it seems like "long names" and newsgroup hierarchies shouldn't be applicable at all. Eric ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-10 5:27 ` Eric Abrahamsen @ 2024-03-10 18:33 ` Eric Abrahamsen 2024-03-14 3:26 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-10 21:32 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 1 reply; 12+ messages in thread From: Eric Abrahamsen @ 2024-03-10 18:33 UTC (permalink / raw) To: James Thomas; +Cc: Eli Zaretskii, 69517 Eric Abrahamsen <eric@ericabrahamsen.net> writes: > James Thomas <jimjoe@gmx.net> writes: > >> Eli Zaretskii wrote: >> >>>> Date: Sun, 03 Mar 2024 07:22:29 +0530 >>>> From: James Thomas via "Bug reports for GNU Emacs, >>>> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> >>>> >>>> Tags: patch >>>> >>>> Reproduction steps: >>>> >>>> - Setup Gnus with any group name having a slash ('/') such as >>>> "[Gmail]/Drafts" or an Atom feed (they usually have slashes) using the >>>> patch in bug#66188. >>>> >>>> - Press '*' on a message in the group. >>>> >>>> - Do (info "(gnus) Creating a Virtual Server") >>>> >>>> - Open the above from the Server buffer; RET on the new group fails. >>>> >>>> A patch is attached. I couldn't find the problematic commit or its >>>> original branch (where it was a consolidated merge from) but >>>> 'gnus-use-long-file-names' is apparently not meant for backends: it >>>> can't even be customized with that 'not-cache' option. I think this is >>>> the right way to solve it: the other lines removed in this patch are >>>> even older, but they were never being called due to the above reason. >>> >>> Eric, could you please review the patch and install if it's okay? >>> >>> Thanks. >> >> There's a small caveat after applying this: >> >> Before this patch, cache entries would've effectively ignored >> 'nnmail-use-long-file-names' (nil by default) and used long names. After >> this patch new entries would honor it, resulting in an extra directory >> tree for the same group. But only the original one would be opened (due >> to [[./lisp/gnus/nnmail.el::;; If this direc]]). To fix it, one would >> have to copy all the files in from the original directory into the new >> one (and retain the active file entry). Or of course, change the above >> variable (but that could have other implications depending upon one's >> configuration). > > Thanks for reporting this! Obviously something is wrong here, but I get > different results from your reproduction recipe (I'm running master): > when I create the cache nnml server and hit RET on it, I get: > > K 1: nnml:left.right > K 1: nnml:left/right > > I am able to subscribe to both groups. When I return to the *Group* > buffer, I can enter the version of the group with the ".", and read the > cached article, while attempting to enter the version with the slash > gives me "Invalid group (no such directory)". > > On disk, the "News/cache/active" file starts out with the version with > the ".", and after I create the extra cache server it contains both, it > looks like: > > "nnml:left.right" 1 1 y > "nnml:left/right" 1 1 y > > Meanwhile, the directory structure "News/cache/nnml:left/right/1" is > created. > > The "." in the active file comes from `gnus-cache-generate-active`, and > that mechanism seems to be working fine, at least in my case -- do you > not see that version of the group? > > To be honest I have no idea what's happening in `gnus-cache-file-name'. > I'd be hesitant to remove all that code without understanding it better > And I don't think we can argue that that code is "dead" just because it > is behind an option value that isn't one of the customization options. > > Anyway, it would be good to first know why you're not seeing the "." > version of the offending filename, and then I guess try to work out > what's going on with the file names. It kind of looks like a collision > between two different mechanisms -- one that translates "/" to "_" to > prevent unwanted filesystem hierarchies, and one that translates between > "/" and "." in order to convert newsgroup-name dot hierarchies into > filesystem hierarchies. In this case, it seems like "long names" and > newsgroup hierarchies shouldn't be applicable at all. I was briefly optimistic about the possibility that your previous patch, on bug#65467, might help address this issue. That patch did make it possible for nnml groups with slashes in the name to work correctly (they didn't, previously), but it still doesn't address this particular scenario. Meanwhile, I realize that my hesitation on that bug report is bogus, as everywhere else we already have active files with quotes in them, and `gnus-active-to-gnus-format' does a fair amount of work to handle various quoting situations. The real problem (well, one of the real problems) is that we should just have two central routines for reading and writing active files, so that there are only two places to keep in sync. Instead we have those two places, and then a smattering of other functions in other places that do something similar, and also have to be kept in sync, and I haven't done that. At the very least I'll need to apply your patch from 65467. The other real problem is that gnus-cache is confused about whether it wants to be a select method, or a modified version of article saving. The presence of `gnus-use-long-file-name' indicates the latter, but the manual's instructions about the nnml select method indicates the former. I think it should be a select method, which means that the group directory should be created in "News/cache" the same way it is created at the top level: with the "/" replaced by "_", and everything else using the proper "left/right" group name. Perhaps "long file names" can still play a role, but if so that should be via `nnmail-use-long-file-names', and gnus-cache in general should behave like a nnmail backend. Eric ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-10 18:33 ` Eric Abrahamsen @ 2024-03-14 3:26 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-15 17:33 ` Daniel Semyonov via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 12+ messages in thread From: James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-14 3:26 UTC (permalink / raw) To: 69517; +Cc: Eric Abrahamsen, Eli Zaretskii [-- Attachment #1: Type: text/plain, Size: 1253 bytes --] Eric Abrahamsen wrote: > The real problem (well, one of the real problems) is that we should just > have two central routines for reading and writing active files, so that > there are only two places to keep in sync. Instead we have those two > places, and then a smattering of other functions in other places that do > something similar, and also have to be kept in sync, and I haven't done > that. At the very least I'll need to apply your patch from 65467. > > The other real problem is that gnus-cache is confused about whether it > wants to be a select method, or a modified version of article saving. > The presence of `gnus-use-long-file-name' indicates the latter, but the > manual's instructions about the nnml select method indicates the former. > > I think it should be a select method, which means that the group > directory should be created in "News/cache" the same way it is created > at the top level: with the "/" replaced by "_", and everything else > using the proper "left/right" group name. Perhaps "long file names" can > still play a role, but if so that should be via > `nnmail-use-long-file-names', and gnus-cache in general should behave > like a nnmail backend. Well, I have sort of, an approach based on my earlier patch: [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Make-gnus-cache-work-with-group-names-having.patch --] [-- Type: text/x-diff, Size: 3116 bytes --] From 0f5d693dc439b11294b3bde3dc9129bdf45a2179 Mon Sep 17 00:00:00 2001 From: James Thomas <james.thomas@ahimsa.global> Date: Thu, 14 Mar 2024 08:42:00 +0530 Subject: [PATCH] Make gnus cache work with group names having '/' Make `gnus-cache-file-name` use the existing `nnmail-group-pathname`. * lisp/gnus/gnus-cache.el (gnus-cache-file-name) (gnus-cache-update-article): * lisp/gnus/nnmail.el (nnmail-group-pathname): --- lisp/gnus/gnus-cache.el | 27 +++++++-------------------- lisp/gnus/nnmail.el | 3 ++- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/lisp/gnus/gnus-cache.el b/lisp/gnus/gnus-cache.el index 961219eee8f..7af02368d36 100644 --- a/lisp/gnus/gnus-cache.el +++ b/lisp/gnus/gnus-cache.el @@ -443,23 +443,9 @@ gnus-cache-member-of-class (and (not unread) (not ticked) (not dormant) (memq 'read class)))) (defun gnus-cache-file-name (group article) - (expand-file-name - (if (stringp article) article (int-to-string article)) - (file-name-as-directory - (expand-file-name - (nnheader-translate-file-chars - (if (gnus-use-long-file-name 'not-cache) - group - (let ((group (nnheader-replace-duplicate-chars-in-string - (nnheader-replace-chars-in-string group ?/ ?_) - ?. ?_))) - ;; Translate the first colon into a slash. - (when (string-match ":" group) - (setq group (concat (substring group 0 (match-beginning 0)) - "/" (substring group (match-end 0))))) - (nnheader-replace-chars-in-string group ?. ?/))) - t) - gnus-cache-directory)))) + (nnmail-group-pathname + group gnus-cache-directory + (if (stringp article) article (int-to-string article)))) (defun gnus-cache-update-article (group article) "If ARTICLE is in the cache, remove it and re-enter it." @@ -699,9 +685,10 @@ gnus-cache-generate-active (file-name-as-directory (expand-file-name gnus-cache-directory)))) (directory-file-name directory)) - (nnheader-replace-chars-in-string - (substring (directory-file-name directory) (match-end 0)) - ?/ ?.))) + (url-unhex-string + (nnheader-replace-chars-in-string + (substring (directory-file-name directory) (match-end 0)) + ?/ ?.)))) nums alphs) (when top (gnus-message 5 "Generating the cache active file...") diff --git a/lisp/gnus/nnmail.el b/lisp/gnus/nnmail.el index fef12eebe09..a9f5b89c6fe 100644 --- a/lisp/gnus/nnmail.el +++ b/lisp/gnus/nnmail.el @@ -33,6 +33,7 @@ (require 'mail-source) (require 'mm-util) (require 'gnus-int) +(require 'browse-url) (autoload 'mail-send-and-exit "sendmail" nil t) @@ -627,7 +628,7 @@ nnmail-group-pathname (concat (let ((dir (file-name-as-directory (expand-file-name dir)))) (setq group (nnheader-replace-duplicate-chars-in-string - (nnheader-replace-chars-in-string group ?/ ?_) + (browse-url-url-encode-chars group "[/%]") ?. ?_)) (setq group (nnheader-translate-file-chars group)) ;; If this directory exists, we use it directly. -- 2.34.1 [-- Attachment #3: Type: text/plain, Size: 749 bytes --] James Thomas wrote: > + (if (not nnmail-use-long-file-names) > + (nnheader-replace-chars-in-string group ?. ?/) > + group)) Since directory names cannot have '/' they used to be replaced by '_' in group names before conversion. But this makes it impossible, when generating (non-existent) active files to know whether a '_' in the directory name was _ or / originally. The above patch tries a possible solution inspired from [1] but would break existing users of the cache or agent (xref-find-references "nnmail-group-pathname") who have groups with % or / in their names. Seems to work in my limited testing. WDYT? Regards, James 1: https://stackoverflow.com/questions/9847288/is-it-possible-to-use-in-a-filename#answer-9847319 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-14 3:26 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-15 17:33 ` Daniel Semyonov via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-16 0:22 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-28 9:42 ` Eli Zaretskii 0 siblings, 2 replies; 12+ messages in thread From: Daniel Semyonov via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-15 17:33 UTC (permalink / raw) To: James Thomas; +Cc: Eric Abrahamsen, Eli Zaretskii, 69517 >>>>> James Thomas writes: > Eric Abrahamsen wrote: >> The real problem (well, one of the real problems) is that we should just >> have two central routines for reading and writing active files, so that >> there are only two places to keep in sync. Instead we have those two >> places, and then a smattering of other functions in other places that do >> something similar, and also have to be kept in sync, and I haven't done >> that. At the very least I'll need to apply your patch from 65467. >> >> The other real problem is that gnus-cache is confused about whether it >> wants to be a select method, or a modified version of article saving. >> The presence of `gnus-use-long-file-name' indicates the latter, but the >> manual's instructions about the nnml select method indicates the former. >> >> I think it should be a select method, which means that the group >> directory should be created in "News/cache" the same way it is created >> at the top level: with the "/" replaced by "_", and everything else >> using the proper "left/right" group name. Perhaps "long file names" can >> still play a role, but if so that should be via >> `nnmail-use-long-file-names', and gnus-cache in general should behave >> like a nnmail backend. > Well, I have sort of, an approach based on my earlier patch: [patch] > James Thomas wrote: >> + (if (not nnmail-use-long-file-names) >> + (nnheader-replace-chars-in-string group ?. ?/) >> + group)) > Since directory names cannot have '/' they used to be replaced by '_' in > group names before conversion. But this makes it impossible, when > generating (non-existent) active files to know whether a '_' in the > directory name was _ or / originally. > The above patch tries a possible solution inspired from [1] but would > break existing users of the cache or agent (xref-find-references > "nnmail-group-pathname") who have groups with % or / in their names. > Seems to work in my limited testing. WDYT? I tested it and it seems to work, but I'm pretty sure it will also break existing groups with % or / in their names in several backends. For example, the `nnmh' and `nndiary' backends use this function to locate groups on disk, which will fail for those groups (unless users rename the files manually). FWIW I think this approach is good, but since Gnus doesn't even emit a warning currently when creating a group with % in its name, I don't think breaking these groups is a good idea. Daniel ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-15 17:33 ` Daniel Semyonov via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-16 0:22 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-28 9:42 ` Eli Zaretskii 1 sibling, 0 replies; 12+ messages in thread From: James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-16 0:22 UTC (permalink / raw) To: Daniel Semyonov; +Cc: Eric Abrahamsen, Eli Zaretskii, 69517 Daniel Semyonov wrote: >>>>>> James Thomas writes: > > > James Thomas wrote: > > >> + (if (not nnmail-use-long-file-names) > >> + (nnheader-replace-chars-in-string group ?. ?/) > >> + group)) > > > Since directory names cannot have '/' they used to be replaced by '_' in > > group names before conversion. But this makes it impossible, when > > generating (non-existent) active files to know whether a '_' in the > > directory name was _ or / originally. > > > The above patch tries a possible solution inspired from [1] but would > > break existing users of the cache or agent (xref-find-references > > "nnmail-group-pathname") who have groups with % or / in their names. > > > Seems to work in my limited testing. WDYT? > > I tested it and it seems to work, but I'm pretty sure it will also break > existing groups with % or / in their names in several backends. > For example, the `nnmh' and `nndiary' backends use this function to > locate groups on disk, which will fail for those groups (unless users > rename the files manually). I think only one of these combinations is likely to be a problem in practice: groups such as [Gmail]/Drafts. I've never seen a % in a group name. The cache wouldn't work for one with / anyway (this bug) and as far as the agent is concerned, none of the Gmail groups with spaces in them work anyway (bug#65467: note that the patch on this is also needed for the agent to work on these groups with /). I haven't used nnmh or nndiary but I don't think they normally use group names with '/'. Regards, James ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-15 17:33 ` Daniel Semyonov via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-16 0:22 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 9:42 ` Eli Zaretskii 2024-03-29 0:09 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 1 reply; 12+ messages in thread From: Eli Zaretskii @ 2024-03-28 9:42 UTC (permalink / raw) To: Daniel Semyonov; +Cc: eric, 69517, jimjoe Ping! Is there anything else left to be done here, or should we close this? > From: Daniel Semyonov <daniel@dsemy.com> > Cc: 69517@debbugs.gnu.org, Eric Abrahamsen <eric@ericabrahamsen.net>, Eli > Zaretskii <eliz@gnu.org> > Date: Fri, 15 Mar 2024 19:33:42 +0200 > > >>>>> James Thomas writes: > > > Eric Abrahamsen wrote: > >> The real problem (well, one of the real problems) is that we should just > >> have two central routines for reading and writing active files, so that > >> there are only two places to keep in sync. Instead we have those two > >> places, and then a smattering of other functions in other places that do > >> something similar, and also have to be kept in sync, and I haven't done > >> that. At the very least I'll need to apply your patch from 65467. > >> > >> The other real problem is that gnus-cache is confused about whether it > >> wants to be a select method, or a modified version of article saving. > >> The presence of `gnus-use-long-file-name' indicates the latter, but the > >> manual's instructions about the nnml select method indicates the former. > >> > >> I think it should be a select method, which means that the group > >> directory should be created in "News/cache" the same way it is created > >> at the top level: with the "/" replaced by "_", and everything else > >> using the proper "left/right" group name. Perhaps "long file names" can > >> still play a role, but if so that should be via > >> `nnmail-use-long-file-names', and gnus-cache in general should behave > >> like a nnmail backend. > > > Well, I have sort of, an approach based on my earlier patch: [patch] > > > James Thomas wrote: > > >> + (if (not nnmail-use-long-file-names) > >> + (nnheader-replace-chars-in-string group ?. ?/) > >> + group)) > > > Since directory names cannot have '/' they used to be replaced by '_' in > > group names before conversion. But this makes it impossible, when > > generating (non-existent) active files to know whether a '_' in the > > directory name was _ or / originally. > > > The above patch tries a possible solution inspired from [1] but would > > break existing users of the cache or agent (xref-find-references > > "nnmail-group-pathname") who have groups with % or / in their names. > > > Seems to work in my limited testing. WDYT? > > I tested it and it seems to work, but I'm pretty sure it will also break > existing groups with % or / in their names in several backends. > For example, the `nnmh' and `nndiary' backends use this function to > locate groups on disk, which will fail for those groups (unless users > rename the files manually). > > FWIW I think this approach is good, but since Gnus doesn't even emit a > warning currently when creating a group with % in its name, I don't > think breaking these groups is a good idea. > > Daniel > ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-28 9:42 ` Eli Zaretskii @ 2024-03-29 0:09 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-30 22:21 ` Eric Abrahamsen 0 siblings, 1 reply; 12+ messages in thread From: James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-29 0:09 UTC (permalink / raw) To: Eli Zaretskii; +Cc: eric, Daniel Semyonov, 69517 Eli Zaretskii wrote: > Ping! Is there anything else left to be done here, or should we close > this? > >> From: Daniel Semyonov <daniel@dsemy.com> >> Cc: 69517@debbugs.gnu.org, Eric Abrahamsen <eric@ericabrahamsen.net>, Eli >> Zaretskii <eliz@gnu.org> >> Date: Fri, 15 Mar 2024 19:33:42 +0200 >> >> >>>>> James Thomas writes: >> >> > Eric Abrahamsen wrote: >> >> The real problem (well, one of the real problems) is that we should just >> >> have two central routines for reading and writing active files, so that >> >> there are only two places to keep in sync. Instead we have those two >> >> places, and then a smattering of other functions in other places that do >> >> something similar, and also have to be kept in sync, and I haven't done >> >> that. At the very least I'll need to apply your patch from 65467. >> >> >> >> The other real problem is that gnus-cache is confused about whether it >> >> wants to be a select method, or a modified version of article saving. >> >> The presence of `gnus-use-long-file-name' indicates the latter, but the >> >> manual's instructions about the nnml select method indicates the former. >> >> >> >> I think it should be a select method, which means that the group >> >> directory should be created in "News/cache" the same way it is created >> >> at the top level: with the "/" replaced by "_", and everything else >> >> using the proper "left/right" group name. Perhaps "long file names" can >> >> still play a role, but if so that should be via >> >> `nnmail-use-long-file-names', and gnus-cache in general should behave >> >> like a nnmail backend. >> >> > Well, I have sort of, an approach based on my earlier patch: [patch] >> >> > James Thomas wrote: >> >> >> + (if (not nnmail-use-long-file-names) >> >> + (nnheader-replace-chars-in-string group ?. ?/) >> >> + group)) >> >> > Since directory names cannot have '/' they used to be replaced by '_' in >> > group names before conversion. But this makes it impossible, when >> > generating (non-existent) active files to know whether a '_' in the >> > directory name was _ or / originally. >> >> > The above patch tries a possible solution inspired from [1] but would >> > break existing users of the cache or agent (xref-find-references >> > "nnmail-group-pathname") who have groups with % or / in their names. >> >> > Seems to work in my limited testing. WDYT? >> >> I tested it and it seems to work, but I'm pretty sure it will also break >> existing groups with % or / in their names in several backends. >> For example, the `nnmh' and `nndiary' backends use this function to >> locate groups on disk, which will fail for those groups (unless users >> rename the files manually). >> >> FWIW I think this approach is good, but since Gnus doesn't even emit a >> warning currently when creating a group with % in its name, I don't >> think breaking these groups is a good idea. >> >> Daniel >> I've been using this since then, on master, with no problems. Let me add to my earlier reasons for being confident about this not breaking existing setups: James Thomas wrote: > I think only one of these combinations is likely to be a problem in > practice: groups such as [Gmail]/Drafts. I've never seen a % in a > group name. The cache wouldn't work for one with / anyway (this bug) > and as far as the agent is concerned, none of the Gmail groups with > spaces in them work anyway (bug#65467: note that the patch on this is > also needed for the agent to work on these groups with /). I haven't > used nnmh or nndiary but I don't think they normally use group names > with '/'. The only ones with '/' (and no spaces) I know of are the Gmail groups, namely: [Gmail]/Bin [Gmail]/Drafts [Gmail]/Important [Gmail]/Spam [Gmail]/Starred ...which are unlikely to be agentized because of their nature (though ideally the patch at bug#65467 ought to be applied as well, after this). Moreover, getting Gmail to work with Gnus requires a paid account and a complicated setup with OAuth2 (AFAIK, because I use one) which only few people are likely to have managed. Lastly, the fix for such an unlikely existing setup is a simple rename of the directory: say, from [Gmail]_Drafts to [Gmail]%2FDrafts. Regards, James ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-29 0:09 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-30 22:21 ` Eric Abrahamsen 0 siblings, 0 replies; 12+ messages in thread From: Eric Abrahamsen @ 2024-03-30 22:21 UTC (permalink / raw) To: James Thomas; +Cc: 69517-done, Eli Zaretskii, Daniel Semyonov On 03/29/24 05:39 AM, James Thomas wrote: > Eli Zaretskii wrote: > >> Ping! Is there anything else left to be done here, or should we close >> this? >> >>> From: Daniel Semyonov <daniel@dsemy.com> >>> Cc: 69517@debbugs.gnu.org, Eric Abrahamsen <eric@ericabrahamsen.net>, Eli >>> Zaretskii <eliz@gnu.org> >>> Date: Fri, 15 Mar 2024 19:33:42 +0200 >>> >>> >>>>> James Thomas writes: >>> >>> > Eric Abrahamsen wrote: >>> >> The real problem (well, one of the real problems) is that we should just >>> >> have two central routines for reading and writing active files, so that >>> >> there are only two places to keep in sync. Instead we have those two >>> >> places, and then a smattering of other functions in other places that do >>> >> something similar, and also have to be kept in sync, and I haven't done >>> >> that. At the very least I'll need to apply your patch from 65467. >>> >> >>> >> The other real problem is that gnus-cache is confused about whether it >>> >> wants to be a select method, or a modified version of article saving. >>> >> The presence of `gnus-use-long-file-name' indicates the latter, but the >>> >> manual's instructions about the nnml select method indicates the former. >>> >> >>> >> I think it should be a select method, which means that the group >>> >> directory should be created in "News/cache" the same way it is created >>> >> at the top level: with the "/" replaced by "_", and everything else >>> >> using the proper "left/right" group name. Perhaps "long file names" can >>> >> still play a role, but if so that should be via >>> >> `nnmail-use-long-file-names', and gnus-cache in general should behave >>> >> like a nnmail backend. >>> >>> > Well, I have sort of, an approach based on my earlier patch: [patch] >>> >>> > James Thomas wrote: >>> >>> >> + (if (not nnmail-use-long-file-names) >>> >> + (nnheader-replace-chars-in-string group ?. ?/) >>> >> + group)) >>> >>> > Since directory names cannot have '/' they used to be replaced by '_' in >>> > group names before conversion. But this makes it impossible, when >>> > generating (non-existent) active files to know whether a '_' in the >>> > directory name was _ or / originally. >>> >>> > The above patch tries a possible solution inspired from [1] but would >>> > break existing users of the cache or agent (xref-find-references >>> > "nnmail-group-pathname") who have groups with % or / in their names. >>> >>> > Seems to work in my limited testing. WDYT? >>> >>> I tested it and it seems to work, but I'm pretty sure it will also break >>> existing groups with % or / in their names in several backends. >>> For example, the `nnmh' and `nndiary' backends use this function to >>> locate groups on disk, which will fail for those groups (unless users >>> rename the files manually). >>> >>> FWIW I think this approach is good, but since Gnus doesn't even emit a >>> warning currently when creating a group with % in its name, I don't >>> think breaking these groups is a good idea. >>> >>> Daniel >>> > > I've been using this since then, on master, with no problems. Let me add > to my earlier reasons for being confident about this not breaking > existing setups: > > James Thomas wrote: > >> I think only one of these combinations is likely to be a problem in >> practice: groups such as [Gmail]/Drafts. I've never seen a % in a >> group name. The cache wouldn't work for one with / anyway (this bug) >> and as far as the agent is concerned, none of the Gmail groups with >> spaces in them work anyway (bug#65467: note that the patch on this is >> also needed for the agent to work on these groups with /). I haven't >> used nnmh or nndiary but I don't think they normally use group names >> with '/'. > > The only ones with '/' (and no spaces) I know of are the Gmail groups, > namely: > > [Gmail]/Bin > [Gmail]/Drafts > [Gmail]/Important > [Gmail]/Spam > [Gmail]/Starred > > ...which are unlikely to be agentized because of their nature (though > ideally the patch at bug#65467 ought to be applied as well, after this). > > Moreover, getting Gmail to work with Gnus requires a paid account and a > complicated setup with OAuth2 (AFAIK, because I use one) which only few > people are likely to have managed. > > Lastly, the fix for such an unlikely existing setup is a simple rename > of the directory: say, from [Gmail]_Drafts to [Gmail]%2FDrafts. Thanks, I've now applied this, and am closing the bug. Sorry this took so long -- I kept thinking I would dive a little deeper into the nnmail file/directory shenanigans and get a better grasp on the internals, but obviously was not finding the time. In it goes! Eric ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#69517: [PATCH] Make gnus cache work with group names having '/' 2024-03-10 5:27 ` Eric Abrahamsen 2024-03-10 18:33 ` Eric Abrahamsen @ 2024-03-10 21:32 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 0 replies; 12+ messages in thread From: James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-10 21:32 UTC (permalink / raw) To: 69517; +Cc: Eric Abrahamsen, Eli Zaretskii (This is an interim untested opinion before I can backup my existing messages and verify it...) Eric Abrahamsen wrote: > Thanks for reporting this! Obviously something is wrong here, but I get > different results from your reproduction recipe (I'm running master): > when I create the cache nnml server and hit RET on it, I get: > > K 1: nnml:left.right > K 1: nnml:left/right > > I am able to subscribe to both groups. When I return to the *Group* > buffer, I can enter the version of the group with the ".", and read the > cached article, while attempting to enter the version with the slash > gives me "Invalid group (no such directory)". > > On disk, the "News/cache/active" file starts out with the version with > the ".", and after I create the extra cache server it contains both, it > looks like: > > "nnml:left.right" 1 1 y > "nnml:left/right" 1 1 y > > Meanwhile, the directory structure "News/cache/nnml:left/right/1" is > created. > > The "." in the active file comes from `gnus-cache-generate-active`, and > that mechanism seems to be working fine, at least in my case -- do you > not see that version of the group? `gnus-cache-generate-active` seems to be run if the active file doesn't exist already (i.e. at the very start when creating a server): I was adding new groups to an existing server. If my guess is right, that first line with the dot being generated is a way of reversing the earlier 'dot -> slash' when turning a group into a directory name. That should be fixed as well if group names with slash have to be supported. > while attempting to enter the version with the slash gives me "Invalid > group (no such directory)". Could you try entering it with this patch applied? > > To be honest I have no idea what's happening in `gnus-cache-file-name'. > I'd be hesitant to remove all that code without understanding it better > And I don't think we can argue that that code is "dead" just because it > is behind an option value that isn't one of the customization options. > > Anyway, it would be good to first know why you're not seeing the "." > version of the offending filename, and then I guess try to work out > what's going on with the file names. It kind of looks like a collision > between two different mechanisms -- one that translates "/" to "_" to > prevent unwanted filesystem hierarchies, and one that translates between > "/" and "." in order to convert newsgroup-name dot hierarchies into > filesystem hierarchies. In this case, it seems like "long names" and > newsgroup hierarchies shouldn't be applicable at all. I think if the original group name contains dots, that should be part of the hierarchy. But not slashes. > The other real problem is that gnus-cache is confused about whether it > wants to be a select method, or a modified version of article saving. > The presence of `gnus-use-long-file-name' indicates the latter, but > the manual's instructions about the nnml select method indicates the > former. Hmm.. > I think it should be a select method, which means that the group > directory should be created in "News/cache" the same way it is created > at the top level: with the "/" replaced by "_", and everything else > using the proper "left/right" group name. Perhaps "long file names" > can still play a role, but if so that should be via > `nnmail-use-long-file-names', and gnus-cache in general should behave > like a nnmail backend. I think this patch is on those lines, isn't it? Regards, James ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-03-30 22:21 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-03 1:52 bug#69517: [PATCH] Make gnus cache work with group names having '/' James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-09 8:42 ` Eli Zaretskii 2024-03-09 21:56 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-10 5:27 ` Eric Abrahamsen 2024-03-10 18:33 ` Eric Abrahamsen 2024-03-14 3:26 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-15 17:33 ` Daniel Semyonov via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-16 0:22 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-28 9:42 ` Eli Zaretskii 2024-03-29 0:09 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-30 22:21 ` Eric Abrahamsen 2024-03-10 21:32 ` James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.