Date: Sun, 24 Aug 2014 18:16:29 -0400
From: Austin Clements <amdragon@mit.edu>
To: David Bremner <david@tethera.net>
Subject: Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained
	"features"
Message-ID: <20140824221629.GP3015@mit.edu>
References: <1406859003-11561-1-git-send-email-amdragon@mit.edu>
	<1406859003-11561-5-git-send-email-amdragon@mit.edu>
	<87ppfqsv8s.fsf@maritornes.cs.unb.ca>
	<87fvgmg0tx.fsf@awakening.csail.mit.edu>
	<87zjeu4jyp.fsf@maritornes.cs.unb.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87zjeu4jyp.fsf@maritornes.cs.unb.ca>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth David Bremner on Aug 23 at  8:58 pm:
> Austin Clements <amdragon@mit.edu> writes:
> 
> >>> @@ -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 ;).
> >
> > I'm not quite sure what you mean.  notmuch_database_needs_upgrade
> > returns false for read-only databases because you can't upgrade a
> > read-only database.  This was true before this patch, too, though it was
> > less obvious.  (Maybe that's not what you're asking?)
> 
> Yes, that's what I was asking. I guess it's orthogonal to your patch
> series, but the logic of returning FALSE for read only databases is not
> very intuitive to me (in the sense that "needs upgrade" is not the
> opposite of "can't be upgraded".  Your new comment later in the series
> is better, but it would IMHO be even better if you mentioned the read
> only case.

That's a good point.  Ideally this should be
"notmuch_database_can_upgrade" or something, which I think would
capture exactly what it means after this series.  However, I don't
think it's worth breaking API compatibility given that this is already
how callers use this function (even though that violates the current
library spec).

So, how's this for a more updated doc comment on needs_upgrade?

/**
 * Can the database be upgraded to a newer database version?
 *
 * If this function returns TRUE, then the caller may call
 * notmuch_database_upgrade to upgrade the database.  If the caller
 * does not upgrade an out-of-date database, then some functions may
 * fail with NOTMUCH_STATUS_UPGRADE_REQUIRED.  This always returns
 * FALSE for a read-only database because there's no way to upgrade a
 * read-only database.
 */