From: Tomi Ollila <tomi.ollila@iki.fi>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: WIP: add metadata to dump output
In-Reply-To: <1452394301-29499-1-git-send-email-david@tethera.net>
References: <1452394301-29499-1-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.21+32~g73439f8 (http://notmuchmail.org) Emacs/24.3.1
 (x86_64-unknown-linux-gnu)
Date: Sun, 10 Jan 2016 16:36:59 +0200
Message-ID: <m2y4bxcxh0.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Sun, Jan 10 2016, David Bremner <david@tethera.net> wrote:

> It seems (at least to me) that xapian metadata is the right way store
> certain configuration data, including tag aliases [1] and perhaps some
> non-CLI specific configuration. On the other hand we don't want to
> have things lost if we dump and restore a database. Hence this series,
> which is a start at dumping and restore such config.
>
> The main idea here is that various classes of metadata can be defined
> by using prefixes, in exactly the same way as tags are defined for
> documents. This will hopefully help prevent e.g. config from stomping
> on tag aliases.
>
> The first 6 patches impliment iterators for simple "queries" on
> metadata. They are probably split a bit fine, but that's the way I
> developed them.
>
> The last 3 impliment the printing of metadata in dump output. In order
> to be upwardly compatible, it uses the old dodge of hiding things in
> comments. In fact the comment syntax (# in first column) was never
> well documented; this does mean that the notmuch dump output can be
> tested without breaking the current restore tests. I threw an @ in to
> help autodetection of formats; obviously this is not foolproof. On the
> other hand, I don't know how much people currently rely on comments in
> dump files, since notmuch doesn't generate them.
>
> There's lots of bikes to shed here. Probably the most important bits
> are the library API, the dump output format, and of course the ever
> tricky command line argument names.

Generally this series looks pretty good. IMO this could have gone with
way less separate patches -- It would have made the review easier,
now I had to go back to previous mails just to look context. But,
anyone who disagrees w/ this make David know (in any appropriate 
channel so my opinion does not get too emphasized ;D)

The first thing that came into my mind was this naming of
*_FIRST_CLASS and *_LAST_CLASS in enum values. the naming
is inconsistent in sense that first is first, but last is last + 1.
Unfortunately there is nothing we can do with that as these *_LAST_*
are used in other enums too so we just have to live with it. 

In last in this series there is
+typedef enum dump_includes {
+    DUMP_INCLUDE_TAGS=1,
+    DUMP_INCLUDE_METADATA=2,
+} dump_include_t;

-- spacing around ' = ' missing -- I did not see other whitespace errors
(not that there might not be those, though, as we know David ;)

One bug I found:

+    for (mclass = NOTMUCH_METADATA_FIRST_CLASS; mclass < NOTMUCH_METADATA_LAST_CLASS; mclass++) {
+	status = notmuch_database_get_all_metadata (notmuch, NOTMUCH_METADATA_CONFIG, &meta);

(mclass should be there). Currently as there is only that one in the enum
there is no problem -- also for the same reason current test can not 
notice this. If this were not fixed, this would be noticed in the future
by that particular test - unless it is changed erronelously ;)


Anyway, good stuff in general...


Tomi

> Getting the memory ownership semantics is tricky, especially with the
> mix of C++ objects and talloc. So I'd appreciate a critical eye on
> those bits of metadata.cc.

uh puh -- maybe I look that again (hmm, have to apply the patch series as
all of the metadata.cc is not in one patch ;/