From: Michal Sojka <sojkam1@fel.cvut.cz>
To: Carl Worth <cworth@cworth.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v4 0/4] Maildir synchronization
In-Reply-To: <877hgsrjau.fsf@yoom.home.cworth.org>
References: <87tyk3vpxd.fsf@wsheee.2x.cz>
	<1288560558-18915-1-git-send-email-sojkam1@fel.cvut.cz>
	<877hgsrjau.fsf@yoom.home.cworth.org>
User-Agent: Notmuch/0.4-30-ga01dbf0 (http://notmuchmail.org) Emacs/23.2.1
	(i486-pc-linux-gnu)
Date: Sun, 07 Nov 2010 02:46:08 +0100
Message-ID: <87d3qhq527.fsf@resox.2x.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Thu, 04 Nov 2010, Carl Worth wrote:
> Meanwhile, here are some of the things I'm still thinking about in
> regards to this patch. First, the commit message describes the
> synchronization happening at "notmuch new" and "notmuch tag/notmuch
> restore". But the implementation shows that the functionality is within
> the library, not the command-line tool above it.
> 
> I think having this at the library makes sense, but as you certainly
> noticed, the library has historically been entirely unaware of any
> configuration, (which I'd like to keep). Obviously, you maintained that
> separation in your patch series, but you added a new
> notmuch_database_set_maildir_sync function so that the library can be
> informed of the desired behavior.
> 
> I'm not entirely sure I like a big, global state-changing function like
> that in the library. But if we do want to have that, we need to fix the
> documentation of all functions that are affected by it to correctly
> document their current behavior.

I can imagine two other solutions. One would be to add a parameter
(probably called flags) to the following functions:

    notmuch_message_add_tag()
    notmuch_message_remove_tag()
    notmuch_message_thaw()

Since you want to keep ABI this would require to implement second
versions of these functions to keep backward compatibility.

The other option would be to put a flag into notmuch_message_t but this
is probably not very different from having it in notmuch_database_t.

> Also, the synchronization is inherently 2-way, but the two directions
> are implemented differently in the library. One direction, (from tags to
> maildir flags), is implemented implicitly in the library (existing
> functions do the new synchronization under the influence of
> database_set_maildir_sync). But the other direction, (from maildir flags
> to tags), requires an explicit call to a new function
> (notmuch_message_maildir_to_flags). I definitely don't like this.

I think I could make notmuch_message_maildir_to_flags() private and call
it from notmuch_database_add_message() so that both directions will be
implemented in the library.
 
> Finally, the documentation for notmuch_message_maildir_to_tags ("Add or
> remove tags based on the maildir flags in the file name")
> inadequate. This documentation needs to say which tags are added/removed
> in what conditions. It should also give guidance on when this function
> should be called in order to achieve some particular behavior.
> 
> I recognize that in the above I don't give specific guidance on whether
> the new functionality should be implicit or explicit in the
> library. I'm not certain which is better, and I'm willing to listen to
> justification from someone who has spent some time implementing and
> testing this stuff. But I don't like the current mixed state.

I found the following what I'd like the most: Do not export
notmuch_message_maildir_to_tags() from the library and call it
implicitly from notmuch_database_add_message(). Keep
notmuch_database_set_maildir_sync() and update the documentation of the
affected functions. This sounds to me better than adding explicit flags
parameters to enable/disable synchronization to all of the affected
functions. The additional parameters would make the API harder to use.

> One other issue, how does this support deal with multiple files that
> have the same message ID (and hence a single record in the database)?
> Some bad failure modes I can imagine are cycling of tags/filenames with
> successive runs of notmuch new, or "leaking"[*] of tags from one filename
> to another through the database.
> 
> [*] Imagine, for example, someone using an external client that
> identifies duplicate messages in the mailstore and adds the maildir flag
> corresponding to "deleted" to all but one of each of the
> duplicates. There's then the possibility that notmuch could propagate
> this "deleted" flag through the "deleted" tag in the database, (perhaps
> after a notmuch dump/restore cycle). And that could be a catastrophic
> result if all messages that have duplicates get flagged for deletion!
> 
> What thoughts do you have on this multiple-file issue?

The current implementation renames only the file whose name is stored
first in the database. I have a TODO comment there to add a loop through
all file names, but I have never realized that deleted flag could be so
dangerous.

I will need to extend the test suite for tests with multiple messages
having the same id and during that work I'll hopefully find some sane
solution.

It will take me probably a few days until I find time to work on this.
So let me now in that time whether you have some preferences in the
above.

-Michal