bug#40280: gnus-cloud doesn't do anything useful

bug#40280: gnus-cloud doesn't do anything useful

[David Edmondson]

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

After some time away, I'm playing with Gnus again. In this instance,
gnus-cloud isn't working for me.

Specifically, I can add a gnus-cloud storage server (on nnimap) and
register another server (nntp) with it. ~u causes messages to be added
to Emacs-Cloud and they appear to have the right kind of content.

~d on another machine never does anything useful (e.g. mark some
articles as read) because this test:

              (if (and newer (not force-older))
                (gnus-message 3 "Skipping outdated cloud info for group %s, the info is from %s (now is %s)" group date now)

always returns true.

Looking at how `newer' is calculated, it compares the group timestamp
from the gnus-cloud message with the current time. It's not clear to me
how that test is ever going to return `nil', unless I have a version of
emacs running somewhere in the future that updates gnus-cloud.

In general I would have expected to replay any changes with a higher
sequence number, irrespective of the date of the changes. That they
happened in the past is somewhat the point, isn't it? :-)

Another problem is that after a set of Emacs-Cloud messages have been
used to replay, the emacs instance doing that replay doesn't persist the
last seen sequence value as `gnus-cloud-sequence', so the next time it
attempts to replay it will unnecessarily replay the same sequence
numbered messages again.

Attached is a suggested patch. This makes things work for me in some
limited testing.


Gnus v5.13
GNU Emacs 26.1 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.11)
 of 2019-09-09, modified by Debian
200 news.gmane.io InterNetNews NNRP server INN 2.6.3 ready (posting ok)
100 Legal commands
  ARTICLE [message-ID|number]
  AUTHINFO USER name|PASS password|SASL mechanism [initial-response]|GENERIC program [argument ...]
  BODY [message-ID|number]
  CAPABILITIES [keyword]
  COMPRESS DEFLATE
  DATE
  GROUP newsgroup
  HDR header [message-ID|range]
  HEAD [message-ID|number]
  HELP
  IHAVE message-ID
  LAST
  LIST [ACTIVE [wildmat]|ACTIVE.TIMES [wildmat]|COUNTS [wildmat]|DISTRIB.PATS|DISTRIBUTIONS|HEADERS [MSGID|RANGE]|MODERATORS|MOTD|NEWSGROUPS [wildmat]|OVERVIEW.FMT|SUBSCRIPTIONS [wildmat]]
  LISTGROUP [newsgroup [range]]
  MODE READER
  NEWGROUPS [yy]yymmdd hhmmss [GMT]
  NEWNEWS wildmat [yy]yymmdd hhmmss [GMT]
  NEXT
  OVER [range]
  POST
  QUIT
  STARTTLS
  STAT [message-ID|number]
  XGTITLE [wildmat]
  XHDR header [message-ID|range]
  XOVER [range]
  XPAT header message-ID|range pattern [pattern ...]
Report problems to <usenet@ciao.gmane.io>.
.
382 Begin TLS negotiation now
100 Legal commands
  ARTICLE [message-ID|number]
  AUTHINFO USER name|PASS password|SASL mechanism [initial-response]|GENERIC program [argument ...]
  BODY [message-ID|number]
  CAPABILITIES [keyword]
  COMPRESS DEFLATE
  DATE
  GROUP newsgroup
  HDR header [message-ID|range]
  HEAD [message-ID|number]
  HELP
  IHAVE message-ID
  LAST
  LIST [ACTIVE [wildmat]|ACTIVE.TIMES [wildmat]|COUNTS [wildmat]|DISTRIB.PATS|DISTRIBUTIONS|HEADERS [MSGID|RANGE]|MODERATORS|MOTD|NEWSGROUPS [wildmat]|OVERVIEW.FMT|SUBSCRIPTIONS [wildmat]]
  LISTGROUP [newsgroup [range]]
  MODE READER
  NEWGROUPS [yy]yymmdd hhmmss [GMT]
  NEWNEWS wildmat [yy]yymmdd hhmmss [GMT]
  NEXT
  OVER [range]
  POST
  QUIT
  STARTTLS
  STAT [message-ID|number]
  XGTITLE [wildmat]
  XHDR header [message-ID|range]
  XOVER [range]
  XPAT header message-ID|range pattern [pattern ...]
Report problems to <usenet@ciao.gmane.io>.
.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnus-cloud-Improve-cloud-sync.patch --]
[-- Type: text/x-diff, Size: 6196 bytes --]

From a3bdd2219855a72e2ee90de1b6f823c9678a470c Mon Sep 17 00:00:00 2001
From: David Edmondson <dme@dme.org>
Date: Sat, 28 Mar 2020 19:03:58 +0000
Subject: [PATCH] gnus-cloud: Improve cloud sync

After replaying a set of actions downloaded by gnus-cloud, persist the
highest sequence number seen as the local `gnus-cloud-sequence'
number, in order that a future download will not unnecessarily replay
previously seen actions and any future uploads from this emacs
instance use a higher sequence number than that downloaded.

Remove the test on whether individual newsrc entries are older than
the current time, as that is always going to be the case.
---
 lisp/gnus/gnus-cloud.el | 54 ++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/lisp/gnus/gnus-cloud.el b/lisp/gnus/gnus-cloud.el
index da6231d73300..7ea691e7220c 100644
--- a/lisp/gnus/gnus-cloud.el
+++ b/lisp/gnus/gnus-cloud.el
@@ -223,13 +223,10 @@ easy interactive way to set this from the Server buffer."
        (t
         (gnus-message 1 "Unknown type %s; ignoring" type))))))
 
