From eda61398dc4f74cbe067f4c07b1bf10bb627cf0f Mon Sep 17 00:00:00 2001 From: David Bremner Date: Mon, 25 Aug 2014 16:03:23 +1700 Subject: [PATCH] Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features" --- 0a/887c12d0f7b00c96ab8dfa41e7420084e16d51 | 97 +++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 0a/887c12d0f7b00c96ab8dfa41e7420084e16d51 diff --git a/0a/887c12d0f7b00c96ab8dfa41e7420084e16d51 b/0a/887c12d0f7b00c96ab8dfa41e7420084e16d51 new file mode 100644 index 000000000..8122e8220 --- /dev/null +++ b/0a/887c12d0f7b00c96ab8dfa41e7420084e16d51 @@ -0,0 +1,97 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 14548431FB6 + for ; Sun, 24 Aug 2014 16:03:33 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: 0 +X-Spam-Level: +X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none] + autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id 5lcIJk2kWzhW for ; + Sun, 24 Aug 2014 16:03:29 -0700 (PDT) +Received: from yantan.tethera.net (yantan.tethera.net [199.188.72.155]) + (using TLSv1 with cipher DHE-RSA-AES128-SHA (128/128 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id 37412431FAE + for ; Sun, 24 Aug 2014 16:03:29 -0700 (PDT) +Received: from remotemail by yantan.tethera.net with local (Exim 4.80) + (envelope-from ) + id 1XLgoq-0007Al-1X; Sun, 24 Aug 2014 20:03:28 -0300 +Received: (nullmailer pid 5134 invoked by uid 1000); Sun, 24 Aug 2014 + 23:03:23 -0000 +From: David Bremner +To: Austin Clements +Subject: Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained + "features" +In-Reply-To: <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> <20140824221629.GP3015@mit.edu> +User-Agent: Notmuch/0.18.1+72~g028c560 (http://notmuchmail.org) Emacs/24.3.1 + (x86_64-pc-linux-gnu) +Date: Sun, 24 Aug 2014 16:03:23 -0700 +Message-ID: <871ts5mqx0.fsf@maritornes.cs.unb.ca> +MIME-Version: 1.0 +Content-Type: text/plain +Cc: notmuch@notmuchmail.org +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Sun, 24 Aug 2014 23:03:33 -0000 + +Austin Clements writes: + +> Quoth David Bremner on Aug 23 at 8:58 pm: +>> Austin Clements 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 -- 2.26.2