Re: [PATCH 1/7] Split notmuch_database_close into two functions

[Mark Walters <markwalters1009@gmail.com>]

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.