On Tue, Nov 04 2014, David Bremner wrote: > Michal Sojka <sojk...@fel.cvut.cz> writes: > >> >> +/* Returns TRUE iff name and addr is duplicate. */ > > If you're revising this patch, it would be good to mention the side > effect of this function. > >> -process_address_list (const search_context_t *ctx, InternetAddressList >> *list) >> +process_address_list (const search_context_t *ctx, >> + InternetAddressList *list) > > It probably doesn't make any difference, but this looks like a needless > whitespace change. > > This function definitely needs some comment / pointer to > documention. And probably not to have _my in the name. > >> +static void >> +_my_talloc_free_for_g_hash (void *ptr) >> +{ >> + talloc_free (ptr); >> +} >> + > > I don't understand the name of the next subtest > >> +test_begin_subtest "No --output" >> +notmuch address --output=sender --output=recipients '*' >OUTPUT
This should be "notmuch address '*' >OUTPUT". I'll fix that. >> +# Use EXPECTED from previous subtest >> +test_expect_equal_file OUTPUT EXPECTED >> + >> + >> +test_done > > nitpick, extra blank lines > > So, AIUI, this is all of the series proposed for 0.19. Agreed. > It looks close to OK to me, modulo some minor style nits. One > anonymous commentator on IRC mentioned the use of module scope > variables, I guess in patch 6/10. I'm not sure of a better solution, > but it's true in a perfect world we wouldn't have module local state. A possible solution would be fill in common_options structure programmatically, but this would make the code much less readable. I can think of a few other solutions but none of them would fit into "perfect world" :) I'll send updated patches in the evening (CET timezone). Thanks -Michal _______________________________________________ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch