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

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

[Eric Abrahamsen]

Here's the next thing: turning Gnus' obarrays-as-hash-tables into real
hash tables. Gnus currently stores information about groups by coercing
group names to unibyte, interning them in custom obarrays, and then
setting their symbol-value to whatever value needs to be stored. I think
all this was written before Emacs had actual hash tables.

I think real hash tables are better for a few reasons:

1. Hash table lookups seem to be marginally faster than obarray lookups,
2. It's "less weird" for contributors and bug hunters,
3. It allows us to reduces the amount of encoding/decoding going on:
group names can stay strings instead of being forced into symbols.

I've pushed a branch, scratch/gnus-hashtables, with two commits in it.
The first changes all the obarrays to hash tables. Apart from simply
replacing function calls, there were a few bumps.

1. Gnus uses `text-property-any' to find and compare group-name text
   properties; I made a new `gnus-text-property-search' which behaves
   similarly, but compares with equal.
2. Some of the "hash tables" were simply storing the value t, ie just
   used for membership. I left these as hash tables, but I think in many
   cases simple string-in-list membership tests would suffice.
3. The hash table in gnus-async.el didn't appear to be doing anything --
   it seemed to be redundant with `gnus-async-article-alist'. I've
   removed it, but it might need to be put back if I'm missing something.
4. The creation of `gnus-newsgroup-dependencies' was the most fiddly,
   and I added tests for that. I'm not entirely convinced that
   `gnus-thread-loop-p' behaves the way it's meant to, it appears to
   only check for direct loops between parent and child, not parent and
   descendant.
5. The old return value of (gnus-gethash <group-name>
   gnus-newsrc-hashtb) was kind of a slice into `gnus-newsrc-alist': it
   behaved a bit like `member', but also included the group *before* the
   group you wanted, as well as all those after, so you could traverse
   the list in either direction. It now no longer preserves the order of
   `gnus-newsrc-alist' (this ordering wasn't actually used in many
   places), and instead there's a new variable `gnus-group-list' which
   records the proper sort order of the groups.

I feel fairly confident that all this is working okay. The second commit
I do *not* feel very confident about, and it's more of a "let's see how
things break" attempt. In theory, we should now be able to limit group
name encoding/decoding to the boundaries of Gnus -- reading/writing
active files, and talking to servers. Within Gnus, the group names can
remain decoded strings.

In the second commit I've just gone in and pulled out all the decoding I
can find, changed all the 'raw-text encoding options to 'undecided, and
deleted the `mm-disable-multibyte's. I have no confidence that I've
covered all the bases, though I have been using this branch for a couple
of weeks, with nnml, nnmaildir, and nnimap groups with multibyte names,
and so far nothing has broken. I'm less confident about nntp.

I'll continue working on this, and I hope I can get some feedback here,
particularly on the second commit.

Thanks, Eric

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

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Here's the next thing: turning Gnus' obarrays-as-hash-tables into real
> hash tables. Gnus currently stores information about groups by coercing
> group names to unibyte, interning them in custom obarrays, and then
> setting their symbol-value to whatever value needs to be stored. I think
> all this was written before Emacs had actual hash tables.

Also hounding Lars for his opinion...

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

[Eric Abrahamsen]

On 12/11/18 20:23 PM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>>> Here's the next thing: turning Gnus' obarrays-as-hash-tables into real
>>> hash tables. Gnus currently stores information about groups by coercing
>>> group names to unibyte, interning them in custom obarrays, and then
>>> setting their symbol-value to whatever value needs to be stored. I think
>>> all this was written before Emacs had actual hash tables.
>>
>> Also hounding Lars for his opinion...
>
> Using real hash tables sounds nice, if there's no negative performance
> impact.

Cool! In fact it seems to be a hair faster.

If you have a moment for this, would you look at the second commit in
the scratch/gnus-hashtables branch? The first commit seems fairly
stable, but the second has to do with encoding -- if group names never
need to be unibyte, it should be possible to leave them decoded inside
Gnus itself.

Besides active files and server communication, are there any other
places where group names would need to be encoded?

Thanks,
Eric

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

[Eric Abrahamsen]

On 12/11/18 20:23 PM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>>> Here's the next thing: turning Gnus' obarrays-as-hash-tables into real
>>> hash tables. Gnus currently stores information about groups by coercing
>>> group names to unibyte, interning them in custom obarrays, and then
>>> setting their symbol-value to whatever value needs to be stored. I think
>>> all this was written before Emacs had actual hash tables.
>>
>> Also hounding Lars for his opinion...
>
> Using real hash tables sounds nice, if there's no negative performance
> impact.

I've pushed another commit to scratch/gnus-hashtables, fixing a small
bug in gnus-bklg.el. Otherwise, I've been using this branch for a couple
months now, with no ill effects.

I guess what I'd like to do is squash the first commit with the third
(the changes that actually implement the hash tables), but leave the
second aside for now (the one that stops doing internal
encoding/decoding).

That ought to be done eventually, but I'll need more time to figure it
out, and would like to get more exposure for this refactoring in the
meantime.

Eric

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

[Eric Abrahamsen]

On 02/04/19 18:05 PM, Eric Abrahamsen wrote:
> On 12/11/18 20:23 PM, Lars Ingebrigtsen wrote:
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>>> Here's the next thing: turning Gnus' obarrays-as-hash-tables into real
>>>> hash tables. Gnus currently stores information about groups by coercing
>>>> group names to unibyte, interning them in custom obarrays, and then
>>>> setting their symbol-value to whatever value needs to be stored. I think
>>>> all this was written before Emacs had actual hash tables.
>>>
>>> Also hounding Lars for his opinion...
>>
>> Using real hash tables sounds nice, if there's no negative performance
>> impact.
>
> I've pushed another commit to scratch/gnus-hashtables, fixing a small
> bug in gnus-bklg.el. Otherwise, I've been using this branch for a couple
> months now, with no ill effects.
>
> I guess what I'd like to do is squash the first commit with the third
> (the changes that actually implement the hash tables), but leave the
> second aside for now (the one that stops doing internal
> encoding/decoding).

Weeeeelll... I'd really like to push this. I promise to be on hand with
quick fixes, and a revert if necessary.

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

[Robert Pluim]

>>>>> On Thu, 21 Mar 2019 17:09:07 -0700, Eric Abrahamsen <eric@ericabrahamsen.net> said:

    Eric> Weeeeelll... I'd really like to push this. I promise to be
    Eric> on hand with quick fixes, and a revert if necessary.

This is for emacs-27, right? Iʼd say go for it :-)

Robert

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

[Eric Abrahamsen]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Thu, 21 Mar 2019 17:09:07 -0700, Eric Abrahamsen <eric@ericabrahamsen.net> said:
>
>     Eric> Weeeeelll... I'd really like to push this. I promise to be
>     Eric> on hand with quick fixes, and a revert if necessary.
>
> This is for emacs-27, right? 

Most definitely.

> Iʼd say go for it :-)

Okay, if it explodes I'll say you said I could do it! :)

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

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Robert Pluim <rpluim@gmail.com> writes:
>
>>>>>>> On Thu, 21 Mar 2019 17:09:07 -0700, Eric Abrahamsen <eric@ericabrahamsen.net> said:
>>
>>     Eric> Weeeeelll... I'd really like to push this. I promise to be
>>     Eric> on hand with quick fixes, and a revert if necessary.
>>
>> This is for emacs-27, right? 
>
> Most definitely.
>
>> Iʼd say go for it :-)
>
> Okay, if it explodes I'll say you said I could do it! :)

And... it exploded!

Well, not really, but on my second machine there were a pile of stale
*.elc files that aren't rebuilt with the new macro definitions, and even
"make distclean" doesn't get rid of them (?), I had to go into the
lisp/gnus directory, manually delete the elc files, and run "make"
again. (Or maybe this is what "make bootstrap" does?)

This must happen from time to time -- is there anything to be done
besides warning people on the mailing lists?

The bummer is that you don't get an error until you run Gnus, so it just
looks like I broke Gnus :(

Eric

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

[Eli Zaretskii]

> From: Eric Abrahamsen <eric@ericabrahamsen.net>
> Date: Fri, 22 Mar 2019 12:54:54 -0700
> 
> is there anything to be done besides warning people on the mailing
> lists?

A warning would be nice, yes.

> "make distclean" doesn't get rid of them (?), I had to go into the
> lisp/gnus directory, manually delete the elc files, and run "make"
> again. (Or maybe this is what "make bootstrap" does?)

Yes, that's what a bootstrap does (if you don't mind doing a bootstrap
in such situations).

Thanks.

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

[Eric Abrahamsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>> Date: Fri, 22 Mar 2019 12:54:54 -0700
>> 
>> is there anything to be done besides warning people on the mailing
>> lists?
>
> A warning would be nice, yes.

Okay, I'll do that. I've already gotten a bug report :(

>> "make distclean" doesn't get rid of them (?), I had to go into the
>> lisp/gnus directory, manually delete the elc files, and run "make"
>> again. (Or maybe this is what "make bootstrap" does?)
>
> Yes, that's what a bootstrap does (if you don't mind doing a bootstrap
> in such situations).

Got it, thank you.

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

[Glenn Morris]

Eric Abrahamsen wrote:


> even "make distclean" doesn't get rid of them (?),

Because .elc files are distributed in release tarfiles.

> This must happen from time to time -- is there anything to be done
> besides warning people on the mailing lists?

1a) make bootstrap the default make target.
1b) rename the current default make target as
faster-but-sometimes-will-go-wrong-for-obscure-reasons

CPU time is cheaper than human time spent figuring out problems.

2) Make load-prefer-newer non-nil by default.

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

[Eric Abrahamsen]

On 03/22/19 18:40 PM, Glenn Morris wrote:
> Eric Abrahamsen wrote:
>
>
>> even "make distclean" doesn't get rid of them (?),
>
> Because .elc files are distributed in release tarfiles.

Right, that makes sense.

>> This must happen from time to time -- is there anything to be done
>> besides warning people on the mailing lists?
>
> 1a) make bootstrap the default make target.
> 1b) rename the current default make target as
> faster-but-sometimes-will-go-wrong-for-obscure-reasons

Obviously this is the best solution!

> CPU time is cheaper than human time spent figuring out problems.
>
> 2) Make load-prefer-newer non-nil by default.

I didn't know about this, and really like it. Recently Emacs seems to
save up all the "foo.el is newer than compiled file" warnings and feed
them to me at random intervals, usually while I'm trying to use the
minibuffer. Maybe this option will fix that.

Thanks,
Eric

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

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Fri, 22 Mar 2019 18:40:45 -0400
> Cc: 33653@debbugs.gnu.org
> 
> 1a) make bootstrap the default make target.

Don't know about the others, but almost I never bootstrap, except in a
fresh checkout.  The reasons are (a) it's slow, and I don't want to
wait for 10 or so minutes just because I updated from upstream; and
(b) it deletes all previous executables of Emacs, which I use for
fast-bisecting reported problems.

When these rare situations happen, it is not hard to guess the file
whose .elc should be removed and regenerated; if worse comes to worst,
deleting all the *.elc files in the relevant subdirectory (lisp/gnus/
in this case) and typing "make" solves the problem much faster for me,
and without the unwanted side effects.

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

[Andy Moreton]

On Fri 22 Mar 2019, Eric Abrahamsen wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>>> Date: Fri, 22 Mar 2019 12:54:54 -0700
>>> 
>>> is there anything to be done besides warning people on the mailing
>>> lists?
>>
>> A warning would be nice, yes.
>
> Okay, I'll do that. I've already gotten a bug report :(

After bootstrapping, I observe that the summary buffer does not update
the faces correctly for nntp articles that have been read: they still
have an unread face.

    AndyM

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

[Eric Abrahamsen]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> On Fri 22 Mar 2019, Eric Abrahamsen wrote:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>>>> Date: Fri, 22 Mar 2019 12:54:54 -0700
>>>> 
>>>> is there anything to be done besides warning people on the mailing
>>>> lists?
>>>
>>> A warning would be nice, yes.
>>
>> Okay, I'll do that. I've already gotten a bug report :(
>
> After bootstrapping, I observe that the summary buffer does not update
> the faces correctly for nntp articles that have been read: they still
> have an unread face.

Thanks for testing!

I saw that problem while working on this patch, and thought it was
solved. I'll revisit the issue, and come back to you if I have more
questions.

Thanks again,
Eric

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

[Bastien]

Hi Eric,

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> The bummer is that you don't get an error until you run Gnus, so it just
> looks like I broke Gnus :(

I hope not!  :)

I have not followed this change.
Can it corrupt the .newsrc.eld file?

-- 
 Bastien

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

[Eric Abrahamsen]

On 03/24/19 23:29 PM, Bastien wrote:
> Hi Eric,
>
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> The bummer is that you don't get an error until you run Gnus, so it just
>> looks like I broke Gnus :(
>
> I hope not!  :)
>
> I have not followed this change.
> Can it corrupt the .newsrc.eld file?

Worst come to worst it will write non-ascii group names as unibyte when
they should be utf-8, or else the other way around. I thought I could
avoid dealing with this question for now, and just preserve Gnus'
original method of saving group names, but it looks like I may have
forced the issue instead...

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

[Katsumi Yamaoka]

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)))

Such groups I have are only a few, mainly for testing, so not so
damaged though, I'm afraid there may be those who using them as
normal.  Info says that nntp, nnml, and nnrss back ends support
non-ASCII group names[3].

Regards,

[1] gnus-group-get-new-news-this-group
[2] gnus-group-select-group with the prefix arg 0
[3] (info "(gnus) Non-ASCII Group Names")

;; I did `make bootstrap' to recompile all the Gnus modules.

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

