On Mon, 11 Apr 2016, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote: > On Sun 2016-04-10 20:33:02 -0400, David Bremner <david@tethera.net> wrote: >> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes: >> >>> To fully complete the ghost-on-removal-when-shared-thread-exists >>> proposal, we need to clear all ghost messages when the last active >>> message is removed from a thread. >> >> For me this patch breaks T530 as follows >> >> T530-upgrade: Testing database upgrade >> FAIL ghost message retains thread ID >> --- T530-upgrade.13.expected 2016-04-11 00:25:19.482196274 +0000 >> +++ T530-upgrade.13.output 2016-04-11 00:25:19.482196274 +0000 >> @@ -1 +1 @@ >> -thread:000000000000001d >> +thread:0000000000000011 >> No new mail. >> No new mail. Removed 1 message. >> >> I pushed my rebased version of the patches to >> >> https://git.notmuchmail.org/git?p=notmuch;a=shortlog;h=refs/heads/thread-fix >> >> in case the problem is some mistake on my part. > > After having done "make download-test-databases", I can confirm that > this is happening for me as well: it's not an issue with bremner's > config. > > however, i think this particular test is wrong, and should probably be > removed. > > For review, here's the final test: > > ---------- > # Ghost messages are difficult to test since they're nearly invisible. > # However, if the upgrade works correctly, the ghost message should > # retain the right thread ID even if all of the original messages in > # the thread are deleted. That's what we test. This won't detect if > # the upgrade just plain didn't happen, but it should detect if > # something went wrong. > test_begin_subtest "ghost message retains thread ID" > # Upgrade database > notmuch new > # Get thread ID of real message > thread=$(notmuch search --output=threads id:4EFC743A.3060609@april.org) > # Delete all real messages in that thread > rm $(notmuch search --output=files $thread) > notmuch new > # "Deliver" ghost message > add_message '[subject]=Ghost' '[id]=4EFC3931.6030007@april.org' > # If the ghost upgrade worked, the new message should be attached to > # the existing thread ID. > nthread=$(notmuch search --output=threads id:4EFC3931.6030007@april.org) > test_expect_equal "$thread" "$nthread" > ------------- > > I don't think this reasoning is sensible. If the entire thread is > deleted, and a new message comes in, it should *not* get the same mesage > ID. ghosts should only exist in the database when other messages point > to them. > > So i'd be fine with killing this entire last test, unless someone can > propose a good reason to keep it. I agree that it's fine to remove this test. From what I recall, it wasn't intended to test that ghost messages retained their thread IDs; this was just the implementation property it exploited to test that ghosts were working. I haven't looked through this series, but it would be nice if there was still some tests that ghosts were working across upgrade.