Remove improved_mboxlist_sort leftovers

• Remove option CYRUSDB_MBOXSORT • The database comparisson functions do not need a copy of the database handle, so simplify accordingly. • Delete unused functions bsearch_uncompare_mbox() and bsearch_compare_mbox()
dilyanpalauzov · Aug 16, 2023 · 7d7c515 · 7d7c515
1 parent 1525e55
commit 7d7c515
Show file tree

Hide file tree

Showing 29 changed files with 58 additions and 423 deletions.
diff --git a/cassandane/Cassandane/Config.pm b/cassandane/Cassandane/Config.pm
@@ -108,8 +108,6 @@ sub default
             delete_mode => 'delayed',
             # for debugging - see cassandane.ini.example
             debug_command => '@prefix@/utils/gdbtramp %s %d',
-            # everyone should be running this
-            improved_mboxlist_sort => 'yes',
             # default changed, we want to be explicit about it
             unixhierarchysep => 'no',
             # let's hear all about it

diff --git a/cassandane/Cassandane/Cyrus/Prometheus.pm b/cassandane/Cassandane/Cyrus/Prometheus.pm
@@ -257,70 +257,6 @@ sub test_quota_commitments
     $self->assert_equals(10000, $report->{'cyrus_usage_quota_commitment'}->{'partition="p2",resource="STORAGE"'}->{value});
 }
 
-# tests for pathological quotaroot/partition subdivisions
-sub test_quota_commitments_no_improved_mboxlist_sort
-    :min_version_3_1 :needs_component_httpd :Partition2 :NoStartInstances
-{
-    my ($self) = @_;
-
-    $self->{instance}->{config}->set('improved_mboxlist_sort', 'no');
-    $self->_start_instances();
-
-    my $admintalk = $self->{adminstore}->get_client();
-
-    my $inbox = 'user.cassandane';  # allocate top level quota here
-    my $child = "$inbox.child";
-    my $gchild1 = "$child.cat"; # we'll stick this one on a sep part
-    my $gchild2 = "$child.dog"; # give this one its own quota
-    my $gchild3 = "$child.sheep"; # normal, but sorts after weird ones
-    my $ggchild1 = "$gchild1.manx"; # and give this one its own quota
-    my $ggchild2 = "$gchild1.siamese"; # and this one back on def part
-    my $interm = "$inbox.foo.bar.baz"; # contains intermediate folders
-    my $inbox2 = 'user.cassandane-child'; # hyphen! own quota
-
-    # make some folders
-    foreach my $f ($child, $gchild1, $gchild2, $gchild3, $ggchild1,
-                   $ggchild2, $interm, $inbox2) {
-        $admintalk->create($f);
-        $self->assert_str_equals('ok',
-                                 $admintalk->get_last_completion_response());
-    }
-
-    # stick one of them on a different partition
-    $admintalk->rename($gchild1, $gchild1, 'p2');
-    $self->assert_str_equals('ok', $admintalk->get_last_completion_response());
-
-    # but not one of its children
-    $admintalk->rename($ggchild2, $ggchild2, 'default');
-    $self->assert_str_equals('ok', $admintalk->get_last_completion_response());
-
-    # create a mess of quotas
-    $admintalk->setquota($inbox, '(STORAGE 8000)');
-    $self->assert_str_equals('ok', $admintalk->get_last_completion_response());
-    $admintalk->setquota($gchild2, '(STORAGE 4000)');
-    $self->assert_str_equals('ok', $admintalk->get_last_completion_response());
-    $admintalk->setquota($ggchild1, '(STORAGE 2000)');
-    $self->assert_str_equals('ok', $admintalk->get_last_completion_response());
-    $admintalk->setquota($inbox2, '(STORAGE 1000)');
-    $self->assert_str_equals('ok', $admintalk->get_last_completion_response());
-
-    $admintalk->logout();
-
-    sleep 3;
-
-    my $response = $self->http_report();
-    $self->assert($response->{success});
-
-    my $report = parse_report($response->{content});
-    $self->assert(scalar keys %{$report});
-
-    # now we expect default partition to have 8000 + 4000 +1000 committed
-    $self->assert_equals(13000, $report->{'cyrus_usage_quota_commitment'}->{'partition="default",resource="STORAGE"'}->{value});
-
-    # and p2 partition to have 8000 + 2000 committed
-    $self->assert_equals(10000, $report->{'cyrus_usage_quota_commitment'}->{'partition="p2",resource="STORAGE"'}->{value});
-}
-
 sub test_shared_mailbox_namespaces
     :min_version_3_1 :needs_component_httpd
 {

diff --git a/cassandane/Cassandane/Cyrus/Quota.pm b/cassandane/Cassandane/Cyrus/Quota.pm
@@ -1121,96 +1121,6 @@ sub test_quota_d
     $self->assert_does_not_match(qr{dave\@qux.com}, $content);
 }
 
