[PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[Tomi Ollila]

On Tue, Dec 03 2013, Jani Nikula <jani at nikula.org> wrote:

> On Tue, 03 Dec 2013, Tomi Ollila <tomi.ollila at iki.fi> wrote:
>> On Tue, Dec 03 2013, David Bremner <david at tethera.net> wrote:
>>
>>> The first patch looks ok. I like the new API overall.
>>>
>>> As far as breaking contrib/notmuch-deliver, I'd rather fix
>>> notmuch-insert than put effort into notmuch-deliver at this point. I
>>> guess it could be a rough transition for people running notmuch-deliver
>>> from git.
>>>
>>> Jani Nikula <jani at nikula.org> writes:
>>>
>>>> +/*
>>>> + * XXX: error handling should clean up *all* state created!
>>>> + */
>>> is this a warning to future hackers or some current problem?
>>>
>>>>  notmuch_status_t
>>>> -notmuch_database_open (const char *path,
>>>> -		       notmuch_database_mode_t mode,
>>>> -		       notmuch_database_t **database)
>>>> +notmuch_database_open (notmuch_database_t *notmuch, const char *path,
>>>> +		       notmuch_database_mode_t mode)
>>>>  
>>>> +/* Initialize a new, empty database handle.
>>>> + *
>>>
>>> I wondered about making the new documentation adhere to doxygen?
>>>
>>>
>>>> +    if (notmuch_database_open (notmuch,
>>>> +			       notmuch_config_get_database_path (config),
>>>> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>>>
>>> Would it make any sense to migrate the mode argument to an option
>>> setting while we're messing with the API?  
>>
>> if that, then also database_path to options ?
>
> See my reply to David.

I agree with your explanation there. 

>
>> I also like this suggestion best of all seen so far, but what if:
>>
>>   #define NOTMUCH_MAJOR_VERSION	0
>>   #define NOTMUCH_MINOR_VERSION	17
>>  -#define NOTMUCH_MICRO_VERSION	0
>>  +#define NOTMUCH_MICRO_VERSION	90
>>
>> until changed API/ABI is set into stone (i.e. 0.18.0 at freeze time).
>
> That would require the user to conditional build with:
>
> #if NOTMUCH_CHECK_VERSION(0, 17, 90)
> 	/* building against post-0.17 git master or released 0.18 */
> #else
> 	/* building against 0.17 */
> #endif
>
> instead of the IMHO more natural and accurate:
>
> #if NOTMUCH_CHECK_VERSION(0, 18, 0)
> 	/* building against post-0.17 git master or released 0.18 */
> #else
> 	/* building against 0.17 */
> #endif

True -- I always forget that NOTMUCH_CHECK_VERSION() checks for the
value and *newer*. Although there is slight difference I don't think
it warrants the use of intermediate values -- the changing API is
much bigger issue than this and that is what we have to concentrate on.
>
>
> BR,
> Jani.

Tomi