2.0 newsgroup name incompatibility (unlikely to affect anyone...)

2.0 newsgroup name incompatibility (unlikely to affect anyone...)

[Eric Wong]

I don't think this affects anyone, but historically we supported
uppercase characters in newsgroup names since (IIRC) the venerable
INN server was case-sensitive.  However, IMAP, POP3 require
lowercase; and our extindex (and WIP -cindex) get confused/broken
with uppercase characters.

So I think it's fine to start lowercasing all newsgroup names
internally and warning about uppercase ones:

-----8<-----
Subject: [PATCH] config: avoid eidx_key and newsgroup conflicts

Start lowercasing newsgroup names automatically since uppercase
names are incompatible with IMAP and POP3 and also causes
problems with both -extindex and -cindex.

We'll also warn on eidx_key and newsgroup conflicts to avoid
sometimes subtle breakage when using -extindex and -cindex.
---
 Documentation/RelNotes/v2.0.0.wip     |  8 ++++++
 Documentation/public-inbox-config.pod |  5 ++++
 lib/PublicInbox/Config.pm             | 14 +++++++++--
 lib/PublicInbox/IMAPD.pm              |  9 ++-----
 t/config.t                            | 35 ++++++++++++++++++++++++++-
 t/imap.t                              | 13 +++++-----
 6 files changed, 67 insertions(+), 17 deletions(-)

diff --git a/Documentation/RelNotes/v2.0.0.wip b/Documentation/RelNotes/v2.0.0.wip
index 859d858b..5fc9ecd7 100644
--- a/Documentation/RelNotes/v2.0.0.wip
+++ b/Documentation/RelNotes/v2.0.0.wip
@@ -18,6 +18,14 @@ Upgrading:
   backwards-incompatible data format changes so rolling back to
   older versions is harmless.
 
+Compatibility:
+
+  Uppercase newsgroup names were always broken with IMAP, POP3, and
+  -extindex.  Uppercase names will now be lowercased by default and
+  warnings will be emitted.  Conflicting newsgroup names (and `inboxdir'
+  entries if `newsgroup' isn't specified) will also generate warnings
+  since it breaks -extindex and the new -cindex (coderepo index)
+
 treewide
 
   * support raw UTF-8 headers from SMTPUTF8 hosts
diff --git a/Documentation/public-inbox-config.pod b/Documentation/public-inbox-config.pod
index 1ef7f46f..d394d31f 100644
--- a/Documentation/public-inbox-config.pod
+++ b/Documentation/public-inbox-config.pod
@@ -74,6 +74,11 @@ Omitting this for a given inbox will prevent the inbox from
 being served by L<public-inbox-nntpd(1)>,
 L<public-inbox-imapd(1)>, and/or L<public-inbox-pop3d(1)>
 
+Newsgroup names should be all lowercase.  Uppercase characters are
+converted to lowercase for compatibility with IMAP, POP3, and our
+L<public-inbox-extindex(1)> and L<public-inbox-cindex(1)> tools
+starting with public-inbox 2.0+ (they were unusable before).
+
 Default: none, optional
 
 =item publicinbox.<name>.watch
diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 01cb536d..9bee94b8 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -509,9 +509,16 @@ sub _fill_ibx {
 			delete $ibx->{newsgroup};
 			warn "newsgroup name invalid: `$ngname'\n";
 		} else {
+			my $lc = $ibx->{newsgroup} = lc $ngname;
+			warn <<EOM if $lc ne $ngname;
+W: newsgroup=`$ngname' lowercased to `$lc'
+EOM
 			# PublicInbox::NNTPD does stricter ->nntp_usable
 			# checks, keep this lean for startup speed
-			$self->{-by_newsgroup}->{$ngname} = $ibx;
+			my $cur = $self->{-by_newsgroup}->{$lc} //= $ibx;
+			warn <<EOM if $cur != $ibx;
+W: newsgroup=`$lc' is used by both `$cur->{name}' and `$ibx->{name}'
+EOM
 		}
 	}
 	unless (defined $ibx->{newsgroup}) { # for ->eidx_key
@@ -531,7 +538,10 @@ sub _fill_ibx {
 		require PublicInbox::Isearch;
 		$ibx->{isrch} = PublicInbox::Isearch->new($ibx, $es);
 	}
-	$self->{-by_eidx_key}->{$ibx->eidx_key} = $ibx;
+	my $cur = $self->{-by_eidx_key}->{my $ekey = $ibx->eidx_key} //= $ibx;
+	$cur == $ibx or warn
+		"W: `$ekey' used by both `$cur->{name}' and `$ibx->{name}'\n";
+	$ibx;
 }
 
 sub _fill_ei ($$) {
diff --git a/lib/PublicInbox/IMAPD.pm b/lib/PublicInbox/IMAPD.pm
index bdadb7a3..42dc2a9f 100644
--- a/lib/PublicInbox/IMAPD.pm
+++ b/lib/PublicInbox/IMAPD.pm
@@ -27,13 +27,8 @@ sub _refresh_ibx { # pi_cfg->each_inbox cb
 	my ($ibx, $imapd, $cache, $dummies) = @_;
 	my $ngname = $ibx->{newsgroup} // return;
 
-	# We require lower-case since IMAP mailbox names are
-	# case-insensitive (but -nntpd matches INN in being
-	# case-sensitive)
-	if ($ngname =~ m![^a-z0-9/_\.\-\~\@\+\=:]! ||
-			# don't confuse with 50K slices
-			$ngname =~ /\.[0-9]+\z/) {
-		warn "mailbox name invalid: newsgroup=`$ngname'\n";
+	if ($ngname =~ /\.[0-9]+\z/) { # don't confuse with 50K slices
+		warn "E: mailbox name invalid: newsgroup=`$ngname' (ignored)\n";
 		return;
 	}
 	my $ce = $cache->{$ngname};
diff --git a/t/config.t b/t/config.t
index 9b6684b7..975cf836 100644
--- a/t/config.t
+++ b/t/config.t
@@ -299,4 +299,37 @@ my $re = $glob2re->('a/**/b');
 is_deeply($re, 'a/.*?b', 'double asterisk middle');
 like($_, qr!$re!, "a/**/b matches $_") for ('a/b', 'a/c/b', 'a/c/a/b');
 
-done_testing();
+{
+	my $w = '';
+	local $SIG{__WARN__} = sub { $w .= "@_"; };
+	my $cfg = cfg_new $tmpdir, <<EOF;
+[publicinbox "a"]
+	address = a\@example.com
+	inboxdir = $tmpdir/aa
+[publicinbox "b"]
+	address = b\@example.com
+	inboxdir = $tmpdir/aa
+EOF
+	$cfg->fill_all;
+	like $w, qr!`\Q$tmpdir/aa\E' used by both!, 'inboxdir conflict warned';
+}
+
+{
+	my $w = '';
+	local $SIG{__WARN__} = sub { $w .= "@_"; };
+	my $cfg = cfg_new $tmpdir, <<EOF;
+[publicinbox "a"]
+	address = a\@example.com
+	inboxdir = $tmpdir/a
+	newsgroup = inbox.test
+[publicinbox "b"]
+	address = b\@example.com
+	inboxdir = $tmpdir/b
+	newsgroup = inbox.tesT
+EOF
+	$cfg->fill_all;
+	like $w, qr!`inbox\.test' used by both!, 'newsgroup conflict warned';
+	like $w, qr!`inbox\.tesT' lowercased!, 'upcase warned';
+}
+
+done_testing;
diff --git a/t/imap.t b/t/imap.t
index e6efe04f..44b8ef2a 100644
--- a/t/imap.t
+++ b/t/imap.t
@@ -1,5 +1,5 @@
 #!perl -w
-# Copyright (C) 2020-2021 all contributors <meta@public-inbox.org>
+# Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 # unit tests (no network) for IMAP, see t/imapd.t for end-to-end tests
 use strict;
@@ -9,12 +9,12 @@ require_git 2.6;
 require_mods(qw(-imapd));
 require_ok 'PublicInbox::IMAP';
 require_ok 'PublicInbox::IMAPD';
+use PublicInbox::IO qw(write_file);
 
 my ($tmpdir, $for_destroy) = tmpdir();
 my $cfgfile = "$tmpdir/config";
 {
-	open my $fh, '>', $cfgfile or BAIL_OUT $!;
-	print $fh <<EOF or BAIL_OUT $!;
+	write_file '>', $cfgfile, <<EOF;
 [publicinbox "a"]
 	inboxdir = $tmpdir/a
 	newsgroup = x.y.z
@@ -23,9 +23,8 @@ my $cfgfile = "$tmpdir/config";
 	newsgroup = x.z.y
 [publicinbox "c"]
 	inboxdir = $tmpdir/c
-	newsgroup = IGNORE.THIS
+	newsgroup = ignore.this.9
 EOF
-	close $fh or BAIL_OUT $!;
 	local $ENV{PI_CONFIG} = $cfgfile;
 	for my $x (qw(a b c)) {
 		ok(run_script(['-init', '-Lbasic', '-V2', $x, "$tmpdir/$x",
@@ -37,8 +36,8 @@ EOF
 	local $SIG{__WARN__} = sub { push @w, @_ };
 	$imapd->refresh_groups;
 	my $self = { imapd => $imapd };
-	is(scalar(@w), 1, 'got a warning for upper-case');
-	like($w[0], qr/IGNORE\.THIS/, 'warned about upper-case');
+	is(scalar(@w), 1, 'got a warning for slice-like name');
+	like($w[0], qr/ignore\.this\.9/i, 'warned about slice-like name');
 	my $res = PublicInbox::IMAP::cmd_list($self, 'tag', 'x', '%');
 	is(scalar($$res =~ tr/\n/\n/), 2, 'only one result');
 	like($$res, qr/ x\r\ntag OK/, 'saw expected');