From: Tomi Ollila <tomi.ollila@iki.fi>
To: Jani Nikula <jani@nikula.org>, David Bremner <david@tethera.net>,
	notmuch@notmuchmail.org
Subject: Re: [PATCH 2/2] lib: introduce notmuch_database_new for initializing
	a database handle
In-Reply-To: <871u1tyhai.fsf@nikula.org>
References: <cover.1385903109.git.jani@nikula.org>
	<fc7ecd990e55fcfba17de4d71e8823c98760f9ce.1385903109.git.jani@nikula.org>
	<87wqjm2m2q.fsf@zancas.localnet>
	<m2iov6m3c7.fsf@guru.guru-group.fi> <871u1tyhai.fsf@nikula.org>
User-Agent: Notmuch/0.17~rc1+17~ga2e1a2f (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-unknown-linux-gnu)
Date: Tue, 03 Dec 2013 21:35:18 +0200
Message-ID: <m2y541pw21.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Tue, Dec 03 2013, Jani Nikula <jani@nikula.org> wrote:

> On Tue, 03 Dec 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>> On Tue, Dec 03 2013, David Bremner <david@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@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