-(defun gnus-cloud-update-newsrc-data (group elem &optional force-older)
-  "Update the newsrc data for GROUP from ELEM.
-Use old data if FORCE-OLDER is not nil."
+(defun gnus-cloud-update-newsrc-data (group elem)
+  "Update the newsrc data for GROUP from ELEM."
   (let* ((contents (plist-get elem :contents))
          (date (or (plist-get elem :timestamp) "0"))
-         (now (gnus-cloud-timestamp nil))
-         (newer (string-lessp date now))
          (group-info (gnus-get-info group)))
     (if (and contents
              (stringp (nth 0 contents))
@@ -238,15 +235,13 @@ Use old data if FORCE-OLDER is not nil."
             (if (equal (format "%S" group-info)
                        (format "%S" contents))
                 (gnus-message 3 "Skipping cloud update of group %s, the info is the same" group)
-              (if (and newer (not force-older))
-                (gnus-message 3 "Skipping outdated cloud info for group %s, the info is from %s (now is %s)" group date now)
-                (when (or (not gnus-cloud-interactive)
-                          (gnus-y-or-n-p
-                           (format "%s has older different info in the cloud as of %s, update it here? "
-				   group date)))
-		  (gnus-message 2 "Installing cloud update of group %s" group)
-		  (gnus-set-info group contents)
-		  (gnus-group-update-group group))))
+              (when (or (not gnus-cloud-interactive)
+			(gnus-y-or-n-p
+			 (format "%s has different info in the cloud from %s, update it here? "
+				 group date)))
+		(gnus-message 2 "Installing cloud update of group %s" group)
+		(gnus-set-info group contents)
+		(gnus-group-update-group group)))
           (gnus-error 1 "Sorry, group %s is not subscribed" group))
       (gnus-error 1 "Sorry, could not update newsrc for group %s (invalid data %S)"
                   group elem))))
@@ -380,8 +375,9 @@ When FULL is t, upload everything, not just a difference from the last full."
                   (gnus-cloud-files-to-upload full)
                   (gnus-cloud-collect-full-newsrc)))
           (group (gnus-group-full-name gnus-cloud-group-name gnus-cloud-method)))
+      (setq gnus-cloud-sequence (1+ (or gnus-cloud-sequence 0)))
       (insert (format "Subject: (sequence: %s type: %s storage-method: %s)\n"
-                      (or gnus-cloud-sequence "UNKNOWN")
+                      gnus-cloud-sequence
                       (if full :full :partial)
                       gnus-cloud-storage-method))
       (insert "From: nobody@gnus.cloud.invalid\n")
