Austin Clements <amdra...@mit.edu> writes: > Quoth David Bremner on Aug 23 at 8:58 pm: >> Austin Clements <amdra...@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 _______________________________________________ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch