On Mon, 16 Apr 2012, Justus Winter <4win...@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. _______________________________________________ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch