From: Carl Worth <cworth@cworth.org>
To: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>,
	notmuch@notmuchmail.org
In-Reply-To:
 <1258914526-30984-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
References:
 <1258914526-30984-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
Date: Mon, 23 Nov 2009 04:51:34 +0100
Message-ID: <87pr79gapl.fsf@yoom.home.cworth.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: Re: [notmuch] [RFC PATCH -V2] notmuch: Add support for multiple
 maildirs
Precedence: list

On Sun, 22 Nov 2009 23:58:46 +0530, "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> wrote:
> This patch separate database path and maildir paths.
> It also adds support for multiple maildir paths which
> is represented by comma separated values.

I have a running joke with my good friend Behdad that we should add a
git-hook to refuse any commit with the word "also" in the commit
message. It's often a sign that the commit is actually doing two
independent things, and that those two independent things should be
split into separate commits.

I think that's the case here.

> You need to have in ~/.notmuch-config
> [maildirs]
> path=path1,path2

Documentation in the commit message doesn't count. We need this
documentation to be inserted into the configuration file as a comment,
like with the other values.

> +	     dirs = g_key_file_get_string (config->key_file,
> +				     "maildirs", "path", NULL);
> +		if (dirs) {
> +			size = sizeof(struct count_ele) + strlen(dirs) + 1;
> +			/* comma separated paths */
> +			maildirs = (struct count_ele *)malloc(size);

There's a g_key_file_get_string_list function that will do all the
splitting of a multiple-valued configuration entry into multiple
strings. I suggest using that here instead of doing open-coded parsing.

> -    prompt ("Top-level directory of your email archive [%s]: ",
> +    prompt ("Directory for notmuch database [%s]: ",

Is there really a reason to prompt for this here, rather than just using
a default value (and letting the user know what it is?). One thing I'm
worried about is the user thinking they can run "notmuch setup", change
this value, and expect things to work, (which of course, they won't).

I'd be happier if the user understood that the database is relocatable
and they can just "mv" things around as they want to.

Of course, for that, then we'd actually need to *make* the database
relocatable. One thing that we get right now with the .notmuch directory
_inside_ the mail directory is that everything is relocatable.

If you just pull it out, like you do here, then suddenly the database
will be full of absolute paths to mail files, and things will break if
the top-level mail-directory is moved. And that's a regression compared
to the current case.

> +    prompt ("Comma separated maildirs [%s]: ", cmaildirs);

If we are going to support multiple directories for mail here, then we
should prompt separately for each (like we are doing for the email
addresses already).

Also, I'd prefer something like "Top-level directory of email archive"
like we had before. I certainly don't want to give the user the
impression that they need to type in a path to each individual maildir
that they have.

Finally, (and maybe I should have started with this), what's the actual
use case here? Is it difficult to just arrange all mail to be under one
top-level directory, (perhaps just using symlinks even).

-Carl