Subject: Re: bug#33653: 27.0.50; Change Gnus obarrays-as-hash-tables into real hash tables

From: Andy Moreton <[email-masked]>
Date: Mon, 25 Mar 2019 14:45:24 +0000

On Sun 24 Mar 2019, Eric Abrahamsen wrote:

> Katsumi Yamaoka <[email-masked]> writes:
>
>> Hi,
>>
>> Gnus got not to work for groups of which the group name contains
>> non-ASCII letters.  For instance, I got this error when trying
>> to update the "nnml:テスト" group using `M-g'[1]:
>>
>> nnml:\343\203\206\343\202\271\343\203\210 error: No such group: テスト
>>
>> When trying to enter the group using `0 RET'[2] I got:
>>
>> Group nnml:\343\203\206\343\202\271\343\203\210 couldn't be activated
>>
>> Those raw bytes are utf-8 encoded "テスト", that is also used in
>> the group entry in gnus-newsrc-alist saved in the ~/.newsrc.eld
>> file as follows:
>>
>> ("nnml:\343\203\206\343\202\271\343\203\210" 1 nil ((unexist) (seen (1
>> . 5))) "nnml:" ((timestamp 23704 11958)))
>
> Yes, this is something I screwed up in c1b63af445. Gnus has always
> stored group names as raw bytes in.newsrc.eld (at least I believe it
> has, you probably know better than I do, it does in my experiments with
> Emacs 26, anyway), and only encodes during display. But obviously I've
> messed something up between file persistence and display, and I'm
> working on sorting it out.

Perhaps it would be better to revert and reintroduce your changes after
further testing ? Taking time over this is better than causing data loss
for gnus users.

Other notes from reading the code:

1) In `gnus-gnus-to-quick-newsrc-format' you ignore the contents of
   `gnus-newsrc-alist' when saving "newsrc.eld", and replace it with the
   details from `gnus-newsrc-hashtb'. Why ? The rest of the gnus code
   appears to treat `gnus-newsrc-alist' as the single source of truth,
   with the hash tables being used only for faster access to it.

2) In `gnus-gnus-to-quick-newsrc-format' you dropped the code to remove
   the dummy group from `gnus-newsrc-alist'. Why ? This internal dummy
   group is now saved in "newsrc.eld", which is not needed.

3) The format of the entries in `gnus-newsrc-hashtb' has changed,
   removing the second element. Why ?

4) You changed several hash tale sizesfrom 4096 to 4000, and 1024 to
   1000. Why ?

Your patch contains several logical changes that would be easier to
understand (and bisect) as a series of patches with one logical change
in each patch:
 - code layout changes
 - add missing doc strings and code comments
 - change hash table implementation
 - change format of `gnus-newsrc-hashtb' entries
 - change usage of `gnus-group-change-level'
 - change coding of group names
While it can take extra work to split things up, the end result is much
easier to understand.

Thanks for working on gnus,

    AndyM