From: Mark Walters <markwalters1009@gmail.com>
To: Justus Winter <4winter@informatik.uni-hamburg.de>, notmuch@notmuchmail.org
Subject: Re: [PATCH 1/7] Split notmuch_database_close into two functions
In-Reply-To: <20120416215112.15615.94985@thinkbox.jade-hamburg.de>
References:
 <1332291311-28954-1-git-send-email-4winter@informatik.uni-hamburg.de>
	<1332291311-28954-2-git-send-email-4winter@informatik.uni-hamburg.de>
	<87fwcopu5g.fsf@qmul.ac.uk>
	<20120416215112.15615.94985@thinkbox.jade-hamburg.de>
User-Agent: Notmuch/0.12+110~gbc97b4a (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Tue, 17 Apr 2012 09:37:44 +0100
Message-ID: <87pqb64uuf.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Mon, 16 Apr 2012, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> Quoting Mark Walters (2012-03-31 19:17:15)
>> Secondly, I think the patch series could be made clearer and easier to
>> review. If you do it in three steps
>> 
>> 1) change of notmuch_database_close to notmuch_database_destroy (just
>>    the function name change)
>> 2) split the new notmuch_database_destroy into two as in the current
>>    first patch
>> 3) Make any changes (if there are any) of notmuch_database_destroy to
>>    notmuch_database_close.
>> 
>> The advantage is that the first change is easy to test (essentially does
>> it build) and then changes from notmuch_database_destroy to
>> notmuch_database_close in step 3 are explicit rather than the current
>> situation where we need to grep the code to see if some instances of
>> notmuch_database_close were not changed to notmuch_database_destroy.
>
> I don't buy it. The patch series first touches the library and
> documentation and the lib compiles fine. The next patch updates the
> cli tools, all of them compile fine afterwards.
>
> Every patch addresses the issue component wise, this seems rather
> natural for me.

I will try an explain my concern better. I assume that the patch
actually introduces a functional change : that is something somewhere in
the code calls the new notmuch_database_close instead of
notmuch_database_destroy [1]. In your current patch series someone
reading the patches alone can't see the functional change: it comes from
the occurrences of notmuch_database_close that you *don't* change to
notmuch_database_destroy.

Indeed, if the only change is to allow out-of-tree code access to the
new notmuch_database_close function then doing the patch series as

rename notmuch_database_close to notmuch_database_destroy

Then split notmuch_database_destroy make it clearer. (And if the code
compiles after step 1 then I know *all* occurrences of
notmuch_database_close have been changed to notmuch_database_destroy).


Best wishes

Mark

[1] Apart, of course, from notmuch_database_destroy.