@@ -390,7 +386,6 @@ When FULL is t, upload everything, not just a difference from the last full."
       (if (gnus-request-accept-article gnus-cloud-group-name gnus-cloud-method
                                        t t)
           (progn
-            (setq gnus-cloud-sequence (1+ (or gnus-cloud-sequence 0)))
             (gnus-cloud-add-timestamps elems)
             (gnus-message 3 "Uploaded Gnus Cloud data successfully to %s" group)
             (gnus-group-refresh-group group))
@@ -459,18 +454,21 @@ instead of `gnus-cloud-sequence'.
 When UPDATE is t, returns the result of calling `gnus-cloud-update-all'.
 Otherwise, returns the Gnus Cloud data chunks."
   (let ((articles nil)
+	(highest-sequence-seen gnus-cloud-sequence)
         chunks)
     (dolist (header (gnus-cloud-available-chunks))
-      (when (> (gnus-cloud-chunk-sequence (mail-header-subject header))
-               (or sequence-override gnus-cloud-sequence -1))
-
-        (if (string-match (format "storage-method: %s" gnus-cloud-storage-method)
-                          (mail-header-subject header))
-            (push (mail-header-number header) articles)
-          (gnus-message 1 "Skipping article %s because it didn't match the Gnus Cloud method %s: %s"
-                        (mail-header-number header)
-                        gnus-cloud-storage-method
-                        (mail-header-subject header)))))
+      (let ((this-sequence (gnus-cloud-chunk-sequence (mail-header-subject header))))
+	(when (> this-sequence (or sequence-override gnus-cloud-sequence -1))
+
+	  (if (string-match (format "storage-method: %s" gnus-cloud-storage-method)
+			    (mail-header-subject header))
+	      (progn
+		(push (mail-header-number header) articles)
+		(setq highest-sequence-seen (max highest-sequence-seen this-sequence)))
+	    (gnus-message 1 "Skipping article %s because it didn't match the Gnus Cloud method %s: %s"
+			  (mail-header-number header)
+			  gnus-cloud-storage-method
+			  (mail-header-subject header))))))
     (when articles
       (nnimap-request-articles (nreverse articles) gnus-cloud-group-name)
       (with-current-buffer nntp-server-buffer
@@ -480,7 +478,9 @@ Otherwise, returns the Gnus Cloud data chunks."
           (push (gnus-cloud-parse-chunk) chunks)
           (forward-line 1))))
     (if update
-        (mapcar #'gnus-cloud-update-all chunks)
+        (progn
+	  (mapcar #'gnus-cloud-update-all chunks)
+	  (setq gnus-cloud-sequence highest-sequence-seen))
       chunks)))
 
 (defun gnus-cloud-server-p (server)
-- 
2.25.1


[-- Attachment #3: Type: text/plain, Size: 71 bytes --]


dme.
-- 
And the sign said: long haired freaky people need not apply.

bug#40280: gnus-cloud doesn't do anything useful

[Mathieu Othacehe]

Hello,

> Looking at how `newer' is calculated, it compares the group timestamp
> from the gnus-cloud message with the current time. It's not clear to me
> how that test is ever going to return `nil', unless I have a version of
> emacs running somewhere in the future that updates gnus-cloud.

I share the same conclusions on 26.3, the downloaded message is always
older than current time, and nothing happens.

The patch you are proposing fixes gnus cloud update for me.

Thanks,

Mathieu

bug#40280: gnus-cloud doesn't do anything useful

[Ted Zlatanov]

On Wed, 13 May 2020 12:21:42 +0200 Mathieu Othacehe <othacehe@gnu.org> wrote: 

>> Looking at how `newer' is calculated, it compares the group timestamp
>> from the gnus-cloud message with the current time. It's not clear to me
>> how that test is ever going to return `nil', unless I have a version of
>> emacs running somewhere in the future that updates gnus-cloud.

MO> I share the same conclusions on 26.3, the downloaded message is always
MO> older than current time, and nothing happens.

MO> The patch you are proposing fixes gnus cloud update for me.

I agree this is a problem and agree with David's patch.

I've pushed the patch to branch scratch/tzz/dme/bug-40280-gnus-cloud-newest

I'll merge it into master and adjust any docs needed if I can get a
second review please :)

David, thank you very much for contributing the fix.

Ted

bug#40280: gnus-cloud doesn't do anything useful

[Ted Zlatanov]

On Tue, 09 Jun 2020 16:41:29 +0000 Ted Zlatanov <tzz@lifelogs.com> wrote: 

TZ> On Wed, 13 May 2020 12:21:42 +0200 Mathieu Othacehe <othacehe@gnu.org> wrote: 
>>> Looking at how `newer' is calculated, it compares the group timestamp
>>> from the gnus-cloud message with the current time. It's not clear to me
>>> how that test is ever going to return `nil', unless I have a version of
>>> emacs running somewhere in the future that updates gnus-cloud.

MO> I share the same conclusions on 26.3, the downloaded message is always
MO> older than current time, and nothing happens.

MO> The patch you are proposing fixes gnus cloud update for me.

TZ> I agree this is a problem and agree with David's patch.

TZ> I've pushed the patch to branch scratch/tzz/dme/bug-40280-gnus-cloud-newest

TZ> I'll merge it into master and adjust any docs needed if I can get a
TZ> second review please :)