[Eric Abrahamsen]

Katsumi Yamaoka <yamaoka@jpl.org> 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.

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

[Andy Moreton]

On Sun 24 Mar 2019, Eric Abrahamsen wrote:

> Katsumi Yamaoka <yamaoka@jpl.org> 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

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

[Eric Abrahamsen]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> On Sun 24 Mar 2019, Eric Abrahamsen wrote:
>
>> Katsumi Yamaoka <yamaoka@jpl.org> 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.

That's the conclusion I'm coming to, yes.

> 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.

Eventually I would like to reduce the number of data structures so that
groups are held in `gnus-newsrc-hashtb', and ordering is kept in
`gnus-group-list', and that's it. `gnus-newsrc-alist' would only be used
when persisting to disk. My next proposed change (once I've recovered my
confidence) is to turn groups into actual objects, in which case the
alist would really just be a kind of serialization format.

The hash table ought to be in sync with the rest of the data structure
-- if it isn't, that's another bug.

> 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.

This was an error. (Though in my case, I've had the dummy group in my
newsrc.eld for months, and it hasn't done any harm. I don't know why
it's necessary.)

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

Because the old `gnus-gethash' call returned a slice of
`gnus-newsrc-alist', where the second element was actually the group
*before* the group you wanted, and the third element was the cdr of
`gnus-newsrc-alist', starting with the group you wanted. This was
undocumented, and took a bit to figure out. Now, the gethash call just
gives you the group. Ideally, in the next set of changes, it will give
you an object.

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

My understanding is that using a prime number is significant when it
comes to vector access, but that the hash table implementation is
higher-level, where a prime number is no longer significant. If that's
incorrect I would like to know!

> 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.

In principle I agree with this completely. In practice I found it
extraordinarily difficult to touch one part of Gnus without running into
knock-on repercussions.

The ultimate goal of the changes I have in mind for Gnus is to address
exactly this: to make it more modular, to improve isolation of code
paths, and to reduce the number of semi-redundant data structures. But
the process is evidently even messier than I thought. I held back
another commit to group name encoding in an attempt to keep things
simple, but that seems to have made things even worse.

But yes, if I end up backing this change out, I'll try to break it up
into smaller commits.

> Thanks for working on gnus,

Thanks for the code review! I wish I'd gotten this to begin with.

Eric

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

[Robert Pluim]

>>>>> On Mon, 25 Mar 2019 10:35:32 -0700, Eric Abrahamsen <eric@ericabrahamsen.net> said:
    >> 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.

    Eric> That's the conclusion I'm coming to, yes.

FWIW, gnus on master has been working flawlessly for me since you
pushed the hash table changes. People running master should expect
occasional breakage, so I wouldnʼt give up too soon.

Robert

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

[Basil L. Contovounesios]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Mon, 25 Mar 2019 10:35:32 -0700, Eric Abrahamsen <eric@ericabrahamsen.net> said:
>     >> 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.
>
>     Eric> That's the conclusion I'm coming to, yes.
>
> FWIW, gnus on master has been working flawlessly for me since you
> pushed the hash table changes. People running master should expect
> occasional breakage, so I wouldnʼt give up too soon.

Seconded.

-- 
Basil

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

[Bastien]

Robert Pluim <rpluim@gmail.com> writes:

> FWIW, gnus on master has been working flawlessly for me since you
> pushed the hash table changes. People running master should expect
> occasional breakage, so I wouldnʼt give up too soon.

For something as critical as Gnus and for which many various configs
exist, I would recommend that a patch can be confirmed by *at least*
three persons on top of the contributor himself.

This is perhaps the case for this patch, I don't know.

If things are okay on your side, perhaps you don't feel the pain, but
I can tell you it's here :)

Also, the fact that Gnus has been crazily stable on the master branch
from... well, from as far as I can remember, makes the situation even
worse: you really don't expect anything to break in this area.

Not a big deal, I'll wait, and thanks again Eric for both the patches
and your patience in finding possible issues!

-- 
 Bastien

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

[Andy Moreton]

On Mon 25 Mar 2019, Eric Abrahamsen wrote:

