[PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features"
David Bremner
david at tethera.net
Sun Aug 24 16:03:23 PDT 2014
Austin Clements <amdragon at mit.edu> writes:
> Quoth David Bremner on Aug 23 at 8:58 pm:
>> Austin Clements <amdragon at 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?
LGTM
More information about the notmuch
mailing list