From: Austin Clements <aclements@csail.mit.edu>
To: Daniel Kahn Gillmor <dkg@fifthhorseman.net>,
 David Bremner <david@tethera.net>, Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH v4 7/7] complete
 ghost-on-removal-when-shared-thread-exists
In-Reply-To: <87zit0t0mj.fsf@alice.fifthhorseman.net>
References: <1459445693-3900-1-git-send-email-dkg@fifthhorseman.net>
 <1460166892-29721-1-git-send-email-dkg@fifthhorseman.net>
 <1460166892-29721-7-git-send-email-dkg@fifthhorseman.net>
 <87r3ed6l35.fsf@zancas.localnet> <87zit0t0mj.fsf@alice.fifthhorseman.net>
User-Agent: Notmuch/0.21+62~g7edba1d (http://notmuchmail.org) Emacs/24.5.1
 (x86_64-pc-linux-gnu)
Date: Tue, 19 Apr 2016 23:36:48 -0400
Message-ID: <87zispne7j.fsf@csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

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.