bug#70579: 30.0.50; gnus: Wrong unread count in the Group buffer

[Eric Abrahamsen <eric@ericabrahamsen.net>]

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

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>> Ping!  Eric, can we make some progress here?
>>>
>>>> Cc: jimjoe@gmx.net
>>>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>>>> Date: Thu, 25 Apr 2024 21:34:45 -0700
>>>> 
>>>> James Thomas via "Bug reports for GNU Emacs, the Swiss army knife of
>>>> text editors" <bug-gnu-emacs@gnu.org> writes:
>>>> 
>>>> > - (Preferably starting with an empty drafts folder) Compose a message
>>>> >   and save it.
>>>> > - Open the drafts group, press e on the message and then kill the new
>>>> >   buffer; then (incidentally, if you now do '/ N' then this bug does not
>>>> >   arise) delete the message (B DEL)
>>>> > - Press q
>>>> > - The message count is wrong (but can be corrected with M-g)
>>>> >
>>>> > cf. In gnus.general (gnus-summary-goto-article "87y192lr8f.fsf@gmx.net")
>>
>> I've made some progress here -- the root of the problem seems to be
>> that, when we hit "e" in the draft summary buffer to resume editing a
>> draft, Gnus "jumps ahead" in message numbers. Basically what "editing"
>> actually means is that the old draft is deleted, and a new draft is
>> started, but the new draft has a article number that's the previous
>> draft's number + 2, and the "draft" group's active number is also
>> inflated (for instance (12 . 14) when it should be (12 . 13)). I was
>> also able to get it to jump three numbers in some cases.
>>
>>>>From this point, *any* normal usage will end up correcting the error:
>> using "C-c C-k" to kill the editing buffer (instead of "C-x k") or as
>> you noted any of the commands that lead to refreshing the unread count.
>> But if you don't use any of those commands, you'll see the inflated
>> active/unread count when you get back to the *Group* buffer (the "B DEL"
>> isn't necessary for the recipe, and in fact at that stage the message
>> under point has already been deleted).
>>
>> That's as far as I've gotten, and I'll keep working on why the article
>> number starts off inflated. But in the meantime, the solution is "don't
>> do that".
>
> Sorry, that sounded a bit unfriendly, when I was the one who asked you
> to submit the bug report! An hour or two of chasing Gnus function calls
> will do that to you...

Okay, I found two things, one the proximate cause of this bug, another
"probably wrong" adjacent issue.

The main problem is that `gnus-draft-setup' both calls `message-mode'
(which calls `message-set-auto-save-file-name'), and then directly calls
`message-set-auto-save-file-name' itself. Without getting into horrible
details, that functions shouldn't be called twice, because it generates
an extra numerical file name, which ends up inflating the active value
of the drafts group.

I've attached a patch that removes the second call. If you feel
comfortable applying and testing patches, I hope you'll try it. In my
experiments it fixes the problem.

The patch also semi-addresses the second issue. When saving a draft,
message-mode only buries the buffer, it doesn't delete it. If you go
back and start editing the draft, `gnus-draft-check-draft-articles' is
supposed to see if a there's already a buffer visiting the draft file,
and return you to that buffer instead of creating a new one. But it only
does that if the buffer is modified, which it isn't if you've
saved/buried the draft.

I don't see why that should mean that you need a whole new buffer for
editing the message, and the fact that there are now two "copies" of the
message buffer causes further problems with the inflating article
numbers (why I could sometimes see three or even four "jumps").

The patch removes the check for modification, and in my brief testing
behaves the way I would expect it to.

That whole part of the code is weird:

    (let ((buffers (buffer-list))
	  file buffs buff)
      (save-current-buffer
	(while (and articles
		    (not buff))
	  (setq file (nndraft-article-filename (pop articles))
		buffs buffers)
	  (while buffs
	    (set-buffer (setq buff (pop buffs)))
	    (if (and buffer-file-name
		     (equal (file-remote-p file)
			    (file-remote-p buffer-file-name))
		     (string-equal (file-truename buffer-file-name)
				   (file-truename file))
		     ; (buffer-modified-p)
                     )
		(setq buffs nil)
	      (setq buff nil)))))

That seems like a very long way of writing `find-buffer-visiting'. Maybe
the remote check should be carried over, but otherwise this seems very
replaceable.

Anyway, please let me know if you can check the patch.

Eric

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gnus-draft-fixes.diff --]
[-- Type: text/x-patch, Size: 928 bytes --]

diff --git a/lisp/gnus/gnus-draft.el b/lisp/gnus/gnus-draft.el
index 1fc2b33fffb..406ff705f3d 100644
--- a/lisp/gnus/gnus-draft.el
+++ b/lisp/gnus/gnus-draft.el
@@ -262,8 +262,7 @@ gnus-draft-setup
             (setq ga
                   (message-fetch-field gnus-draft-meta-information-header)))
           (insert mail-header-separator)
-          (forward-line 1)
-          (message-set-auto-save-file-name))))
+          (forward-line 1))))
     (gnus-backlog-remove-article group narticle)
     (when (and ga
                (ignore-errors (setq ga (car (read-from-string ga)))))
@@ -303,8 +302,7 @@ gnus-draft-check-draft-articles
 		     (equal (file-remote-p file)
 			    (file-remote-p buffer-file-name))
 		     (string-equal (file-truename buffer-file-name)
-				   (file-truename file))
-		     (buffer-modified-p))
+				   (file-truename file)))
 		(setq buffs nil)
 	      (setq buff nil)))))
       (when buff