From: David Bremner <david@tethera.net>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained
	"features"
In-Reply-To: <1406859003-11561-5-git-send-email-amdragon@mit.edu>
References: <1406859003-11561-1-git-send-email-amdragon@mit.edu>
	<1406859003-11561-5-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.18.1+72~g028c560 (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-pc-linux-gnu)
Date: Sat, 23 Aug 2014 15:21:07 -0700
Message-ID: <87ppfqsv8s.fsf@maritornes.cs.unb.ca>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

Austin Clements <amdragon@MIT.EDU> writes:
>  
> +    /* Bit mask of features used by this database.  Features are
> +     * named, independent aspects of the database schema.  This is a
> +     * bitwise-OR of NOTMUCH_FEATURE_* values (below). */
> +    unsigned int features;

Should we be using a fixed size integer (uint_32t or whatever) for
features? iirc the metadata in the database is actually a string, so I
guess arbitrary precision there.

> +/* Bit masks for _notmuch_database::features. */
> +enum {
> +    /* If set, file names are stored in "file-direntry" terms.  If
> +     * unset, file names are stored in document data.
> +     *
> +     * Introduced: version 1.  Implementation support: both for read;
> +     * required for write. */
> +    NOTMUCH_FEATURE_FILE_TERMS = 1 << 0,

I agree with Jani that the Implementation support: part is a bit
mystifying without the commit message. Maybe part of the commit message
could migrate here? Or maybe just add a pointer to the comment in database.cc.

> +		if (! *incompat_out)

Should we support passing NULL for incompat_out? or at least check for
it?

> @@ -1048,7 +1164,8 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
>  notmuch_bool_t
>  notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
>  {
> -    return notmuch->needs_upgrade;
> +    return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
> +	(NOTMUCH_FEATURES_CURRENT & ~notmuch->features);
>  }

Maybe I'm not thinking hard enough here, but how does this deal with a
feature that is needed to open a database in read only mode? Maybe it
needs a comment for people not as clever as Austin ;).

d