> Andy Moreton <andrewjmoreton@gmail.com> writes:
>> 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.
>
> Eventually I would like to reduce the number of data structures so that
> groups are held in `gnus-newsrc-hashtb', and ordering is kept in
> `gnus-group-list', and that's it. `gnus-newsrc-alist' would only be used
> when persisting to disk. My next proposed change (once I've recovered my
> confidence) is to turn groups into actual objects, in which case the
> alist would really just be a kind of serialization format.

ok, but if the hash table and the alist reference the same lisp objects
then the existing setup is not so bad.

> The hash table ought to be in sync with the rest of the data structure
> -- if it isn't, that's another bug.
>
>> 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.
>
> This was an error. (Though in my case, I've had the dummy group in my
> newsrc.eld for months, and it hasn't done any harm. I don't know why
> it's necessary.)

I agree that it's harmless, it just seemed to be an unnecessary change.
I expect it is there to ensure that the alist is never empty to avoid
the need to check that everywhere.

>> 3) The format of the entries in `gnus-newsrc-hashtb' has changed,
>>    removing the second element. Why ?
>
> Because the old `gnus-gethash' call returned a slice of
> `gnus-newsrc-alist', where the second element was actually the group
> *before* the group you wanted, and the third element was the cdr of
> `gnus-newsrc-alist', starting with the group you wanted. This was
> undocumented, and took a bit to figure out. Now, the gethash call just
> gives you the group. Ideally, in the next set of changes, it will give
> you an object.

ok, so it sounds like the old code was trying hard to use the same lisp
objects in both the hash table and the alist. That avoids alloacting
twice the storage, and ensures that the hash table and the alist stay in
sync.

>> 4) You changed several hash tale sizesfrom 4096 to 4000, and 1024 to
>>    1000. Why ?
>
> My understanding is that using a prime number is significant when it
> comes to vector access, but that the hash table implementation is
> higher-level, where a prime number is no longer significant. If that's
> incorrect I would like to know!

None of these numbers are prime. The old numbers are powers of two,
which are reasonable for allocation sizes on a binary machine. Again,
not a terribly important change, but not really needed either.

>> 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.
>
> In principle I agree with this completely. In practice I found it
> extraordinarily difficult to touch one part of Gnus without running into
> knock-on repercussions.

Agreed. I find that this sort of work usually goes in two phases: some
exploratory programming to decide on the path forward and the eventual
goal, followed by rearranging the changes into a logical set of
(bisectable) patches that get to that goal in small, self-contained
steps. The second half is extra work, but worth it to make it easier for
other people to review the patches (and to simplify any archaeology
needed by a later maintainer).

If changing gnus is hard, then perhaps writing new tests to document
what you have discovered about the code will help to ensure that later
changes do not change the observed behaviour.

> The ultimate goal of the changes I have in mind for Gnus is to address
> exactly this: to make it more modular, to improve isolation of code
> paths, and to reduce the number of semi-redundant data structures. But
> the process is evidently even messier than I thought. I held back
> another commit to group name encoding in an attempt to keep things
> simple, but that seems to have made things even worse.

A worthwhile goal - please do not get discouraged.

    AndyM

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

[Andy Moreton]

On Sat 23 Mar 2019, Andy Moreton wrote:

> On Fri 22 Mar 2019, Eric Abrahamsen wrote:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>>>> Date: Fri, 22 Mar 2019 12:54:54 -0700
>>>> 
>>>> is there anything to be done besides warning people on the mailing
>>>> lists?
>>>
>>> A warning would be nice, yes.
>>
>> Okay, I'll do that. I've already gotten a bug report :(
>
> After bootstrapping, I observe that the summary buffer does not update
> the faces correctly for nntp articles that have been read: they still
> have an unread face.

After more debugging, I have found the problem.

Updating summary lines is handled in `gnus-summary-insert-line':
    (condition-case ()
	(put-text-property
	 (point)
	 (progn (eval gnus-summary-line-format-spec) (point))
	 'gnus-number gnus-tmp-number)
      (error (gnus-message 5 "Error updating the summary line")))

In my setup, `gnus-summary-line-format' used starts with "%U%R%u&score;"
to use a user format function for printing score info. That results in
calls to my user function to display summary lines:

  (defun gnus-user-format-function-score (header)
    (let* ((article (mail-header-number header))
           (thread  (gnus-id-to-thread (mail-header-id header)))
           ...

This now fails because `gnus-id-to-thread' sometimes throws an error,
because `gnus-newsgroup-dependencies' is nil when it is called. This
usage worked in emacs-26 and in master before your changes.

I've fixed this in my user format function with:

  (defun gnus-user-format-function-score (header)
    (let* ((article (mail-header-number header))
           (thread  (and gnus-newsgroup-dependencies
                         (gnus-id-to-thread (mail-header-id header))))
           ...

After that change, the marks are displayed correctly in the summary
buffer. I'm not sure if that is something that only worked by chance in
old code, or if it is a regression, but it is a change of behaviour.

    AndyM

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

[Eric Abrahamsen]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> On Sat 23 Mar 2019, Andy Moreton wrote:
>
>> On Fri 22 Mar 2019, Eric Abrahamsen wrote:
>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>>>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>>>>> Date: Fri, 22 Mar 2019 12:54:54 -0700
>>>>> 
>>>>> is there anything to be done besides warning people on the mailing
>>>>> lists?
>>>>
>>>> A warning would be nice, yes.
>>>
>>> Okay, I'll do that. I've already gotten a bug report :(
>>
>> After bootstrapping, I observe that the summary buffer does not update
>> the faces correctly for nntp articles that have been read: they still
>> have an unread face.
>
> After more debugging, I have found the problem.
>
> Updating summary lines is handled in `gnus-summary-insert-line':
>     (condition-case ()
> 	(put-text-property
> 	 (point)
> 	 (progn (eval gnus-summary-line-format-spec) (point))
> 	 'gnus-number gnus-tmp-number)
>       (error (gnus-message 5 "Error updating the summary line")))
>
> In my setup, `gnus-summary-line-format' used starts with "%U%R%u&score;"
> to use a user format function for printing score info. That results in
> calls to my user function to display summary lines:
>
>   (defun gnus-user-format-function-score (header)
>     (let* ((article (mail-header-number header))
>            (thread  (gnus-id-to-thread (mail-header-id header)))
>            ...
>
> This now fails because `gnus-id-to-thread' sometimes throws an error,
> because `gnus-newsgroup-dependencies' is nil when it is called. This
> usage worked in emacs-26 and in master before your changes.
>
> I've fixed this in my user format function with:
>
>   (defun gnus-user-format-function-score (header)
>     (let* ((article (mail-header-number header))
>            (thread  (and gnus-newsgroup-dependencies
>                          (gnus-id-to-thread (mail-header-id header))))
>            ...
>
> After that change, the marks are displayed correctly in the summary
> buffer. I'm not sure if that is something that only worked by chance in
> old code, or if it is a regression, but it is a change of behaviour.

Yes, the format spec is eval'ed twice (to set summary buffer positions)
before the threads are actually built, and `gnus-newsgroup-dependencies'
is nil at that point. Back when this was a call to `intern', it would
just silently return nil, now it errors. Your fix works, but I could
also have `gnus-id-to-thread' return nil if
`gnus-newsgroup-dependencies' is nil. That would mimic Gnus' previous
behavior.

Thanks,
Eric

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

[Eric Abrahamsen]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> On Mon 25 Mar 2019, Eric Abrahamsen wrote:
>
>> Andy Moreton <andrewjmoreton@gmail.com> writes:
>>> 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.
>>
>> Eventually I would like to reduce the number of data structures so that
>> groups are held in `gnus-newsrc-hashtb', and ordering is kept in
>> `gnus-group-list', and that's it. `gnus-newsrc-alist' would only be used
>> when persisting to disk. My next proposed change (once I've recovered my
>> confidence) is to turn groups into actual objects, in which case the
>> alist would really just be a kind of serialization format.
>
> ok, but if the hash table and the alist reference the same lisp objects
> then the existing setup is not so bad.

There's nothing wrong with the existing setup here! I'm just laying the
groundwork for the next round of changes.

>> The hash table ought to be in sync with the rest of the data structure
>> -- if it isn't, that's another bug.
>>
>>> 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.
>>
>> This was an error. (Though in my case, I've had the dummy group in my
>> newsrc.eld for months, and it hasn't done any harm. I don't know why
>> it's necessary.)
>
> I agree that it's harmless, it just seemed to be an unnecessary change.
> I expect it is there to ensure that the alist is never empty to avoid
> the need to check that everywhere.

Yes, for sure, and I didn't mean to leave it in there, that's just a bug.

>>> 3) The format of the entries in `gnus-newsrc-hashtb' has changed,
>>> removing the second element. Why?
>>
>> Because the old `gnus-gethash' call returned a slice of
>> `gnus-newsrc-alist', where the second element was actually the group
>> *before* the group you wanted, and the third element was the cdr of
>> `gnus-newsrc-alist', starting with the group you wanted. This was
>> undocumented, and took a bit to figure out. Now, the gethash call just
>> gives you the group. Ideally, in the next set of changes, it will give
>> you an object.
>
> ok, so it sounds like the old code was trying hard to use the same lisp
> objects in both the hash table and the alist. That avoids alloacting
> twice the storage, and ensures that the hash table and the alist stay in
> sync.

Yes, I'm looking forward to when the groups are actual objects, . But in
the meantime, the lisp objects are still the same, I haven't doubled
storage (at least `eq' returns t, so I assume that's what that means).

>>> 4) You changed several hash tale sizesfrom 4096 to 4000, and 1024 to
>>> 1000. Why?
>>
>> My understanding is that using a prime number is significant when it
>> comes to vector access, but that the hash table implementation is
>> higher-level, where a prime number is no longer significant. If that's
>> incorrect I would like to know!
>
> None of these numbers are prime. The old numbers are powers of two,
> which are reasonable for allocation sizes on a binary machine. Again,
> not a terribly important change, but not really needed either.

Yes, sorry, powers of two. It's true it's a do-nothing change, I guess
I've been erring on the side of trying to make the code more
self-explanatory, less "odd". Probably this change didn't need to be
made, but it seems about as un-risky as a code change could be.

>>> 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.
>>
>> In principle I agree with this completely. In practice I found it
>> extraordinarily difficult to touch one part of Gnus without running into
>> knock-on repercussions.
>
> Agreed. I find that this sort of work usually goes in two phases: some
> exploratory programming to decide on the path forward and the eventual
> goal, followed by rearranging the changes into a logical set of
> (bisectable) patches that get to that goal in small, self-contained
> steps. The second half is extra work, but worth it to make it easier for
> other people to review the patches (and to simplify any archaeology
> needed by a later maintainer).
>
> If changing gnus is hard, then perhaps writing new tests to document
> what you have discovered about the code will help to ensure that later
> changes do not change the observed behaviour.

The previous commit did add some new Gnus tests -- perhaps the first!
I'm planning more, once Gnus group are actual objects. Everything's so
interconnected now (and data-reliant) that it's very hard to write
effective tests.

>> The ultimate goal of the changes I have in mind for Gnus is to address
>> exactly this: to make it more modular, to improve isolation of code
>> paths, and to reduce the number of semi-redundant data structures. But
>> the process is evidently even messier than I thought. I held back
>> another commit to group name encoding in an attempt to keep things
>> simple, but that seems to have made things even worse.
>
> A worthwhile goal - please do not get discouraged.

Thank you,

Eric

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

[Eric Abrahamsen]

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

Katsumi Yamaoka <yamaoka@jpl.org> 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]:

Thanks for this report. When you have a moment will you please try the
attached patch and see if it fixes the problem?

Yours,
Eric


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-nnml-assoc.diff --]
[-- Type: text/x-patch, Size: 3223 bytes --]

diff --git a/lisp/gnus/nnml.el b/lisp/gnus/nnml.el
index 5770777ad4..205e9e4803 100644
--- a/lisp/gnus/nnml.el
+++ b/lisp/gnus/nnml.el
@@ -259,7 +259,7 @@ nnml-request-group
      (t
       (nnheader-re-read-dir nnml-current-directory)
       (nnmail-activate 'nnml)
-      (let ((active (nth 1 (assoc group nnml-group-alist))))
+      (let ((active (nth 1 (assoc-string group nnml-group-alist))))
 	(if (not active)
 	    (nnheader-report 'nnml "No such group: %s" decoded)
 	  (nnheader-report 'nnml "Selected group %s" decoded)
@@ -295,7 +295,7 @@ nnml-request-create-group
     (nnheader-report 'nnml "%s is a file"
 		     (directory-file-name (nnml-group-pathname group
 							       nil server))))
-   ((assoc group nnml-group-alist)
+   ((assoc-string group nnml-group-alist)
     t)
    (t
     (let (active)
@@ -379,7 +379,7 @@ nnml-request-expire-articles
 		  (nnml-nov-delete-article group number))
 	      (push number rest)))
 	(push number rest)))
-    (let ((active (nth 1 (assoc group nnml-group-alist))))
+    (let ((active (nth 1 (assoc-string group nnml-group-alist))))
       (when active
 	(setcar active (or (and active-articles
 				(apply 'min active-articles))
@@ -520,7 +520,7 @@ nnml-request-delete-group
       (nnheader-report 'nnml "No such directory: %s/" file))
     ;; Remove the group from all structures.
     (setq nnml-group-alist
-	  (delq (assoc group nnml-group-alist) nnml-group-alist)
+	  (delq (assoc-string group nnml-group-alist) nnml-group-alist)
 	  nnml-current-group nil
 	  nnml-current-directory nil)
     ;; Save the active file.
@@ -549,7 +549,7 @@ nnml-request-rename-group
       (when (<= (length (directory-files old-dir)) 2)
 	(ignore-errors (delete-directory old-dir)))
       ;; That went ok, so we change the internal structures.
-      (let ((entry (assoc group nnml-group-alist)))
+      (let ((entry (assoc-string group nnml-group-alist)))
 	(when entry
 	  (setcar entry new-name))
 	(setq nnml-current-directory nil
@@ -597,7 +597,7 @@ nnml-deletable-article-p
     (when (setq path (nnml-article-to-file article))
       (when (file-writable-p path)
 	(or (not nnmail-keep-last-article)
-	    (not (eq (cdr (nth 1 (assoc group nnml-group-alist)))
+	    (not (eq (cdr (nth 1 (assoc-string group nnml-group-alist)))
 		     article)))))))
 
 ;; Find an article number in the current group given the Message-ID.
@@ -742,7 +742,7 @@ nnml-active-number
   "Compute the next article number in GROUP on SERVER."
   (let* ((encoded (if nnmail-group-names-not-encoded-p
 		      (nnml-encoded-group-name group server)))
-	 (active (cadr (assoc (or encoded group) nnml-group-alist))))
+	 (active (cadr (assoc-string (or encoded group) nnml-group-alist))))
     ;; The group wasn't known to nnml, so we just create an active
     ;; entry for it.
     (unless active
@@ -783,7 +783,7 @@ nnml-save-incremental-nov
 	    (cdr nnml-incremental-nov-buffer-alist)))))
 
 (defun nnml-open-incremental-nov (group)
-  (or (cdr (assoc group nnml-incremental-nov-buffer-alist))
+  (or (cdr (assoc-string group nnml-incremental-nov-buffer-alist))
       (let ((buffer (nnml-get-nov-buffer group t)))
 	(push (cons group buffer) nnml-incremental-nov-buffer-alist)
 	buffer)))

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

[Katsumi Yamaoka]

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

On Tue, 26 Mar 2019 14:44:32 -0700, Eric Abrahamsen wrote:
> Thanks for this report. When you have a moment will you please try the
> attached patch and see if it fixes the problem?

Works great.  Now I can enter and read the nnml:テスト group.
Thank you.  But there seem to be some more fix needed.

The group level of nnml:テスト is 1 and there are some unread
articles in it, however the group is not listed in the Group
buffer when I launch Gnus by `C-u 1 M-x gnus RET' (it also
happened when I worked on non-ASCII group names years ago, but
I don't recall how I fixed it, sorry).  Typing L helps it.

I tried changing `assoc' with `assoc-string' in nnrss.el, too.
It works for non-ASCII group names so far, though only I glanced
a few feeds.  A diff is attached.

Regards,


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

--- nnrss.el~	2019-02-20 21:21:21.909516900 +0000
+++ nnrss.el	2019-03-27 04:49:26.756386000 +0000
@@ -340,10 +340,10 @@
   (let (elem)
     ;; There may be two or more entries in `nnrss-group-alist' since
     ;; this function didn't delete them formerly.
-    (while (setq elem (assoc group nnrss-group-alist))
+    (while (setq elem (assoc-string group nnrss-group-alist))
       (setq nnrss-group-alist (delq elem nnrss-group-alist))))
   (setq nnrss-server-data
-	(delq (assoc group nnrss-server-data) nnrss-server-data))
+	(delq (assoc-string group nnrss-server-data) nnrss-server-data))
   (nnrss-save-server-data server)
   (ignore-errors
     (let ((file-name-coding-system nnmail-pathname-coding-system))
@@ -367,7 +367,7 @@
   (with-current-buffer nntp-server-buffer
     (erase-buffer)
     (dolist (group groups)
-      (let ((elem (assoc (gnus-group-decoded-name group) nnrss-server-data)))
+      (let ((elem (assoc-string (gnus-group-decoded-name group) nnrss-server-data)))
 	(insert (format "%S %s 1 y\n" group (or (cadr elem) 0)))))
     'active))
 
@@ -539,7 +539,7 @@
   (if (hash-table-p nnrss-group-hashtb)
       (clrhash nnrss-group-hashtb)
     (setq nnrss-group-hashtb (make-hash-table :test 'equal)))
-  (let ((pair (assoc group nnrss-server-data)))
+  (let ((pair (assoc-string group nnrss-server-data)))
     (setq nnrss-group-max (or (cadr pair) 0))
     (setq nnrss-group-min (+ nnrss-group-max 1)))
   (let ((file (nnrss-make-filename group server))
@@ -644,8 +644,8 @@
 					 (concat group ".xml"))
 					nnrss-directory))))
 	(setq xml (nnrss-fetch file t))
-      (setq url (or (nth 2 (assoc group nnrss-server-data))
-		    (cadr (assoc group nnrss-group-alist))))
+      (setq url (or (nth 2 (assoc-string group nnrss-server-data))
+		    (cadr (assoc-string group nnrss-group-alist))))
       (unless url
 	(setq url
 	      (cdr
@@ -653,7 +653,7 @@
 		      (nnrss-discover-feed
 		       (read-string
 			(format "URL to search for %s: " group) "http://")))))
-	(let ((pair (assoc group nnrss-server-data)))
+	(let ((pair (assoc-string group nnrss-server-data)))
 	  (if pair
 	      (setcdr (cdr pair) (list url))
 	    (push (list group nnrss-group-max url) nnrss-server-data)))
@@ -721,7 +721,7 @@
       (setq extra nil))
     (when changed
       (nnrss-save-group-data group server)
-      (let ((pair (assoc group nnrss-server-data)))
+      (let ((pair (assoc-string group nnrss-server-data)))
 	(if pair
 	    (setcar (cdr pair) nnrss-group-max)
 	  (push (list group nnrss-group-max) nnrss-server-data)))
@@ -792,7 +792,7 @@
   (insert "RSSDIR='" (expand-file-name nnrss-directory) "'\n")
   (dolist (elem nnrss-server-data)
     (let ((url (or (nth 2 elem)
-		   (cadr (assoc (car elem) nnrss-group-alist)))))
+		   (cadr (assoc-string (car elem) nnrss-group-alist)))))
       (insert "$WGET -q -O \"$RSSDIR\"/'"
 	      (nnrss-translate-file-chars (concat (car elem) ".xml"))
 	      "' '" url "'\n"))))

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

[Eric Abrahamsen]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

> On Tue, 26 Mar 2019 14:44:32 -0700, Eric Abrahamsen wrote:
>> Thanks for this report. When you have a moment will you please try the
>> attached patch and see if it fixes the problem?
>
> Works great.  Now I can enter and read the nnml:テスト group.
> Thank you.  But there seem to be some more fix needed.
>
> The group level of nnml:テスト is 1 and there are some unread
> articles in it, however the group is not listed in the Group
> buffer when I launch Gnus by `C-u 1 M-x gnus RET' (it also
> happened when I worked on non-ASCII group names years ago, but
> I don't recall how I fixed it, sorry).  Typing L helps it.

Excellent -- I know where to go from here (I've also run into this
before). I hope I've nearly got this sorted.

> I tried changing `assoc' with `assoc-string' in nnrss.el, too.
> It works for non-ASCII group names so far, though only I glanced
> a few feeds.  A diff is attached.

Thanks!
Eric

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

[Eric Abrahamsen]

On 03/27/19 13:54 PM, Katsumi Yamaoka wrote:
> On Tue, 26 Mar 2019 14:44:32 -0700, Eric Abrahamsen wrote:
>> Thanks for this report. When you have a moment will you please try the
>> attached patch and see if it fixes the problem?
>
> Works great.  Now I can enter and read the nnml:テスト group.
> Thank you.  But there seem to be some more fix needed.
>
> The group level of nnml:テスト is 1 and there are some unread
> articles in it, however the group is not listed in the Group
> buffer when I launch Gnus by `C-u 1 M-x gnus RET' (it also
> happened when I worked on non-ASCII group names years ago, but
> I don't recall how I fixed it, sorry).  Typing L helps it.

I think I've found the source of the problem. The error you and others
are seeing comes when the group has an entry in gnus-newsrc-hashtb (ie,
you're subscribed to the group) but not in gnus-active-hashtb (Gnus
can't tell if the group has articles).

Gnus writes "nnml:テスト" to .newsrc.eld as
"nnml:\343\203\206\343\202\271\343\203\210", and that's used as the key
in gnus-newsrc-hashtb. But when it reads the active info from nnml, the
(unibyte) buffer contains:

nnml:\343\203\206\343\202\271\343\203\210

As a symbol, not a string. My current code then uses `symbol-name' to
turn it into a string, but it's crucially *not the same string* as the
one saved to.newsrc.eld, even though it looks the same in edebug, the
*scratch* buffer, etc.

Hence a mismatch between the two keys in the two hash-tables, and things
going haywire.

Obviously this arises from a misunderstanding on my part about how
bytes and strings interact. The code in question is at line 2145 in
gnus-start.el -- it uses `read' to pick pieces out of the buffer.

Right now my best idea is to swap out `read' for a some sort of regexp
crawl, but I am open to suggestions.

Thanks,
Eric

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

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> On 03/27/19 13:54 PM, Katsumi Yamaoka wrote:
>> On Tue, 26 Mar 2019 14:44:32 -0700, Eric Abrahamsen wrote:
>>> Thanks for this report. When you have a moment will you please try the
>>> attached patch and see if it fixes the problem?
>>
>> Works great.  Now I can enter and read the nnml:テスト group.
>> Thank you.  But there seem to be some more fix needed.
>>
>> The group level of nnml:テスト is 1 and there are some unread
>> articles in it, however the group is not listed in the Group
>> buffer when I launch Gnus by `C-u 1 M-x gnus RET' (it also
>> happened when I worked on non-ASCII group names years ago, but
>> I don't recall how I fixed it, sorry).  Typing L helps it.
>
> I think I've found the source of the problem. The error you and others
> are seeing comes when the group has an entry in gnus-newsrc-hashtb (ie,
> you're subscribed to the group) but not in gnus-active-hashtb (Gnus
> can't tell if the group has articles).
>
> Gnus writes "nnml:テスト" to .newsrc.eld as
> "nnml:\343\203\206\343\202\271\343\203\210", and that's used as the key
> in gnus-newsrc-hashtb. But when it reads the active info from nnml, the
> (unibyte) buffer contains:
>
> nnml:\343\203\206\343\202\271\343\203\210
>
> As a symbol, not a string.

Sorry, as raw bytes I guess. But `read' interprets it as a symbol.

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

[Deus Max]

Hi Eric,

It seems you are implementing some pretty drastic upgrades to Gnus.

Is there a write-up on the planned changes, benefits, what is going to be
affected and what to expect ?

I'm interested and would like to read it.



On Sun, Mar 24 2019, Eric Abrahamsen wrote:

> On 03/24/19 23:29 PM, Bastien wrote:
>> Hi Eric,
>>
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> The bummer is that you don't get an error until you run Gnus, so it just
>>> looks like I broke Gnus :(
>>
>> I hope not!  :)
>>
>> I have not followed this change.
>> Can it corrupt the .newsrc.eld file?
>
> Worst come to worst it will write non-ascii group names as unibyte when
> they should be utf-8, or else the other way around. I thought I could
> avoid dealing with this question for now, and just preserve Gnus'
> original method of saving group names, but it looks like I may have
> forced the issue instead...

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

[Eric Abrahamsen]

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


On 03/27/19 14:27 PM, Eric Abrahamsen wrote:
> On 03/27/19 13:54 PM, Katsumi Yamaoka wrote:
>> On Tue, 26 Mar 2019 14:44:32 -0700, Eric Abrahamsen wrote:
>>> Thanks for this report. When you have a moment will you please try the
>>> attached patch and see if it fixes the problem?
>>
>> Works great.  Now I can enter and read the nnml:テスト group.
>> Thank you.  But there seem to be some more fix needed.
>>
>> The group level of nnml:テスト is 1 and there are some unread
>> articles in it, however the group is not listed in the Group
>> buffer when I launch Gnus by `C-u 1 M-x gnus RET' (it also
>> happened when I worked on non-ASCII group names years ago, but
>> I don't recall how I fixed it, sorry).  Typing L helps it.
>
> I think I've found the source of the problem. The error you and others
> are seeing comes when the group has an entry in gnus-newsrc-hashtb (ie,
> you're subscribed to the group) but not in gnus-active-hashtb (Gnus
> can't tell if the group has articles).

Okay, it might be as simple as the attached patch, applied to current
master. Basically this forces nnmail to always write the group names as
strings, which are then read correctly later on.

Also cc'ing Bastien and Adam, who have been very helpful with
road-testing.

Thanks to all,
Eric


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-on-nnmail-group-names.patch --]
[-- Type: text/x-patch, Size: 7262 bytes --]

From 05c5d299fa4342cce6c4921b5c5387d6be07c89a Mon Sep 17 00:00:00 2001
From: Eric Abrahamsen <eric@ericabrahamsen.net>
Date: Sun, 31 Mar 2019 09:09:18 -0700
Subject: [PATCH 1/2] WIP on nnmail group names

---
 lisp/gnus/nnmail.el |  2 +-
 lisp/gnus/nnml.el   | 16 ++++++++--------
 lisp/gnus/nnrss.el  | 18 +++++++++---------
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/lisp/gnus/nnmail.el b/lisp/gnus/nnmail.el
index a95cdb4a4f..a84830b48d 100644
--- a/lisp/gnus/nnmail.el
+++ b/lisp/gnus/nnmail.el
@@ -691,7 +691,7 @@ nnmail-generate-active
   (erase-buffer)
   (let (group)
     (while (setq group (pop alist))
-      (insert (format "%S %d %d y\n" (intern (car group)) (cdadr group)
+      (insert (format "\"%s\" %d %d y\n" (car group) (cdadr group)
 		      (caadr group))))
     (goto-char (point-max))
     (while (search-backward "\\." nil t)
diff --git a/lisp/gnus/nnml.el b/lisp/gnus/nnml.el
index 5770777ad4..205e9e4803 100644
--- a/lisp/gnus/nnml.el
+++ b/lisp/gnus/nnml.el
@@ -259,7 +259,7 @@ nnml-request-group
      (t
       (nnheader-re-read-dir nnml-current-directory)
       (nnmail-activate 'nnml)
-      (let ((active (nth 1 (assoc group nnml-group-alist))))
+      (let ((active (nth 1 (assoc-string group nnml-group-alist))))
 	(if (not active)
 	    (nnheader-report 'nnml "No such group: %s" decoded)
 	  (nnheader-report 'nnml "Selected group %s" decoded)
@@ -295,7 +295,7 @@ nnml-request-create-group
     (nnheader-report 'nnml "%s is a file"
 		     (directory-file-name (nnml-group-pathname group
 							       nil server))))
-   ((assoc group nnml-group-alist)
+   ((assoc-string group nnml-group-alist)
     t)
    (t
     (let (active)
@@ -379,7 +379,7 @@ nnml-request-expire-articles
 		  (nnml-nov-delete-article group number))
 	      (push number rest)))
 	(push number rest)))
-    (let ((active (nth 1 (assoc group nnml-group-alist))))
+    (let ((active (nth 1 (assoc-string group nnml-group-alist))))
       (when active
 	(setcar active (or (and active-articles
 				(apply 'min active-articles))
@@ -520,7 +520,7 @@ nnml-request-delete-group
       (nnheader-report 'nnml "No such directory: %s/" file))
     ;; Remove the group from all structures.
     (setq nnml-group-alist
-	  (delq (assoc group nnml-group-alist) nnml-group-alist)
+	  (delq (assoc-string group nnml-group-alist) nnml-group-alist)
 	  nnml-current-group nil
 	  nnml-current-directory nil)
     ;; Save the active file.
@@ -549,7 +549,7 @@ nnml-request-rename-group
       (when (<= (length (directory-files old-dir)) 2)
 	(ignore-errors (delete-directory old-dir)))
       ;; That went ok, so we change the internal structures.
-      (let ((entry (assoc group nnml-group-alist)))
+      (let ((entry (assoc-string group nnml-group-alist)))
 	(when entry
 	  (setcar entry new-name))
 	(setq nnml-current-directory nil
@@ -597,7 +597,7 @@ nnml-deletable-article-p
     (when (setq path (nnml-article-to-file article))
       (when (file-writable-p path)
 	(or (not nnmail-keep-last-article)
-	    (not (eq (cdr (nth 1 (assoc group nnml-group-alist)))
+	    (not (eq (cdr (nth 1 (assoc-string group nnml-group-alist)))
 		     article)))))))
 
 ;; Find an article number in the current group given the Message-ID.
@@ -742,7 +742,7 @@ nnml-active-number
   "Compute the next article number in GROUP on SERVER."
   (let* ((encoded (if nnmail-group-names-not-encoded-p
 		      (nnml-encoded-group-name group server)))
-	 (active (cadr (assoc (or encoded group) nnml-group-alist))))
+	 (active (cadr (assoc-string (or encoded group) nnml-group-alist))))
     ;; The group wasn't known to nnml, so we just create an active
     ;; entry for it.
     (unless active
@@ -783,7 +783,7 @@ nnml-save-incremental-nov
 	    (cdr nnml-incremental-nov-buffer-alist)))))
 
 (defun nnml-open-incremental-nov (group)
-  (or (cdr (assoc group nnml-incremental-nov-buffer-alist))
+  (or (cdr (assoc-string group nnml-incremental-nov-buffer-alist))
       (let ((buffer (nnml-get-nov-buffer group t)))
 	(push (cons group buffer) nnml-incremental-nov-buffer-alist)
 	buffer)))
diff --git a/lisp/gnus/nnrss.el b/lisp/gnus/nnrss.el
index 7f2accc2b6..0bfecb28e0 100644
--- a/lisp/gnus/nnrss.el
+++ b/lisp/gnus/nnrss.el
@@ -340,10 +340,10 @@ nnrss-request-delete-group
   (let (elem)
     ;; There may be two or more entries in `nnrss-group-alist' since
     ;; this function didn't delete them formerly.
-    (while (setq elem (assoc group nnrss-group-alist))
+    (while (setq elem (assoc-string group nnrss-group-alist))
       (setq nnrss-group-alist (delq elem nnrss-group-alist))))
   (setq nnrss-server-data
-	(delq (assoc group nnrss-server-data) nnrss-server-data))
+	(delq (assoc-string group nnrss-server-data) nnrss-server-data))
   (nnrss-save-server-data server)
   (ignore-errors
     (let ((file-name-coding-system nnmail-pathname-coding-system))
@@ -367,7 +367,7 @@ nnrss-retrieve-groups
   (with-current-buffer nntp-server-buffer
     (erase-buffer)
     (dolist (group groups)
-      (let ((elem (assoc (gnus-group-decoded-name group) nnrss-server-data)))
+      (let ((elem (assoc-string (gnus-group-decoded-name group) nnrss-server-data)))
 	(insert (format "%S %s 1 y\n" group (or (cadr elem) 0)))))
     'active))
 
@@ -539,7 +539,7 @@ nnrss-read-group-data
   (if (hash-table-p nnrss-group-hashtb)
       (clrhash nnrss-group-hashtb)
     (setq nnrss-group-hashtb (make-hash-table :test 'equal)))
-  (let ((pair (assoc group nnrss-server-data)))
+  (let ((pair (assoc-string group nnrss-server-data)))
     (setq nnrss-group-max (or (cadr pair) 0))
     (setq nnrss-group-min (+ nnrss-group-max 1)))
   (let ((file (nnrss-make-filename group server))
@@ -644,8 +644,8 @@ nnrss-check-group
 					 (concat group ".xml"))
 					nnrss-directory))))
 	(setq xml (nnrss-fetch file t))
-      (setq url (or (nth 2 (assoc group nnrss-server-data))
-		    (cadr (assoc group nnrss-group-alist))))
+      (setq url (or (nth 2 (assoc-string group nnrss-server-data))
+		    (cadr (assoc-string group nnrss-group-alist))))
       (unless url
 	(setq url
 	      (cdr
@@ -653,7 +653,7 @@ nnrss-check-group
 		      (nnrss-discover-feed
 		       (read-string
 			(format "URL to search for %s: " group) "http://")))))
-	(let ((pair (assoc group nnrss-server-data)))
+	(let ((pair (assoc-string group nnrss-server-data)))
 	  (if pair
 	      (setcdr (cdr pair) (list url))
 	    (push (list group nnrss-group-max url) nnrss-server-data)))
@@ -721,7 +721,7 @@ nnrss-check-group
       (setq extra nil))
     (when changed
       (nnrss-save-group-data group server)
-      (let ((pair (assoc group nnrss-server-data)))
+      (let ((pair (assoc-string group nnrss-server-data)))
 	(if pair
 	    (setcar (cdr pair) nnrss-group-max)
 	  (push (list group nnrss-group-max) nnrss-server-data)))
@@ -792,7 +792,7 @@ nnrss-generate-download-script
   (insert "RSSDIR='" (expand-file-name nnrss-directory) "'\n")
   (dolist (elem nnrss-server-data)
     (let ((url (or (nth 2 elem)
-		   (cadr (assoc (car elem) nnrss-group-alist)))))
+		   (cadr (assoc-string (car elem) nnrss-group-alist)))))
       (insert "$WGET -q -O \"$RSSDIR\"/'"
 	      (nnrss-translate-file-chars (concat (car elem) ".xml"))
 	      "' '" url "'\n"))))
-- 
2.21.0

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

[Eric Abrahamsen]

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


On 03/30/19 14:09 PM, Deus Max wrote:
> Hi Eric,
>
> It seems you are implementing some pretty drastic upgrades to Gnus.
>
> Is there a write-up on the planned changes, benefits, what is going to be
> affected and what to expect ?
>
> I'm interested and would like to read it.

There is a writeup! Sort of -- I called it a roadmap. Originally I stuck
it in a branch called scratch/gnus-roadmap but soon realized it didn't
make any sense to keep track of something like this in a git branch.
I've attached the current version.

Obviously, this is _not_ meant as a list of "changes I will unilaterally
implement in Gnus", but as a document for record-keeping and discussion.
I didn't get much discussion, but I know that's to be expected!

Eric


[-- Attachment #2: roadmap.org --]
[-- Type: application/vnd.lotus-organizer, Size: 13719 bytes --]

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

[Adam Sjøgren]

Eric writes:

> Okay, it might be as simple as the attached patch, applied to current
> master. Basically this forces nnmail to always write the group names as
> strings, which are then read correctly later on.
>
> Also cc'ing Bastien and Adam, who have been very helpful with
> road-testing.

I'm running with this patch now.

I started by taking a copy of my .newsrc.eld, so I could return to it.

I swear the first time I started Gnus from master with the patch, my
groups with æøå showed up as previously. But then I stopped Emacs and
copied the .newsrc.eld back, and somehow they did not show up again.

It makes no sense to me, unless there is state somewhere besides
.newsrc.eld that I don't know of.

Since then Gnus from Emacs master with the patch has behaved as
expected.


  Slighly confused, but happy that it works,

    Adam

-- 
 "Everything needs to change.                                 Adam Sjøgren
  And it has to start today."                            asjo@koldfront.dk

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

[Eric Abrahamsen]

Adam Sjøgren <asjo@koldfront.dk> writes:

> Eric writes:
>
>> Okay, it might be as simple as the attached patch, applied to current
>> master. Basically this forces nnmail to always write the group names as
>> strings, which are then read correctly later on.
>>
>> Also cc'ing Bastien and Adam, who have been very helpful with
>> road-testing.
>
> I'm running with this patch now.
>
> I started by taking a copy of my .newsrc.eld, so I could return to it.
>
> I swear the first time I started Gnus from master with the patch, my
> groups with æøå showed up as previously. But then I stopped Emacs and
> copied the .newsrc.eld back, and somehow they did not show up again.
>
> It makes no sense to me, unless there is state somewhere besides
> .newsrc.eld that I don't know of.
>
> Since then Gnus from Emacs master with the patch has behaved as
> expected.
>
>
>   Slighly confused, but happy that it works,

Thank you for testing. This whole process has made me suspect my sanity
(or the interference of cosmic rays), so I'm kind of glad you're
confused, too :)

I am even now able to reproduce bugs *once* (for instance the "j" bug
Lars reported yesterday) in my testing setup (the gnus-mock package),
but not again thereafter. If I restart gnus-mock, I can reproduce it
once again, but only once.

I think once I eventually figure out why, the problem will be solved!

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

[Deus Max]

On Sun, Mar 31 2019, Eric Abrahamsen wrote:

> On 03/30/19 14:09 PM, Deus Max wrote:
>> Hi Eric,
>>
>> It seems you are implementing some pretty drastic upgrades to Gnus.
>>
>> Is there a write-up on the planned changes, benefits, what is going to be
>> affected and what to expect ?
>>
>> I'm interested and would like to read it.
>
> There is a writeup! Sort of -- I called it a roadmap. Originally I stuck
> it in a branch called scratch/gnus-roadmap but soon realized it didn't
> make any sense to keep track of something like this in a git branch.
> I've attached the current version.
>
> Obviously, this is _not_ meant as a list of "changes I will unilaterally
> implement in Gnus", but as a document for record-keeping and discussion.
> I didn't get much discussion, but I know that's to be expected!
>
> Eric

Hi Eric,

A roadmap, yes! that's the word I was looking for.

Thank you, very interesting.
I always wondered why there are multiple select method variables (as
~gnus-secondary-select-methods~), instead of one ~gnus-select-methods~.

I see there are plenty of things to simplify, such as group
reading/selection. Also i think, jumping to an article should be easier
and more stable.

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

[Eric Abrahamsen]

On 04/02/19 01:39 AM, Deus Max wrote:
> On Sun, Mar 31 2019, Eric Abrahamsen wrote:
>
>> On 03/30/19 14:09 PM, Deus Max wrote:
>>> Hi Eric,
>>>
>>> It seems you are implementing some pretty drastic upgrades to Gnus.
>>>
>>> Is there a write-up on the planned changes, benefits, what is going to be
>>> affected and what to expect ?
>>>
>>> I'm interested and would like to read it.
>>
>> There is a writeup! Sort of -- I called it a roadmap. Originally I stuck
>> it in a branch called scratch/gnus-roadmap but soon realized it didn't
>> make any sense to keep track of something like this in a git branch.
>> I've attached the current version.
>>
>> Obviously, this is _not_ meant as a list of "changes I will unilaterally
>> implement in Gnus", but as a document for record-keeping and discussion.
>> I didn't get much discussion, but I know that's to be expected!
>>
>> Eric
>
> Hi Eric,
>
> A roadmap, yes! that's the word I was looking for.
>
> Thank you, very interesting.
> I always wondered why there are multiple select method variables (as
> ~gnus-secondary-select-methods~), instead of one ~gnus-select-methods~.

One thing that makes sense is distinguishing between servers you've
defined in your gnus.el file, versus servers you've added via the
*Server* buffer in Gnus. As things are set up currently, those do need
to be held in different variables. Perhaps that might change in the
future, but this much makes sense.

What makes less sense to me is the distinction between the primary
select method and the secondary select methods. So far as I can tell,
the only practical difference between the two is that groups on the
primary select method don't have the method name prefixed to the group
name. That's something that could be profitably simplified, I think
(there's a ton of code that just checks whether the current method is
primary or secondary).

> I see there are plenty of things to simplify, such as group
> reading/selection. Also i think, jumping to an article should be easier
> and more stable.

Noted! Please do let me know if you have any more ideas or responses to
that document.

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

[Adam Sjøgren]

Eric writes:

> Thank you for testing. This whole process has made me suspect my sanity
> (or the interference of cosmic rays), so I'm kind of glad you're
> confused, too :)

;-)

> I am even now able to reproduce bugs *once* (for instance the "j" bug
> Lars reported yesterday) in my testing setup (the gnus-mock package),
> but not again thereafter. If I restart gnus-mock, I can reproduce it
> once again, but only once.

I was a little too quick to declare victory. While I don't see any
duplicates, nor any problems with the nnml-groups, I still see the
nntp-groups with ø and å in the name show up with a * in front.

If I enter the group, exit it again, the * changes to 0, and if I then
fetch new news, it doesn't show up. Until I start Gnus the next time.
Then it's back with *.

> I think once I eventually figure out why, the problem will be solved!

I think that is very likely!


  Best regards,

    Adam

-- 
 "Everything needs to change.                                 Adam Sjøgren
  And it has to start today."                            asjo@koldfront.dk

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

[Katsumi Yamaoka]

One more regression.

Performing `B <DEL>' or `B m' in the "nnml:inbox" group, i.e.,
that of an ASCII name, does not update the NOV database; in the
summary buffer the face for deleted or moved article lines get
changed so as to be absent at that time, however they revive
when re-entering to the group's summary.
`nnml-generate-nov-databases-directory' helps (marks survives).

I'd like to have time to fix it and others from the next week,
hopefully.

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

[Eric Abrahamsen]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

> One more regression.
>
> Performing `B <DEL>' or `B m' in the "nnml:inbox" group, i.e.,
> that of an ASCII name, does not update the NOV database; in the
> summary buffer the face for deleted or moved article lines get
> changed so as to be absent at that time, however they revive
> when re-entering to the group's summary.
> `nnml-generate-nov-databases-directory' helps (marks survives).
>
> I'd like to have time to fix it and others from the next week,
> hopefully.

Thanks for your continued reports! Was this with the most recent WIP
commit I posted a day or so ago, or with origin/master? I'm working on
this right now, and hope to have an interim fix today.

If you do think you'll have a bit of time to put into this, what would
be _really_ awesome is if you'd help me check over the next step, which
will be keeping group names decoded everywhere inside Gnus. That ought
to simplify this whole process, and avoid similar types of bugs in the
future. That was the original point of this whole patch series; I think
my mistake was breaking the series up at the wrong point.

Anyway, I will solve as much as I can in the next week. It would be
really great to have experienced eyes check my work, though.

Thanks,
Eric

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

[Katsumi Yamaoka]

On Wed, 03 Apr 2019 15:36:58 -0700, Eric Abrahamsen wrote:
> Thanks for your continued reports! Was this with the most recent WIP
> commit I posted a day or so ago, or with origin/master? I'm working on
> this right now, and hope to have an interim fix today.

It's origin/master, I've been installing it almost everyday of
weekday. ;)

I found a cause of the `move article' not to work and pushed a
fix to master.  That is having changed `while loop' to `dolist'
in `gnus-summary-move-article'.  Because of this, the 5th arg
`last' passed to `gnus-request-move-article' gets always nil,
and that function never saves NOV and also ACTIVE at least for
an nnml group.

This is a good lesson to me; changing `while loop' to `dolist'
should be done carefully.

Regards,

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

[Katsumi Yamaoka]

One more regression. :)

In the group buffer the `j' command (`gnus-group-jump-to-group')
does not work for a group that is not activated yet.  The point
goes to the top.  Such a group will appear with the `*' mark by
the `l' or the `L' command like the followig:

       *: nntp+gmane:gmane.emacs.devel

Similarly the "#" mark put in front of a marked such group will
not disappear after processing a certain job (normally
`gnus-group-get-new-news-this-group' on me) as follows:

  #    *: nntp+gmane:gmane.emacs.devel
  ↓↓↓
  #  168: nntp+gmane:gmane.emacs.devel

The cause is the same; such non-activated groups are not listed
in `gnus-active-hashtb':

(gethash "nntp+gmane:gmane.emacs.devel" gnus-active-hashtb)
 => nil

But in Gnus bundled with Emacs 26.1.92 it returns a group name
symbol, though its value (i.e., active) is nil:

(gnus-intern-safe "nntp+gmane:gmane.emacs.devel" gnus-active-hashtb)
 => nntp+gmane:gmane\.emacs\.devel
(symbol-value
 (gnus-intern-safe "nntp+gmane:gmane.emacs.devel" gnus-active-hashtb))
 => nil

In my Gnus, there are many non-activated groups at just after
lauching Gnus with the prefix arg `1', because I set the group
level `1' to the groups normally used and larger level to the
others.  In the starting time of work, my habit is to check
mails in the active groups for a while and then activate and
check groups of larger level.

Running `gnus-group-get-new-news-this-group' again on a group in
question helps (the "#" mark disappears) but I wish it could work
as before.  How about making the value for an inactive group in
hashtb to `t' or something, that means the group exists but not
activated?

Regards,

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

[Basil L. Contovounesios]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

> I found a cause of the `move article' not to work and pushed a
> fix to master.  That is having changed `while loop' to `dolist'
> in `gnus-summary-move-article'.  Because of this, the 5th arg
> `last' passed to `gnus-request-move-article' gets always nil,
> and that function never saves NOV and also ACTIVE at least for
> an nnml group.

Oops, mea culpa.

> This is a good lesson to me; changing `while loop' to `dolist'
> should be done carefully.

I tried to be careful, but clearly wasn't careful enough.

Thanks for fixing this, and sorry for the trouble.

-- 
Basil

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

[Basil L. Contovounesios]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

> One more regression. :)
>
> In the group buffer the `j' command (`gnus-group-jump-to-group')
> does not work for a group that is not activated yet.  The point
> goes to the top.  Such a group will appear with the `*' mark by
> the `l' or the `L' command like the followig:
>
>        *: nntp+gmane:gmane.emacs.devel
>
> Similarly the "#" mark put in front of a marked such group will
> not disappear after processing a certain job (normally
> `gnus-group-get-new-news-this-group' on me) as follows:
>
>   #    *: nntp+gmane:gmane.emacs.devel
>   ↓↓↓
>   #  168: nntp+gmane:gmane.emacs.devel
>
> The cause is the same; such non-activated groups are not listed
> in `gnus-active-hashtb':
>
> (gethash "nntp+gmane:gmane.emacs.devel" gnus-active-hashtb)
>  => nil
>
> But in Gnus bundled with Emacs 26.1.92 it returns a group name
> symbol, though its value (i.e., active) is nil:
>
> (gnus-intern-safe "nntp+gmane:gmane.emacs.devel" gnus-active-hashtb)
>  => nntp+gmane:gmane\.emacs\.devel
> (symbol-value
>  (gnus-intern-safe "nntp+gmane:gmane.emacs.devel" gnus-active-hashtb))
>  => nil
>
> In my Gnus, there are many non-activated groups at just after
> lauching Gnus with the prefix arg `1', because I set the group
> level `1' to the groups normally used and larger level to the
> others.  In the starting time of work, my habit is to check
> mails in the active groups for a while and then activate and
> check groups of larger level.
>
> Running `gnus-group-get-new-news-this-group' again on a group in
> question helps (the "#" mark disappears) but I wish it could work
> as before.  How about making the value for an inactive group in
> hashtb to `t' or something, that means the group exists but not
> activated?

Would this fix bug#35070?  I.e. are you describing the same bug?

Thanks,

-- 
Basil

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

[Eric Abrahamsen]

On 04/05/19 15:44 PM, Katsumi Yamaoka wrote:
> One more regression. :)
>
> In the group buffer the `j' command (`gnus-group-jump-to-group')
> does not work for a group that is not activated yet.  The point
> goes to the top.  Such a group will appear with the `*' mark by
> the `l' or the `L' command like the followig:
>
>        *: nntp+gmane:gmane.emacs.devel
>
> Similarly the "#" mark put in front of a marked such group will
> not disappear after processing a certain job (normally
> `gnus-group-get-new-news-this-group' on me) as follows:
>
>   #    *: nntp+gmane:gmane.emacs.devel
>   ↓↓↓
>   #  168: nntp+gmane:gmane.emacs.devel
>
> The cause is the same; such non-activated groups are not listed
> in `gnus-active-hashtb':
>
> (gethash "nntp+gmane:gmane.emacs.devel" gnus-active-hashtb)
>  => nil
>
> But in Gnus bundled with Emacs 26.1.92 it returns a group name
> symbol, though its value (i.e., active) is nil:
>
> (gnus-intern-safe "nntp+gmane:gmane.emacs.devel" gnus-active-hashtb)
>  => nntp+gmane:gmane\.emacs\.devel
> (symbol-value
>  (gnus-intern-safe "nntp+gmane:gmane.emacs.devel" gnus-active-hashtb))
>  => nil
>
> In my Gnus, there are many non-activated groups at just after
> lauching Gnus with the prefix arg `1', because I set the group
> level `1' to the groups normally used and larger level to the
> others.  In the starting time of work, my habit is to check
> mails in the active groups for a while and then activate and
> check groups of larger level.
>
> Running `gnus-group-get-new-news-this-group' again on a group in
> question helps (the "#" mark disappears) but I wish it could work
> as before.  How about making the value for an inactive group in
> hashtb to `t' or something, that means the group exists but not
> activated?

I think the right solution might be to check

(member group-name (hash-table-keys gnus-active-hashtb))

Or else use the DFLT argument to `gethash' together with a
`make-symbol', to see if "nil" means "no such key", or if it means "key
with nil value".

Eric

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

[Katsumi Yamaoka]

On Fri, 05 Apr 2019 12:02:32 +0100, Basil L. Contovounesios wrote:
> Would this fix bug#35070?  I.e. are you describing the same bug?

Well, what I can reproduce doesn't seem to be exactly the same
with Lars' though, I guess the root cause would probably be:

(gethash "inactive-or-non-existent-group" gnus-active-hashtb)

returns nil.  Maybe "undisplayed group" that Lars says means a
group that is inactive but in existence.  Performing Lars'
recipe in my Gnus produces things as follows:

(gnus-group-jump-to-group "inactive-group")
  ↓
  (gnus-group-goto-group "inactive-group")
    ↓
    (let ((active (gethash "inactive-group" gnus-active-hashtb)))
      ↓
      (gnus-text-property-search 'gnus-group active nil 'goto)

Where `active' is nil but `gnus-text-property-search' finds it
at (point-min) and goes to there.  Though the "inactive-group"
does not appear there.  But if forcing `active' to be an
inactive group name in the above flow, it appears at, as Lars
says, "either at the start of the buffer (before all topics) or
at the start of one of the topics it doesn't belong in" as

       *: nnfoo:bar

and the point is positioned there.

Regards,

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

[Katsumi Yamaoka]

On Fri, 05 Apr 2019 13:18:47 -0700, Eric Abrahamsen wrote:
> I think the right solution might be to check

> (member group-name (hash-table-keys gnus-active-hashtb))

It doesn't help because it returns nil for a non-activated (but
existing) group name.  Probably the solution would be to make
hashtb list all the groups existing in `gnus-newsrc-alist' at
the startup.  It will take time but is necessary, I think.

> Or else use the DFLT argument to `gethash' together with a
> `make-symbol', to see if "nil" means "no such key", or if it means "key
> with nil value".

Doesn't it make any group valid no matter what is really exists
or not?

Regards,

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

[Katsumi Yamaoka]

On Mon, 08 Apr 2019 10:58:30 +0900, Katsumi Yamaoka wrote:
> Probably the solution would be to make hashtb list all
> the groups existing in `gnus-newsrc-alist' at the startup.

I found `gnus-newsrc-hashtb' has all the group names that
`gnus-newsrc-alist' holds.  So, I'm not quite sure since I have
not tested it thoroughly yet though, the problem I brought up
--- j doesn't work for groups having not activated --- can be
fixed with:

--- gnus-group.el~	2019-04-01 21:28:54.404887500 +0000
+++ gnus-group.el	2019-04-08 04:19:19.117882300 +0000
@@ -2562,3 +2562,3 @@
     (let ((start (point))
-	  (active (and (gethash group gnus-active-hashtb)
+	  (active (and (gethash group gnus-newsrc-hashtb)
 		       group)))

;; The variable name `active' seems to be better to change to
;; `name' or something since it is a group name string, not
;; active data here.

This looks to fix Lars' bug#35070 as well.  I'll check and check
it with investigating other issues...

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

[Katsumi Yamaoka]

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

On Wed, 27 Mar 2019 13:54:42 +0900, Katsumi Yamaoka wrote:
> The group level of nnml:テスト is 1 and there are some unread
> articles in it, however the group is not listed in the Group
> buffer when I launch Gnus by `C-u 1 M-x gnus RET'...

I found a cause of it not to be listed in the group buffer just
after launching Gnus.

The group entry in the ~/Mail/active file is:

テスト 5 1 y

The file coding is utf-8-unix.  So, the binary expression of the
group name is \343\203\206\343\202\271\343\203\210, where \343
is a single character, and this form is what the hash tables use
(in the ~/.newsrc.eld file, "\343" consists of four characters
 "\", "3", "4", and "3", though).

`gnus-active-to-gnus-format' reads it in the following way:

・Load the active file to the " *nntpd*" buffer in the binary
　mode.  Copy it to another temp buffer.  There \343 is a single
　character.
・Read the group name using `(read (current-buffer))'.
　Then it is read as a symbol; \343 is still a single character.
・Convert it to a string using `symbol-name'.
　\343 is expanded into the one consists of four characters. :<
　It should never match to the one in the hash tables.

Why the single-char to four-chars conversion happens is that
the buffer where those processes are done is in the multibyte
mode.  So, the patch attached below solves the problem.  I'm
going to test it for the other back ends...


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

--- gnus-start.el~	2019-03-25 21:22:46.184139100 +0000
+++ gnus-start.el	2019-04-08 08:07:41.906976900 +0000
@@ -2139,3 +2139,3 @@
 
-    (with-temp-buffer
+    (mm-with-unibyte-buffer
       (insert-buffer-substring cur)

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

[Andreas Schwab]

On Apr 08 2019, Katsumi Yamaoka <yamaoka@jpl.org> wrote:

> ・Load the active file to the " *nntpd*" buffer in the binary
> 　mode.  Copy it to another temp buffer.  There \343 is a single
> 　character.
> ・Read the group name using `(read (current-buffer))'.
> 　Then it is read as a symbol; \343 is still a single character.
> ・Convert it to a string using `symbol-name'.
> 　\343 is expanded into the one consists of four characters. :<

It's not expanded.  That's just the print representation.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Eric Abrahamsen]

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


On 04/08/19 17:13 PM, Katsumi Yamaoka wrote:
> On Wed, 27 Mar 2019 13:54:42 +0900, Katsumi Yamaoka wrote:
>> The group level of nnml:テスト is 1 and there are some unread
>> articles in it, however the group is not listed in the Group
>> buffer when I launch Gnus by `C-u 1 M-x gnus RET'...
>
> I found a cause of it not to be listed in the group buffer just
> after launching Gnus.
>
> The group entry in the ~/Mail/active file is:
>
> テスト 5 1 y
>
> The file coding is utf-8-unix.  So, the binary expression of the
> group name is \343\203\206\343\202\271\343\203\210, where \343
> is a single character, and this form is what the hash tables use
> (in the ~/.newsrc.eld file, "\343" consists of four characters
>  "\", "3", "4", and "3", though).
>
> `gnus-active-to-gnus-format' reads it in the following way:
>
> ・Load the active file to the " *nntpd*" buffer in the binary
> 　mode.  Copy it to another temp buffer.  There \343 is a single
> 　character.
> ・Read the group name using `(read (current-buffer))'.
> 　Then it is read as a symbol; \343 is still a single character.
> ・Convert it to a string using `symbol-name'.
> 　\343 is expanded into the one consists of four characters. :<
> 　It should never match to the one in the hash tables.
>
> Why the single-char to four-chars conversion happens is that
> the buffer where those processes are done is in the multibyte
> mode.  So, the patch attached below solves the problem.  I'm
> going to test it for the other back ends...
>
> --- gnus-start.el~	2019-03-25 21:22:46.184139100 +0000
> +++ gnus-start.el	2019-04-08 08:07:41.906976900 +0000
> @@ -2139,3 +2139,3 @@
>
> -    (with-temp-buffer
> +    (mm-with-unibyte-buffer
>        (insert-buffer-substring cur)

I was asking on emacs.devel about that, and with Andreas' help came up
with the attached patch. It's a terrible hack, but it seems to work, and
I think would be good as an intermediate step.

What you're doing -- changing the unibyte/multibyte status of the
buffers -- is I think part of the final, more correct solution to the
problem, that will leave group names decoded everywhere. But I would
like to Gnus back into an intermediate working state before tackling
that...

What do you think?

Eric


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-on-nnmail-group-names.patch --]
[-- Type: text/x-patch, Size: 8190 bytes --]

From 4905ecb5dfd301d3ce9c6d92d2c0b6005a24fa51 Mon Sep 17 00:00:00 2001
From: Eric Abrahamsen <eric@ericabrahamsen.net>
Date: Sun, 31 Mar 2019 09:09:18 -0700
Subject: [PATCH] WIP on nnmail group names

---
 lisp/gnus/gnus-start.el | 15 +++++++++------
 lisp/gnus/nnmail.el     |  2 +-
 lisp/gnus/nnml.el       | 16 ++++++++--------
 lisp/gnus/nnrss.el      | 18 +++++++++---------
 4 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/lisp/gnus/gnus-start.el b/lisp/gnus/gnus-start.el
index 9b1be65067..2beb685822 100644
--- a/lisp/gnus/gnus-start.el
+++ b/lisp/gnus/gnus-start.el
@@ -2145,12 +2145,15 @@ gnus-active-to-gnus-format
 	(condition-case ()
 	    (if (and (stringp (progn
 				(setq group (read cur)
-				      group (cond ((numberp group)
-						   (number-to-string group))
-						  ((symbolp group)
-						   (symbol-name group))
-						  ((stringp group)
-						   group)))))
+				      group
+				      (encode-coding-string
+				       (cond ((numberp group)
+					      (number-to-string group))
+					     ((symbolp group)
+					      (symbol-name group))
+					     ((stringp group)
+					      group))
+				       'latin-1))))
 		     (numberp (setq max (read cur)))
 		     (numberp (setq min (read cur)))
 		     (null (progn
diff --git a/lisp/gnus/nnmail.el b/lisp/gnus/nnmail.el
index a95cdb4a4f..b6dbbea74c 100644
--- a/lisp/gnus/nnmail.el
+++ b/lisp/gnus/nnmail.el
@@ -663,7 +663,7 @@ nnmail-parse-active
 	    (narrow-to-region (point) (point-at-eol))
 	    (setq group (read buffer))
 	    (unless (stringp group)
-	      (setq group (symbol-name group)))
+	      (setq group (encode-coding-string (symbol-name group) 'latin-1)))
 	    (if (and (numberp (setq max (read buffer)))
 		     (numberp (setq min (read buffer))))
 		(push (list group (cons min max))
diff --git a/lisp/gnus/nnml.el b/lisp/gnus/nnml.el
index 5770777ad4..205e9e4803 100644
--- a/lisp/gnus/nnml.el
+++ b/lisp/gnus/nnml.el
@@ -259,7 +259,7 @@ nnml-request-group
      (t
       (nnheader-re-read-dir nnml-current-directory)
       (nnmail-activate 'nnml)
-      (let ((active (nth 1 (assoc group nnml-group-alist))))
+      (let ((active (nth 1 (assoc-string group nnml-group-alist))))
 	(if (not active)
 	    (nnheader-report 'nnml "No such group: %s" decoded)
 	  (nnheader-report 'nnml "Selected group %s" decoded)
@@ -295,7 +295,7 @@ nnml-request-create-group
     (nnheader-report 'nnml "%s is a file"
 		     (directory-file-name (nnml-group-pathname group
 							       nil server))))
-   ((assoc group nnml-group-alist)
+   ((assoc-string group nnml-group-alist)
     t)
    (t
     (let (active)
@@ -379,7 +379,7 @@ nnml-request-expire-articles
 		  (nnml-nov-delete-article group number))
 	      (push number rest)))
 	(push number rest)))
-    (let ((active (nth 1 (assoc group nnml-group-alist))))
+    (let ((active (nth 1 (assoc-string group nnml-group-alist))))
       (when active
 	(setcar active (or (and active-articles
 				(apply 'min active-articles))
@@ -520,7 +520,7 @@ nnml-request-delete-group
       (nnheader-report 'nnml "No such directory: %s/" file))
     ;; Remove the group from all structures.
     (setq nnml-group-alist
-	  (delq (assoc group nnml-group-alist) nnml-group-alist)
+	  (delq (assoc-string group nnml-group-alist) nnml-group-alist)
 	  nnml-current-group nil
 	  nnml-current-directory nil)
     ;; Save the active file.
@@ -549,7 +549,7 @@ nnml-request-rename-group
       (when (<= (length (directory-files old-dir)) 2)
 	(ignore-errors (delete-directory old-dir)))
       ;; That went ok, so we change the internal structures.
-      (let ((entry (assoc group nnml-group-alist)))
+      (let ((entry (assoc-string group nnml-group-alist)))
 	(when entry
 	  (setcar entry new-name))
 	(setq nnml-current-directory nil
@@ -597,7 +597,7 @@ nnml-deletable-article-p
     (when (setq path (nnml-article-to-file article))
       (when (file-writable-p path)
 	(or (not nnmail-keep-last-article)
-	    (not (eq (cdr (nth 1 (assoc group nnml-group-alist)))
+	    (not (eq (cdr (nth 1 (assoc-string group nnml-group-alist)))
 		     article)))))))
 
 ;; Find an article number in the current group given the Message-ID.
@@ -742,7 +742,7 @@ nnml-active-number
   "Compute the next article number in GROUP on SERVER."
   (let* ((encoded (if nnmail-group-names-not-encoded-p
 		      (nnml-encoded-group-name group server)))
-	 (active (cadr (assoc (or encoded group) nnml-group-alist))))
+	 (active (cadr (assoc-string (or encoded group) nnml-group-alist))))
     ;; The group wasn't known to nnml, so we just create an active
     ;; entry for it.
     (unless active
@@ -783,7 +783,7 @@ nnml-save-incremental-nov
 	    (cdr nnml-incremental-nov-buffer-alist)))))
 
 (defun nnml-open-incremental-nov (group)
-  (or (cdr (assoc group nnml-incremental-nov-buffer-alist))
+  (or (cdr (assoc-string group nnml-incremental-nov-buffer-alist))
       (let ((buffer (nnml-get-nov-buffer group t)))
 	(push (cons group buffer) nnml-incremental-nov-buffer-alist)
 	buffer)))
diff --git a/lisp/gnus/nnrss.el b/lisp/gnus/nnrss.el
index 7f2accc2b6..0bfecb28e0 100644
--- a/lisp/gnus/nnrss.el
+++ b/lisp/gnus/nnrss.el
@@ -340,10 +340,10 @@ nnrss-request-delete-group
   (let (elem)
     ;; There may be two or more entries in `nnrss-group-alist' since
     ;; this function didn't delete them formerly.
-    (while (setq elem (assoc group nnrss-group-alist))
+    (while (setq elem (assoc-string group nnrss-group-alist))
       (setq nnrss-group-alist (delq elem nnrss-group-alist))))
   (setq nnrss-server-data
-	(delq (assoc group nnrss-server-data) nnrss-server-data))
+	(delq (assoc-string group nnrss-server-data) nnrss-server-data))
   (nnrss-save-server-data server)
   (ignore-errors
     (let ((file-name-coding-system nnmail-pathname-coding-system))
@@ -367,7 +367,7 @@ nnrss-retrieve-groups
   (with-current-buffer nntp-server-buffer
     (erase-buffer)
     (dolist (group groups)
-      (let ((elem (assoc (gnus-group-decoded-name group) nnrss-server-data)))
+      (let ((elem (assoc-string (gnus-group-decoded-name group) nnrss-server-data)))
 	(insert (format "%S %s 1 y\n" group (or (cadr elem) 0)))))
     'active))
 
@@ -539,7 +539,7 @@ nnrss-read-group-data
   (if (hash-table-p nnrss-group-hashtb)
       (clrhash nnrss-group-hashtb)
     (setq nnrss-group-hashtb (make-hash-table :test 'equal)))
-  (let ((pair (assoc group nnrss-server-data)))
+  (let ((pair (assoc-string group nnrss-server-data)))
     (setq nnrss-group-max (or (cadr pair) 0))
     (setq nnrss-group-min (+ nnrss-group-max 1)))
   (let ((file (nnrss-make-filename group server))
@@ -644,8 +644,8 @@ nnrss-check-group
 					 (concat group ".xml"))
 					nnrss-directory))))
 	(setq xml (nnrss-fetch file t))
-      (setq url (or (nth 2 (assoc group nnrss-server-data))
-		    (cadr (assoc group nnrss-group-alist))))
+      (setq url (or (nth 2 (assoc-string group nnrss-server-data))
+		    (cadr (assoc-string group nnrss-group-alist))))
       (unless url
 	(setq url
 	      (cdr
@@ -653,7 +653,7 @@ nnrss-check-group
 		      (nnrss-discover-feed
 		       (read-string
 			(format "URL to search for %s: " group) "http://")))))
-	(let ((pair (assoc group nnrss-server-data)))
+	(let ((pair (assoc-string group nnrss-server-data)))
 	  (if pair
 	      (setcdr (cdr pair) (list url))
 	    (push (list group nnrss-group-max url) nnrss-server-data)))
@@ -721,7 +721,7 @@ nnrss-check-group
       (setq extra nil))
     (when changed
       (nnrss-save-group-data group server)
-      (let ((pair (assoc group nnrss-server-data)))
+      (let ((pair (assoc-string group nnrss-server-data)))
 	(if pair
 	    (setcar (cdr pair) nnrss-group-max)
 	  (push (list group nnrss-group-max) nnrss-server-data)))
@@ -792,7 +792,7 @@ nnrss-generate-download-script
   (insert "RSSDIR='" (expand-file-name nnrss-directory) "'\n")
   (dolist (elem nnrss-server-data)
     (let ((url (or (nth 2 elem)
-		   (cadr (assoc (car elem) nnrss-group-alist)))))
+		   (cadr (assoc-string (car elem) nnrss-group-alist)))))
       (insert "$WGET -q -O \"$RSSDIR\"/'"
 	      (nnrss-translate-file-chars (concat (car elem) ".xml"))
 	      "' '" url "'\n"))))
-- 
2.21.0

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

[Katsumi Yamaoka]

On Mon, 08 Apr 2019 10:57:57 +0200, Andreas Schwab wrote:
> On Apr 08 2019, Katsumi Yamaoka <yamaoka@jpl.org> wrote:
>> ・Convert it to a string using `symbol-name'.
>> 　\343 is expanded into the one consists of four characters. :<

> It's not expanded.  That's just the print representation.

Indeed.  I learned not only symbol-name, but also
eval-print-last-sexp, edebug-eval-expression, describe-variable,
etc. also just make it human readable one.

Thanks.

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

[Katsumi Yamaoka]

On Mon, 08 Apr 2019 11:31:57 -0700, Eric Abrahamsen wrote:
> On 04/08/19 17:13 PM, Katsumi Yamaoka wrote:
>> -    (with-temp-buffer
>> +    (mm-with-unibyte-buffer
>>        (insert-buffer-substring cur)

> I was asking on emacs.devel about that, and with Andreas' help came up
> with the attached patch. It's a terrible hack, but it seems to work, and
> I think would be good as an intermediate step.

> What you're doing -- changing the unibyte/multibyte status of the
> buffers -- is I think part of the final, more correct solution to the
> problem, that will leave group names decoded everywhere. But I would
> like to Gnus back into an intermediate working state before tackling
> that...

> What do you think?

Whatever we should do finally, isn't it necessary to make Gnus on
Emacs master work now anyway even if it is immature?  Those who
like the *latest* code by any means (me included) would be coming
to use it sooner or later.  So, I'd like you to install to master
a fix whatever makes Gnus work.

> --- a/lisp/gnus/gnus-start.el
> +++ b/lisp/gnus/gnus-start.el
[...]
>  				(setq group (read cur)
> +				      group
> +				      (encode-coding-string
> +				       (cond ((numberp group)
> +					      (number-to-string group))
> +					     ((symbolp group)
> +					      (symbol-name group))
> +					     ((stringp group)
> +					      group))
> +				       'latin-1))))

Makes sense to me.  Though encoding the one having already encoded
looks redundant, I have no idea other than this and my patch, and
benchmark reports there is no notably difference in speed between
this and my patch.
Anyway I verified it works for listing the nnml:テスト group.

> --- a/lisp/gnus/nnmail.el
> +++ b/lisp/gnus/nnmail.el

> --- a/lisp/gnus/nnml.el
> +++ b/lisp/gnus/nnml.el

Verified that those patches enable updating groups by `M-g'.

> --- a/lisp/gnus/nnrss.el
> +++ b/lisp/gnus/nnrss.el

Verified that I can read some feeds.

Thanks.

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

[Eric Abrahamsen]

On 04/09/19 09:55 AM, Katsumi Yamaoka wrote:
> On Mon, 08 Apr 2019 11:31:57 -0700, Eric Abrahamsen wrote:
>> On 04/08/19 17:13 PM, Katsumi Yamaoka wrote:
>>> -    (with-temp-buffer
>>> +    (mm-with-unibyte-buffer
>>>        (insert-buffer-substring cur)
>
>> I was asking on emacs.devel about that, and with Andreas' help came up
>> with the attached patch. It's a terrible hack, but it seems to work, and
>> I think would be good as an intermediate step.
>
>> What you're doing -- changing the unibyte/multibyte status of the
>> buffers -- is I think part of the final, more correct solution to the
>> problem, that will leave group names decoded everywhere. But I would
>> like to Gnus back into an intermediate working state before tackling
>> that...
>
>> What do you think?
>
> Whatever we should do finally, isn't it necessary to make Gnus on
> Emacs master work now anyway even if it is immature?  Those who
> like the *latest* code by any means (me included) would be coming
> to use it sooner or later.  So, I'd like you to install to master
> a fix whatever makes Gnus work.

Yes, that's what I was saying! Do the minimum needed now to get things
working again, then I'll be more careful putting a more thorough fix
later.

>> --- a/lisp/gnus/gnus-start.el
>> +++ b/lisp/gnus/gnus-start.el
> [...]
>>  				(setq group (read cur)
>> +				      group
>> +				      (encode-coding-string
>> +				       (cond ((numberp group)
>> +					      (number-to-string group))
>> +					     ((symbolp group)
>> +					      (symbol-name group))
>> +					     ((stringp group)
>> +					      group))
>> +				       'latin-1))))
>
> Makes sense to me.  Though encoding the one having already encoded
> looks redundant, I have no idea other than this and my patch, and
> benchmark reports there is no notably difference in speed between
> this and my patch.

Yes, it's ugly, but hopefully it's temporary. I am glad that it doesn't
affect speed, though.

> Anyway I verified it works for listing the nnml:テスト group.
>
>> --- a/lisp/gnus/nnmail.el
>> +++ b/lisp/gnus/nnmail.el
>
>> --- a/lisp/gnus/nnml.el
>> +++ b/lisp/gnus/nnml.el
>
> Verified that those patches enable updating groups by `M-g'.
>
>> --- a/lisp/gnus/nnrss.el
>> +++ b/lisp/gnus/nnrss.el
>
> Verified that I can read some feeds.

Thanks very much. I'll push this now, and then see what else needs
cleaning up.

Eric

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

[Katsumi Yamaoka]

On Mon, 08 Apr 2019 19:01:08 -0700, Eric Abrahamsen wrote:
> Thanks very much. I'll push this now, and then see what else needs
> cleaning up.

Thanks!  I added this one (cf. <b4mwok52k54.fsf@jpl.org>):

Make `jump to group' work even if it is not activated (bug#33653)

* lisp/gnus/gnus-group.el (gnus-group-goto-group):
Use gnus-newsrc-hashtb instead of gnus-newsrc-hashtb to check if
a group exists even if its server is not activated (bug#33653).

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

[Eric Abrahamsen]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

> On Mon, 08 Apr 2019 19:01:08 -0700, Eric Abrahamsen wrote:
>> Thanks very much. I'll push this now, and then see what else needs
>> cleaning up.
>
> Thanks!  I added this one (cf. <b4mwok52k54.fsf@jpl.org>):
>
> Make `jump to group' work even if it is not activated (bug#33653)
>
> * lisp/gnus/gnus-group.el (gnus-group-goto-group):
> Use gnus-newsrc-hashtb instead of gnus-newsrc-hashtb to check if
> a group exists even if its server is not activated (bug#33653).

Yes, I think that's better behavior.

I'll wait to hear from one or two other reporters, then close this bug
(and reopen new ones as necessary).

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

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Always-set-gnus-group-property-to-a-group-name.patch --]
[-- Type: text/x-diff, Size: 7472 bytes --]

From 5d3eb57ae62decf6316ab7c882cc6f18a9c63ae3 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Thu, 11 Apr 2019 17:24:36 +0100
Subject: [PATCH] Always set gnus-group property to a group name

* lisp/gnus/gnus-group.el (gnus-group-prepare-flat-list-dead):
Set gnus-group property to a group name, not active info.
Simplify surrounding logic.
(gnus-group-prepare-flat, gnus-group-goto-group): Use accessor
macros.
(gnus-group-insert-group-line, gnus-group-new-mail)
(gnus-group-mark-group): Write ?\s instead of ? .
(gnus-group-group-name, gnus-group-list-active): Simplify.
---
 lisp/gnus/gnus-group.el | 98 ++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 55 deletions(-)

diff --git a/lisp/gnus/gnus-group.el b/lisp/gnus/gnus-group.el
index e1554cab8a..b1e4091c97 100644
--- a/lisp/gnus/gnus-group.el
+++ b/lisp/gnus/gnus-group.el
@@ -1320,7 +1320,7 @@ gnus-group-prepare-flat
 	      gnus-group-listed-groups)
       ;; List living groups, according to order in `gnus-group-list'.
       (dolist (g (cdr gnus-group-list))
-	(setq info (nth 1 (gethash g gnus-newsrc-hashtb))
+        (setq info (gnus-get-info g)
 	      group (gnus-info-group info)
 	      params (gnus-info-params info)
 	      unread (gnus-group-unread group))
@@ -1389,39 +1389,35 @@ gnus-group-prepare-flat-list-dead
   ;; List zombies and killed lists somewhat faster, which was
   ;; suggested by Jack Vinson <vinson@unagi.cis.upenn.edu>.  It does
   ;; this by ignoring the group format specification altogether.
-  (let (group)
-    (if (> (length groups) gnus-group-listing-limit)
-	(while groups
-	  (setq group (pop groups))
-	  (when (gnus-group-prepare-logic
-		 group
-		 (or (not regexp)
-		     (and (stringp regexp) (string-match regexp group))
-		     (and (functionp regexp) (funcall regexp group))))
-	    (add-text-properties
-	     (point) (prog1 (1+ (point))
-		       (insert " " mark "     *: "
-			       (gnus-group-decoded-name group)
-			       "\n"))
-	     (list 'gnus-group (gethash group gnus-active-hashtb)
-		   'gnus-unread t
-		   'gnus-level level))))
-      (while groups
-	(setq group (pop groups))
+  (if (nthcdr gnus-group-listing-limit groups)
+      (dolist (group groups)
 	(when (gnus-group-prepare-logic
 	       group
-	       (or (not regexp)
-		   (and (stringp regexp) (string-match regexp group))
-		   (and (functionp regexp) (funcall regexp group))))
-	  (gnus-group-insert-group-line
-	   group level nil
-	   (let ((active (gnus-active group)))
-	     (if active
-		 (if (zerop (cdr active))
-		     0
-		   (- (1+ (cdr active)) (car active)))
-	       nil))
-	   (gnus-method-simplify (gnus-find-method-for-group group))))))))
+               (cond ((not regexp))
+                     ((stringp regexp) (string-match-p regexp group))
+                     ((functionp regexp) (funcall regexp group))))
+          (add-text-properties
+           (point) (prog1 (1+ (point))
+                     (insert " " mark "     *: "
+                             (gnus-group-decoded-name group)
+                             "\n"))
+           (list 'gnus-group group
+                 'gnus-unread t
+                 'gnus-level level))))
+    (dolist (group groups)
+      (when (gnus-group-prepare-logic
+             group
+             (cond ((not regexp))
+                   ((stringp regexp) (string-match-p regexp group))
+                   ((functionp regexp) (funcall regexp group))))
+        (gnus-group-insert-group-line
+         group level nil
+         (let ((active (gnus-active group)))
+           (and active
+                (if (zerop (cdr active))
+                    0
+                  (- (cdr active) (car active) -1))))
+         (gnus-method-simplify (gnus-find-method-for-group group)))))))
 
 (defun gnus-group-update-group-line ()
   "Update the current line in the group buffer."
@@ -1527,7 +1523,7 @@ gnus-group-insert-group-line
 	      (int-to-string (max 0 (- gnus-tmp-number-total number)))
 	    "*"))
 	 (gnus-tmp-subscribed
-	  (cond ((<= gnus-tmp-level gnus-level-subscribed) ? )
+          (cond ((<= gnus-tmp-level gnus-level-subscribed) ?\s)
 		((<= gnus-tmp-level gnus-level-unsubscribed) ?U)
 		((= gnus-tmp-level gnus-level-zombie) ?Z)
 		(t ?K)))
@@ -1546,7 +1542,7 @@ gnus-group-insert-group-line
 	 (gnus-tmp-moderated
 	  (if (and gnus-moderated-hashtb
 		   (gethash gnus-tmp-group gnus-moderated-hashtb))
-	      ?m ? ))
+              ?m ?\s))
 	 (gnus-tmp-moderated-string
 	  (if (eq gnus-tmp-moderated ?m) "(m)" ""))
          (gnus-tmp-group-icon (gnus-group-get-icon gnus-tmp-group))
@@ -1560,15 +1556,15 @@ gnus-group-insert-group-line
 	  (if (and (numberp number)
 		   (zerop number)
 		   (cdr (assq 'tick gnus-tmp-marked)))
-	      ?* ? ))
+              ?* ?\s))
 	 (gnus-tmp-summary-live
 	  (if (and (not gnus-group-is-exiting-p)
 		   (gnus-buffer-live-p (gnus-summary-buffer-name
 					gnus-tmp-group)))
-	      ?* ? ))
+              ?* ?\s))
 	 (gnus-tmp-process-marked
 	  (if (member gnus-tmp-group gnus-group-marked)
-	      gnus-process-mark ? ))
+              gnus-process-mark ?\s))
 	 (buffer-read-only nil)
 	 beg end
          gnus-tmp-header)	  ; passed as parameter to user-funcs.
@@ -1768,10 +1764,8 @@ gnus-group-set-mode-line
 (defun gnus-group-group-name ()
   "Get the name of the newsgroup on the current line."
   (let ((group (get-text-property (point-at-bol) 'gnus-group)))
-    (when group
-      (if (stringp group)
-	  group
-	(symbol-name group)))))
+    (cond ((stringp group) group)
+          (group (symbol-name group)))))
 
 (defun gnus-group-group-level ()
   "Get the level of the newsgroup on the current line."
@@ -1791,7 +1785,7 @@ gnus-group-group-unread
 (defun gnus-group-new-mail (group)
   (if (nnmail-new-mail-p (gnus-group-real-name group))
       gnus-new-mail-mark
-    ? ))
+    ?\s))
 
 (defun gnus-group-level (group)
   "Return the estimated level of GROUP."
@@ -1881,7 +1875,7 @@ gnus-group-mark-group
 	(if unmark
 	    (progn
 	      (setq gnus-group-marked (delete group gnus-group-marked))
-	      (insert-char ? 1 t))
+              (insert-char ?\s 1 t))
 	   (setq gnus-group-marked
 		 (cons group (delete group gnus-group-marked)))
 	   (insert-char gnus-process-mark 1 t)))
@@ -2561,10 +2555,10 @@ gnus-group-goto-group
   (when group
     (let ((start (point))
 	  (active (and (or
-			;; some kind of group may be only there.
-			(gethash group gnus-active-hashtb)
-			;; all groups (but with exception) are there.
-			(gethash group gnus-newsrc-hashtb))
+                        ;; Some kind of group may be only there.
+                        (gnus-active group)
+                        ;; All groups (but with exception) are there.
+                        (gnus-group-entry group))
 		       group)))
       (beginning-of-line)
       (cond
@@ -4013,15 +4007,9 @@ gnus-group-list-active
 	  (gnus-agent gnus-plugged)); If we're actually plugged, store the active file in the agent.
       (gnus-read-active-file)))
   ;; Find all groups and sort them.
-  (let ((groups
-	 (sort
-	  (hash-table-keys gnus-active-hashtb)
-	  'string<))
-	(buffer-read-only nil)
-	group)
+  (let ((buffer-read-only nil))
     (erase-buffer)
-    (while groups
-      (setq group (pop groups))
+    (dolist (group (sort (hash-table-keys gnus-active-hashtb) #'string<))
       (add-text-properties
        (point) (prog1 (1+ (point))
 		 (insert "       *: "
-- 
2.20.1


[-- Attachment #2: Type: text/plain, Size: 346 bytes --]


While looking at bug#35208 and bug#35233, I spotted one more place in
gnus-group.el which sets the gnus-group text property to the group's
active info instead of its name, namely in
gnus-group-prepare-flat-list-dead.

Is the attached patch, which includes some extra cleanup, okay to push?
Or have I misunderstood something?

Thanks,

-- 
Basil

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

[Katsumi Yamaoka]

On Thu, 11 Apr 2019 22:29:23 +0100, Basil L. Contovounesios wrote:
> While looking at bug#35208 and bug#35233, I spotted one more place in
> gnus-group.el which sets the gnus-group text property to the group's
> active info instead of its name, namely in
> gnus-group-prepare-flat-list-dead.

I see this fix is necessary:

(add-text-properties
 (the_beginning_of_each_group_line)
 (list 'gnus-group (gethash group gnus-active-hashtb)
↓
(add-text-properties
 (the_beginning_of_each_group_line)
 (list 'gnus-group group

> Is the attached patch, which includes some extra cleanup, okay to push?

No problem I think.  Please push the patch.

Thanks.

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

[Basil L. Contovounesios]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

>> Is the attached patch, which includes some extra cleanup, okay to push?
>
> No problem I think.  Please push the patch.

Done, thanks.

-- 
Basil

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

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Katsumi Yamaoka <yamaoka@jpl.org> writes:
>
>>> Is the attached patch, which includes some extra cleanup, okay to push?
>>
>> No problem I think.  Please push the patch.
>
> Done, thanks.

Browsing this thread, it seems like everything was resolved, so I'm
closing this bug report.

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