-# https://github.com/cyrusimap/cyrus-imapd/issues/2877
-sub test_quota_f_no_improved_mboxlist_sort
-    :unixHierarchySep :AltNamespace :VirtDomains :NoStartInstances
-{
-    my ($self) = @_;
-
-    my $user = '[email protected]';
-    my @otherusers = (
-        '[email protected]',
-        '[email protected]',
-        '[email protected]',
-    );
-
-    $self->{instance}->{config}->set('improved_mboxlist_sort', 'no');
-    $self->_start_instances();
-
-    my $admintalk = $self->{adminstore}->get_client();
-    $admintalk->create("user/$user");
-    $self->assert_str_equals('ok',
-                                $admintalk->get_last_completion_response());
-    $admintalk->setacl("user/$user", $user, 'lrswipkxtecdan');
-    $self->assert_str_equals('ok',
-                                $admintalk->get_last_completion_response());
-
-    xlog $self, "set ourselves a basic usage quota";
-    $self->_set_limits(
-        quotaroot => "user/$user",
-        storage => 100000,
-        message => 50000,
-        $res_annot_storage => 10000,
-    );
-    $self->_check_usages(
-        quotaroot => "user/$user",
-        storage => 0,
-        message => 0,
-        $res_annot_storage => 0,
-    );
-
-    # create some other users to tickle sort-order issues?
-    foreach my $x (@otherusers) {
-        $admintalk->create("user/$x");
-        $self->_set_limits(
-            quotaroot => "user/$x",
-            storage => 100000,
-            message => 50000,
-            $res_annot_storage => 10000,
-        );
-    }
-
-    my $svc = $self->{instance}->get_service('imap');
-    my $userstore = $svc->create_store(username => $user);
-    my $usertalk = $userstore->get_client();
-
-    foreach my $submbox ('Drafts', 'Junk', 'Sent', 'Trash') {
-        xlog $self, "creating $submbox...";
-        $usertalk->create($submbox);
-        $self->assert_str_equals('ok',
-                                  $usertalk->get_last_completion_response());
-    }
-
-    $usertalk->list("", "*");
-
-    foreach my $mbox (qw(INBOX Drafts Sent Junk Trash)) {
-        $usertalk->select($mbox);
-        foreach (1..3) {
-            $self->make_message("msg $_ in $mbox", store => $userstore);
-        }
-    }
-
-    xlog $self, "run quota -d";
-    $self->{instance}->run_command({ cyrus => 1 },
-                                   'quota', '-d', 'example.com');
-
-    xlog $self, "run quota -d -f";
-    my $outfile = $self->{instance}->{basedir} . '/quota.out';
-    my @data = $self->{instance}->run_command({
-        cyrus => 1,
-        redirects => {
-            stderr => $outfile,
-            stdout => $outfile,
-        },
-    }, 'quota', '-f', '-d', 'example.com');
-
-    my $str = slurp_file($outfile);
-    xlog $self, $str;
-
-    #example.com!user.user1.Junk: quota root example.com!user.user1 --> (none)
-    $self->assert_does_not_match(qr{ quota root \S+ --> \(none\)}, $str);
-}
-
 sub test_quota_f_unixhs
     :UnixHierarchySep
 {

diff --git a/changes/next/deprecate_improved_mboxlist b/changes/next/deprecate_improved_mboxlist
@@ -0,0 +1,20 @@
+Description:
+
+The improved_mboxlist_sort option had no effect since v3.6.  It is now deprecated.
+
+cyr_dbtool option -M/--improved-mboxlist-sort is removed.
+
+
+Config changes:
+
+improved_mboxlist_sort is now deprecated.
+
+
+Upgrade instructions:
+
+Remove improved_mboxlist_sort from imapd.conf.  It has no effect and is now deprecated.
+
+
+GitHub issue:
+
+https://github.com/cyrusimap/cyrus-imapd/pull/4559
diff --git a/cunit/aaa-db.testc b/cunit/aaa-db.testc
@@ -799,9 +799,9 @@ static void test_mboxlist(void)
     r = cyrusdb_close(db);
     CU_ASSERT_EQUAL(r, CYRUSDB_OK);
 
-    /* try again with mboxlist sort */
+    /* try again. This test used CYRUSDB_MBOXSORT. */
 
-    r = cyrusdb_open(backend, filename2, CYRUSDB_CREATE | CYRUSDB_MBOXSORT, &db);
+    r = cyrusdb_open(backend, filename2, CYRUSDB_CREATE, &db);
     CU_ASSERT_EQUAL(r, CYRUSDB_OK);
     CU_ASSERT_PTR_NOT_NULL(db);
 

diff --git a/cunit/quota.testc b/cunit/quota.testc
@@ -734,8 +734,7 @@ static void notest_foreach(void)
 #endif
 
 /*
- * TODO: should test for quota_foreach() iteration order,
- *  with and without IMAPOPT_IMPROVED_MBOXLIST_SORT set.
+ * TODO: should test for quota_foreach() iteration order.
  *  There is code that depends on it, e.g. for quota -f.
  */
 

diff --git a/doc/README.cyrusdb.md b/doc/README.cyrusdb.md
@@ -85,14 +85,6 @@ if (!r) return mydb;  // if (r == CYRUSDB_OK) { ... }
 * `CYRUSDB_CREATE` - if the named database doesn't exist, create a
   blank database.
 
-* `CYRUSDB_MBOXSORT` - use the abomination called
-  `improved_mboxlist_sort` which re-orders a couple of characters to
-  allow "foo.bar" to sort before "foo bar", for perfectly good
-  reasons, but we're going to fix it a better way.  Not every engine
-  supports arbitrary collation, and if many engines corrupt horribly
-  if the same database is opened with different choices for this flag.
-  Ouch.
-
 * `CYRUSDB_CONVERT` - if set and the database fails to open, attempt a
   magic detection on the file content and try to convert the database
   to the requested backend type before opening it.  In-place upgrades!

diff --git a/doc/internal/cyrusdb-api.html b/doc/internal/cyrusdb-api.html
@@ -154,7 +154,6 @@ <h3>cyrusdb_open(const char *backend, const char *fname, int flags,
 <p>Flags:</p>
 <ul>
  <li>CYRUSDB_CREATE - create the database if it doesn't exist</li>
- <li>CYRUSDB_MBOXSORT - sort '.' first, so folder listing is correct</li>
 </ul>
 
 <p>Errors:</p>

diff --git a/doc/internal/improved_mboxlist_sort.html b/doc/internal/improved_mboxlist_sort.html
diff --git a/doc/internal/index.html b/doc/internal/index.html
@@ -44,7 +44,6 @@ <h2>The Cyrus IMAP Server Distribution</h2>
 
 <li><a href="specials.html">Special Characters</a></li>
 
-<li><a href="improved_mboxlist_sort.html">Enabling improved_mboxlist_sort</a></li>
 </ul>
 
 </body>

diff --git a/docsrc/imap/developer/API/cyrusdb.rst b/docsrc/imap/developer/API/cyrusdb.rst
@@ -196,13 +196,6 @@ Accepted flags:
 CYRUSDB_CREATE - if the named database doesn't exist, create a blank
 database.
 
-CYRUSDB_MBOXSORT - use the abomination called improved_mboxlist_sort
-which re-orders a couple of characters to allow "foo.bar" to sort
-before "foo bar", for perfectly good reasons, but we're going to fix it
-a better way. Not every engine supports arbitrary collation, and if
-many engines corrupt horribly if the same database is opened with
-different choices for this flag. Ouch.
-
 CYRUSDB_CONVERT - if set and the database fails to open, attempt a
 magic detection on the file content and try to convert the database to
 the requested backend type before opening it. In-place upgrades! If
@@ -487,7 +480,6 @@ returns an opaque database structure
 Flags:
 
 -  CYRUSDB\_CREATE - create the database if it doesn't exist
--  CYRUSDB\_MBOXSORT - sort '.' first, so folder listing is correct
 
 Errors:
 

diff --git a/docsrc/imap/developer/API/cyrusdb2.rst b/docsrc/imap/developer/API/cyrusdb2.rst
@@ -143,7 +143,6 @@ returns an opaque database structure
 Flags:
 
 -  CYRUSDB\_CREATE - create the database if it doesn't exist
--  CYRUSDB\_MBOXSORT - sort '.' first, so folder listing is correct
 
 Errors:
 

diff --git a/docsrc/imap/developer/thoughts/improved_mboxlist_sort.rst b/docsrc/imap/developer/thoughts/improved_mboxlist_sort.rst
diff --git a/docsrc/imap/download/release-notes/3.6/x/3.6.0.rst b/docsrc/imap/download/release-notes/3.6/x/3.6.0.rst
@@ -132,6 +132,7 @@ change during the process.
 * The `reverseuniqueids` :cyrusman:`imapd.conf(5)` option is now deprecated
   and unused.  Reverse UNIQUEID records are now standard and cannot be turned
   off.
+* The `improved_mboxlist_sort` :cyrusman:`imapd.conf(5)` option does nothing.
 
 Security fixes
 ==============
@@ -150,10 +151,7 @@ Significant bugfixes
   XML element instead of text (thanks Дилян Палаузов)
 * Fixed :issue:`3896`: the `-d` (dump) and `-u` (undump) options to
   :cyrusman:`ctl_mboxlist(8)` now correctly dump and undump all fields in
-  mailboxes.db entries.  The intermediary file format is now JSON.  This change
-  makes it possible to follow the procedure
-  for switching `improved_mboxlist_sort` described in
-  :ref:`enabling improved mboxlist sort`
+  mailboxes.db entries.  The intermediary file format is now JSON.
 * Fixed :issue:`4035`: `ctl_cyrusdb -r` now recovers from mailboxes.db
   records with missing uniqueids, instead of crashing.  A new `-P` option to
   :cyrusman:`reconstruct(8)` enables repairing mailboxes whose header files

diff --git a/docsrc/imap/reference/manpages/systemcommands/cyr_dbtool.rst b/docsrc/imap/reference/manpages/systemcommands/cyr_dbtool.rst
@@ -16,7 +16,7 @@ Synopsis
 
 .. parsed-literal::
 
-    **cyr_dbtool** [ **-C** *config-file* ] [ **-M** ] [ **-n** ] [ **-o** ] [ **-T** ]
+    **cyr_dbtool** [ **-C** *config-file* ] [ **-n** ] [ **-o** ] [ **-T** ]
             *db-file* *db-backend* *action* [ *key* ] [ *value* ]
 
 Description
@@ -68,10 +68,6 @@ Options
 
     |cli-dash-c-text|
 
-.. option:: -M, --improved-mboxlist-sort
-
-    Uses improved MBOX list sort
-
 .. option:: -n, --create
 
     Create the database file if it doesn't already exist.