Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features"

[David Bremner <david@tethera.net>]

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.

d