[PATCH v2 08/10] cli: address: Do not output duplicate addresses

[Michal Sojka]

On Tue, Nov 04 2014, David Bremner wrote:
> Michal Sojka <sojkam1 at 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