[PATCH 1/3] cli: add insert --must-index option

[Tomi Ollila]

On Wed, Oct 23 2013, Austin Clements <amdragon at MIT.EDU> wrote:

> Quoth Tomi Ollila on Oct 23 at 10:05 pm:
>> On Thu, Oct 10 2013, David Bremner <david at tethera.net> wrote:
>> 
>> > Tomi Ollila <tomi.ollila at iki.fi> 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 **database);
>> >> +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.
>> 
>> Maybe something like
>> 
>> #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;        
>> 
>> ...
>> 
>> notmuch_status_t
>> notmuch_database_create (const char *path,
>> 			 notmuch_options_t *options,
>> 			 notmuch_database_t **database);
>> ...
>> 
>> notmuch_status_t
>> notmuch_database_open (const char *path,
>>                        notmuch_database_mode_t mode,
>>                        notmuch_options_t *options,
>>                        notmuch_database_t **database);
>> 
>> then in use:
>> 
>> notmuch_options_t options = {
>>        .options_version = NOTMUCH_API_OPTIONS_VERSION,
>> 
>> .. äsh, 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:
>> 
>> #define NOTMUCH_API_OPTIONS_VERSION_1 1
>> /* #define NOTMUCH_API_OPTIONS_VERSION_2 2 // added in the future */
>> 
>> notmuch_options_t options = {
>>        .options_version = NOTMUCH_API_OPTIONS_VERSION_1,
>>        .log = log_to_stderr,
>>        .logdata = NULL
>> };
>> 
>> Well, this is one idea (does not sound as good as I initially
>> thought...) how does other software tackle this kind of issues...
>> 
>> 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 = { 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 
#define NOTMUCH_OPTIONS_INIT { .options_length = 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 = NOTMUCH_OPTIONS_INIT;
> options.log = 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 = 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 
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: 

from ctypes import Structure, c_size_t

class notmuch_options_t(Structure):
    _fields__ = [ ('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...)