David, Mathieu:

The patch and commit message looked good so I've pushed this.

Please confirm the fix works well for you and I'll close the bug.

Thanks
Ted

bug#40280: gnus-cloud doesn't do anything useful

[Basil L. Contovounesios]

> From a3bdd2219855a72e2ee90de1b6f823c9678a470c Mon Sep 17 00:00:00 2001
> From: David Edmondson <dme@dme.org>
> Date: Sat, 28 Mar 2020 19:03:58 +0000
> Subject: [PATCH] gnus-cloud: Improve cloud sync
>
> After replaying a set of actions downloaded by gnus-cloud, persist the
> highest sequence number seen as the local `gnus-cloud-sequence'
> number, in order that a future download will not unnecessarily replay
> previously seen actions and any future uploads from this emacs
> instance use a higher sequence number than that downloaded.
>
> Remove the test on whether individual newsrc entries are older than
> the current time, as that is always going to be the case.

In the future please follow the formatting guidelines for change log
messages described in the file CONTRIBUTE.

[...]

> --- a/lisp/gnus/gnus-cloud.el
> +++ b/lisp/gnus/gnus-cloud.el
> @@ -480,7 +478,9 @@ Otherwise, returns the Gnus Cloud data chunks."
>            (push (gnus-cloud-parse-chunk) chunks)
>            (forward-line 1))))
>      (if update
> -        (mapcar #'gnus-cloud-update-all chunks)
> +        (progn
> +	  (mapcar #'gnus-cloud-update-all chunks)
> +	  (setq gnus-cloud-sequence highest-sequence-seen))
>        chunks)))

This change means the result of the mapcar is no longer used or
returned, giving rise to the following warning:

  In gnus-cloud-download-data:
  gnus/gnus-cloud.el:482:12: Warning: ‘mapcar’ called for effect; use ‘mapc’ or
      ‘dolist’ instead

It also makes highest-sequence-seen the return value of
gnus-cloud-download-data.  Is this intended?

Thanks,

-- 
Basil

bug#40280: gnus-cloud doesn't do anything useful

[David Edmondson]

On Tuesday, 2020-06-16 at 19:56:05 +01, Basil L. Contovounesios wrote:

>> From a3bdd2219855a72e2ee90de1b6f823c9678a470c Mon Sep 17 00:00:00 2001
>> From: David Edmondson <dme@dme.org>
>> Date: Sat, 28 Mar 2020 19:03:58 +0000
>> Subject: [PATCH] gnus-cloud: Improve cloud sync
>>
>> After replaying a set of actions downloaded by gnus-cloud, persist the
>> highest sequence number seen as the local `gnus-cloud-sequence'
>> number, in order that a future download will not unnecessarily replay
>> previously seen actions and any future uploads from this emacs
>> instance use a higher sequence number than that downloaded.
>>
>> Remove the test on whether individual newsrc entries are older than
>> the current time, as that is always going to be the case.
>
> In the future please follow the formatting guidelines for change log
> messages described in the file CONTRIBUTE.

Understood.

> [...]
>
>> --- a/lisp/gnus/gnus-cloud.el
>> +++ b/lisp/gnus/gnus-cloud.el
>> @@ -480,7 +478,9 @@ Otherwise, returns the Gnus Cloud data chunks."
>>            (push (gnus-cloud-parse-chunk) chunks)
>>            (forward-line 1))))
>>      (if update
>> -        (mapcar #'gnus-cloud-update-all chunks)
>> +        (progn
>> +	  (mapcar #'gnus-cloud-update-all chunks)
>> +	  (setq gnus-cloud-sequence highest-sequence-seen))
>>        chunks)))
>
> This change means the result of the mapcar is no longer used or
> returned, giving rise to the following warning:
>
>   In gnus-cloud-download-data:
>   gnus/gnus-cloud.el:482:12: Warning: ‘mapcar’ called for effect; use ‘mapc’ or
>       ‘dolist’ instead
>
> It also makes highest-sequence-seen the return value of
> gnus-cloud-download-data.  Is this intended?

No, it's not an intended change and is not desirable. Presumably
replacing `progn' with `prog1' would properly restore the previous
behaviour.

Should I submit a further patch?

dme.
-- 
J'aurais toujours faim de toi.

bug#40280: gnus-cloud doesn't do anything useful

[Ted Zlatanov]

On Wed, 17 Jun 2020 09:13:29 +0100 David Edmondson <dme@dme.org> wrote: 

DE> On Tuesday, 2020-06-16 at 19:56:05 +01, Basil L. Contovounesios wrote:
>> This change means the result of the mapcar is no longer used or
>> returned, giving rise to the following warning:
>> 
>> In gnus-cloud-download-data:
>> gnus/gnus-cloud.el:482:12: Warning: ‘mapcar’ called for effect; use ‘mapc’ or
>> ‘dolist’ instead
>> 
>> It also makes highest-sequence-seen the return value of
>> gnus-cloud-download-data.  Is this intended?

DE> No, it's not an intended change and is not desirable. Presumably
DE> replacing `progn' with `prog1' would properly restore the previous
DE> behaviour.

DE> Should I submit a further patch?

Please do. Thanks to you and Basil.

Ted

bug#40280: [PATCH] Fix gnus-cloud-download-all-data return value

[David Edmondson]

* lisp/gnus/gnus-cloud.el (gnus-cloud-download-data): Return the
result of calling `gnus-cloud-update-all' when UPDATE is t, as per the
documented behaviour.
---
 lisp/gnus/gnus-cloud.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/gnus/gnus-cloud.el b/lisp/gnus/gnus-cloud.el
index 5028da5e8d..f730518241 100644
--- a/lisp/gnus/gnus-cloud.el
+++ b/lisp/gnus/gnus-cloud.el
@@ -478,8 +478,8 @@ Otherwise, returns the Gnus Cloud data chunks."
           (push (gnus-cloud-parse-chunk) chunks)
           (forward-line 1))))
     (if update
-        (progn
-	  (mapc #'gnus-cloud-update-all chunks)
+        (prog1
+	  (mapcar #'gnus-cloud-update-all chunks)
 	  (setq gnus-cloud-sequence highest-sequence-seen))
       chunks)))
 
-- 
2.27.0

bug#40280: [PATCH] Fix gnus-cloud-download-all-data return value

[Ted Zlatanov]

On Tue, 23 Jun 2020 12:52:34 +0100 David Edmondson <dme@dme.org> wrote: 

DE> * lisp/gnus/gnus-cloud.el (gnus-cloud-download-data): Return the
DE> result of calling `gnus-cloud-update-all' when UPDATE is t, as per the
DE> documented behaviour.

Thanks David. Applied with slightly adjusted commit message.

Ted

bug#40280: [PATCH] Fix gnus-cloud-download-all-data return value

[Lars Ingebrigtsen]

Ted Zlatanov <tzz@lifelogs.com> writes:

> On Tue, 23 Jun 2020 12:52:34 +0100 David Edmondson <dme@dme.org> wrote: 
>
> DE> * lisp/gnus/gnus-cloud.el (gnus-cloud-download-data): Return the
> DE> result of calling `gnus-cloud-update-all' when UPDATE is t, as per the
> DE> documented behaviour.
>
> Thanks David. Applied with slightly adjusted commit message.

If I understand correctly, the matters in this bug reports were all
fixed, and I'm now closing this bug report.  Please reopen if that isn't
the case.

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