Closed
Bug 532522
Opened 15 years ago
Closed 13 years ago
gloda indexes deleted folders, that are subfolders of trash
Categories
(MailNews Core :: Database, defect)
MailNews Core
Database
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 10.0
People
(Reporter: rkent, Assigned: protz)
References
(Blocks 1 open bug)
Details
(Whiteboard: [gloda key])
Attachments
(1 file, 1 obsolete file)
10.63 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
I have many folders that I previously deleted, that are sitting as subfolders of trash. Gloda should not index them, but does.
Updated•15 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [gloda key]
Updated•14 years ago
|
Blocks: glodafailtracker
Assignee | ||
Comment 1•13 years ago
|
||
I guess you'll want to have tests for that, so just asking for feedback. If not, feel free to r+ :-).
Assignee: nobody → jonathan.protzenko
Status: NEW → ASSIGNED
Attachment #551965 -
Flags: feedback?(bugmail)
Comment 2•13 years ago
|
||
Comment on attachment 551965 [details] [diff] [review] Patch v1 This seems like one of the right places to change the logic, but please use nsIMsgFolder's isSpecialFolder helper which already has the ability to check ancestors: http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgFolder.idl#412 Unfortunately, I believe covering the case rkent mentions requires a bit more legwork. Specifically, deleting a folder by moving it to the trash will likely either trigger folderMoveCopyCompleted or folderRenamed notifications (they confuse me). These will need to make sure to update the folder indexing priority, and potentially need to trigger deletion logic on the messages inside if it constitutes a priority change. And yes, please, I would like a test, as this seems eminently able to be regressed. You will note I have helpfully created stub test functions for you in base_index_messages.js (test_folder_deletion_single/test_folder_deletion_nested). Some might say I prophetically saw the need for these tests, others (more correctly) that I should have written them long ago... Note that you'll need to add them to the list of tests at the bottom of the file since they are not listed.
Attachment #551965 -
Flags: feedback?(bugmail) → feedback+
Assignee | ||
Comment 3•13 years ago
|
||
So the tests were painful for that one. Some remarks: - I added the new test at the bottom of the list, because otherwise, test_pending_commit_tracker_flushes_correctly would complain message headers from the old folder (that was moved) being invalid, and emit an error; the case is perfectly documented in the source, and understood, except it uses ._error instead of ._warn, so well... - I just wrote one test, as the nested case is a strict superset of the simple case, - I had to disable one part of the test unless on local folder injection mode, because the point was precisely to test injecting into these newly moved folders, and since they were not managed by make_sets_in_folder, the test would fail because there was no fake folder associated to them, and we cannot inject directly into imap folders. Also, the documentation pretends that make_sets_in_folders returns a nsIMsgLocalMailFolder which is BALD-FACED LIE! :-D I had to write a wrapper around that to make sure I always give nsIMsgFolders to the msgCopyService. That's pretty much all. The test is pretty intensive, so I believe everything is right...
Attachment #551965 -
Attachment is obsolete: true
Attachment #552721 -
Flags: review?(bugmail)
Comment 4•13 years ago
|
||
Comment on attachment 552721 [details] [diff] [review] Patch v2, with tests async_driver already uses do_execute_soon under the hood, so you don't need to have it do a timeout. (That may not have always been true, but it's been true a long time now.) Your folder moving logic seems like it would do great in messageInjection.js, please move it there. Please remove your pretty color dump you left in. Otherwise, things look fantastic! Thanks!
Attachment #552721 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 5•13 years ago
|
||
http://hg.mozilla.org/comm-central/rev/5273b72d2e58
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
You need to log in
before you can comment on or make changes to this bug.
Description
•