From: Tomi Ollila Date: Wed, 23 Oct 2013 21:34:17 +0000 (+0300) Subject: Re: [PATCH 1/3] cli: add insert --must-index option X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=83750e2eac90d7524c14e719d7421817e0090418;p=notmuch-archives.git Re: [PATCH 1/3] cli: add insert --must-index option --- diff --git a/2e/52704517ea94c84832c4f2acd89adac379a300 b/2e/52704517ea94c84832c4f2acd89adac379a300 new file mode 100644 index 000000000..957c01dea --- /dev/null +++ b/2e/52704517ea94c84832c4f2acd89adac379a300 @@ -0,0 +1,245 @@ +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 50E84431FCF + for ; Wed, 23 Oct 2013 14:34:34 -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 1Xefq4sItd1v for ; + Wed, 23 Oct 2013 14:34:28 -0700 (PDT) +Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34]) + by olra.theworths.org (Postfix) with ESMTP id B7D02431FB6 + for ; Wed, 23 Oct 2013 14:34:27 -0700 (PDT) +Received: from guru.guru-group.fi (localhost [IPv6:::1]) + by guru.guru-group.fi (Postfix) with ESMTP id 533BC100217; + Thu, 24 Oct 2013 00:34:18 +0300 (EEST) +From: Tomi Ollila +To: Austin Clements +Subject: Re: [PATCH 1/3] cli: add insert --must-index option +In-Reply-To: <20131023193209.GF20337@mit.edu> +References: <1374365254-13227-1-git-send-email-novalazy@gmail.com> + <87ip048gbj.fsf@qmul.ac.uk> + <20130727151510.GA13750@hili.localdomain> + <87hadtxfrr.fsf@qmul.ac.uk> + <20130912001349.GA18821@hili.localdomain> + <87zjqhv264.fsf@zancas.localnet> + <87bo2xtdp2.fsf@unb.ca> + + <20131023193209.GF20337@mit.edu> +User-Agent: Notmuch/0.16+115~g11c2ff5 (http://notmuchmail.org) Emacs/24.3.1 + (x86_64-unknown-linux-gnu) +X-Face: HhBM'cA~ +MIME-Version: 1.0 +Content-Type: text/plain; charset=utf-8 +Content-Transfer-Encoding: quoted-printable +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: Wed, 23 Oct 2013 21:34:34 -0000 + +On Wed, Oct 23 2013, Austin Clements wrote: + +> Quoth Tomi Ollila on Oct 23 at 10:05 pm: +>> On Thu, Oct 10 2013, David Bremner wrote: +>>=20 +>> > Tomi Ollila writes: +>> >>> I'm not opposed to doing an SONAME bump for 0.17. Are there other ABI +>> >>> breaking changes that we have been holding back on? Can these maybe = +go +>> >>> through at the same time? +>> >> +>> >> Maybe something along these lines... +>> >> +>> >> (Quick draft for the API part; to start discussion before working too= + much +>> >> for something that is going to be dropped...) +>> >> +>> >> notmuch_status_t +>> >> -notmuch_database_create (const char *path, notmuch_database_t **data= +base); +>> >> +notmuch_database_create (const char *path, +>> >> + notmuch_loghook_t *loghook, +>> >> + notmuch_database_t **database); +>> > +>> > Another idea floated (by Austin?) was to pass in an options struct, to +>> > allow future options to be added without changing the function +>> > signature. I guess with some care this could be done in an upwardly +>> > compatible way. +>>=20 +>> Maybe something like +>>=20 +>> #define NOTMUCH_API_OPTIONS_VERSION 1 +>> typedef struct { +>> int options_version; +>> void (*log)(void *, int level, int status, const char * format, = +...); +>> void * logdata; +>> } notmuch_options_t;=20=20=20=20=20=20=20=20 +>>=20 +>> ... +>>=20 +>> notmuch_status_t +>> notmuch_database_create (const char *path, +>> notmuch_options_t *options, +>> notmuch_database_t **database); +>> ... +>>=20 +>> notmuch_status_t +>> notmuch_database_open (const char *path, +>> notmuch_database_mode_t mode, +>> notmuch_options_t *options, +>> notmuch_database_t **database); +>>=20 +>> then in use: +>>=20 +>> notmuch_options_t options =3D { +>> .options_version =3D NOTMUCH_API_OPTIONS_VERSION, +>>=20 +>> .. =C3=A4sh, this has problem that the macro changes in header file +>> but the structure initialization is not following automatically +>> (in other fields than that). Therefore perhaps "fixing" +>> the version macros: +>>=20 +>> #define NOTMUCH_API_OPTIONS_VERSION_1 1 +>> /* #define NOTMUCH_API_OPTIONS_VERSION_2 2 // added in the future */ +>>=20 +>> notmuch_options_t options =3D { +>> .options_version =3D NOTMUCH_API_OPTIONS_VERSION_1, +>> .log =3D log_to_stderr, +>> .logdata =3D NULL +>> }; +>>=20 +>> Well, this is one idea (does not sound as good as I initially +>> thought...) how does other software tackle this kind of issues... +>>=20 +>> If something like this is finally chosen we could provide easy +>> transition path to allow NULL as options -- making notmuch CLI +>> behave as it used to be (log to stderr...). +> +> Using a version number on the options makes it tricky to maintain +> backwards compatibility, since you need code to read all past versions +> of the structure. A similar but, I think, better way would be to take +> the size of the structure in the structure. Something like: +> +> typedef struct { +> size_t options_length; +> ... +> } notmuch_options_t; + +I thought this option too, but went to this version number scheme +for more possibilities. We could discipline the backwards compatibility +by always expecting old fields being the same and requiring structure +to extend when version changes -- but could divert from that if absolutely +necessary... But this would minimally require storing all the version +structures and have version mapping there... But as this is more complex +than your suggestion let's think it as the current solution of choice... + +> #define NOTMUCH_OPTIONS_INIT { sizeof(notmuch_options_t) } +> // or without pre-processor, but requiring GCC attributes +> static __attribute__((__unused__)) +> notmuch_options_t notmuch_options_init =3D { sizeof(notmuch_options_t) }; +> +> NOTMUCH_OPTIONS_INIT/notmuch_options_init could also contain other +> defaults, though it's best if the defaults are simply the zero +> initializations of the various fields. + +Perhaps we should do=20 +#define NOTMUCH_OPTIONS_INIT { .options_length =3D sizeof (notmuch_options_= +t) } + +IIRC this C99 initialization format makes rest of the structure initialize +to zeros (I don't know/remember whether this is true in the one you +suggested). + +Whenever new fields are added to the options structure they will always be +zero in old code that doesn't know those fields -- therefore zero needs +always be sensible default for every field. If we want to support setting +new fields based on their existence then we need to add something like +#define NOTMUCH_OPTIONS_VER 1 (or 20131024) so that code can #ifdef setting +those and still support older notmuch versions... + + +> Then, in code calling libnotmuch, you'd do something like +> +> notmuch_options_t options =3D NOTMUCH_OPTIONS_INIT; +> options.log =3D log_to_stderr; +> // ... +> +> +> And in libnotmuch, we would do something like +> +> notmuch_status_t +> notmuch_database_open (const char *path, +> notmuch_database_mode_t mode, +> const notmuch_options_t *options, +> notmuch_database_t **database) + +Ah, the 'const'. I had the same in mind but forgot when doing it... + +> { +> notmuch_option_t real_options =3D NOTMUCH_OPTIONS_INIT; +> if (real_options.options_length < options.options_length) +> return error; +> memmove(&real_options, options, options.options_length); +> // ... +> } +> +> This approach makes it free to add fields and we can always deprecate +> fields as long as we leave the space in the structure. This is based +> roughly on how the Linux kernel deals with various structures that +> have grown over time in the syscall ABI. + +Ack, good reference. + +> Another much more verbose but also more robust approach would be to +> make notmuch_options_t opaque and provide setters (and getters, if +> we're feeling benevolent). I kind of like the simplicity of a struct. +> +> One thing to consider is what works best with bindings. Presumably +> the opaque structure wouldn't be a problem. I've never had to wrap +> anything like the struct approach, though, so I don't know if that's +> easy or hard. + +Ruby & Go bindings use C wrapper(s), Python ctypes(*). Attaching working=20 +function pointer (& possibly data) can provide to be nontrivial challenge ;/ +We can provide some support code and/or structures in the library (in +addtion/place of options as NULL... + +We'd like to hear comments from bindings' experts/developers. + +Tomi + + +(*) A small attempt to show how c structures are available in python ctypes= +:=20 + +from ctypes import Structure, c_size_t + +class notmuch_options_t(Structure): + _fields__ =3D [ ('options_length', c_size_t), ... ] + +... adding function pointer to the Structure goes beyond my +current skills (we cannot assume function pointer has the same size as +(void) data pointer...) +