From: Carl Worth <cworth@cworth.org>
To: Mikhail Gusarov <dottedmag@dottedmag.net>, notmuch@notmuchmail.org
In-Reply-To: <1258938805-15961-1-git-send-email-dottedmag@dottedmag.net>
References: <1258938805-15961-1-git-send-email-dottedmag@dottedmag.net>
Date: Thu, 26 Nov 2009 07:40:15 -0800
Message-ID: <87aay9s3a8.fsf@yoom.home.cworth.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: Re: [notmuch] [PATCH] Handle message renames in mail spool
Precedence: list

On Mon, 23 Nov 2009 07:13:25 +0600, Mikhail Gusarov <dottedmag@dottedmag.net> wrote:
> In order to handle message renames the following changes were deemed
> necessary:

Hi Mikhail,

I *really* want this patch in, since I think a lot of current users
would really benefit from it. I only see one big problem with it:

> Note that after applying this patch notmuch still does not handle copying files
> (which is harmless, database will point to the last copy of message found during
> 'notmuch new') and deleting files (which is more serious, as dangling entries
> will show up in searches).
...
> +	} else if (strcmp(notmuch_message_get_filename(message), filename)) {
> +	    /* Message file has been moved/renamed */
> +	    _notmuch_message_set_filename (message, filename);

With multiple copies of the same message being present in the mailstore
as different files, the code above will set the filename of the document
to each filename in turn, correct?

Now, I *think* that Xapian defect #250 doesn't hit us here
currently. But there was an old bug in Xapian that changing just the
data of a document would also rewrite all the terms. But we've also
talked recently about storing the filename as a term as well, and once
we add that, then this code would trigger defect #250 and cause a big
slowdown here.

So I think the code just needs to verify that the old filename no longer
exists before it changes anything in the database. And then if it does
still exist, we can get our NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID return
value back here.

>  	} else {
>  	    ret = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
>  	    goto DONE;
>  	}

In this case, the return value is actually wrong. This is the case for
adding a message file that already exists in the database. I don't know
that we have any users that care about distinguishing this, but if they
did, then DUPLICATE isn't right. I suggest returning
NOTMUCH_STATUS_SUCCESS here.

-Carl