From: Jani Nikula <jani@nikula.org>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [RFC PATCH 4/5] cli: change the data structure for notmuch
 address deduplication
In-Reply-To: <87a8t9tuka.fsf@maritornes.cs.unb.ca>
References: <cover.1440859765.git.jani@nikula.org>
 <ffe5975fd070d3e8e22602b01ceb8bf0d7d47ae0.1440859765.git.jani@nikula.org>
 <87a8t9tuka.fsf@maritornes.cs.unb.ca>
User-Agent: Notmuch/0.20.2+66~gb33abd9 (http://notmuchmail.org) Emacs/24.4.1
 (x86_64-pc-linux-gnu)
Date: Sun, 30 Aug 2015 10:45:46 +0300
Message-ID: <87h9nh1aol.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Sun, 30 Aug 2015, David Bremner <david@tethera.net> wrote:
> Jani Nikula <jani@nikula.org> writes:
>
>>  
>> +static int
>> +strcase_equal (const void *a, const void *b)
>> +{
>> +    return strcasecmp (a, b) == 0;
>> +}
>> +
>> +static unsigned int
>> +strcase_hash (const void *ptr)
>> +{
>> +    const char *s = ptr;
>> +
>> +    /* This is the djb2 hash. */
>> +    unsigned int hash = 5381;
>> +    while (s && *s) {
>> +	hash = ((hash << 5) + hash) + tolower (*s);
>> +	s++;
>> +    }
>> +
>> +    return hash;
>> +}
>> +
>
> as discussed, these functions probably need to be factored out into
> libutil.

Ack.

>
>> +	l = g_list_find_custom (list, mailbox, mailbox_compare);
>> +	if (l) {
>> +	    talloc_free (mailbox);
>> +	    mailbox = l->data;
>> +	    mailbox->count++;
>> +	    return TRUE;
>> +	}
>
> I found this use of mailbox as a temporary variable confusing; despite
> the obvious return I thought it might have something to do with the
> g_list_append below.  Maybe just make a block scope temporary variable?

This is how the function would turn out with that. Better, I guess? I
also tried to think of ways to combine the two g_list_append paths here,
but in the end doing it like this has most clarity I think.

static notmuch_bool_t
is_duplicate (const search_context_t *ctx, const char *name, const char *addr)
{
    char *key;
    GList *list, *l;
    mailbox_t *mailbox;

    list = g_hash_table_lookup (ctx->addresses, addr);
    if (list) {
	mailbox_t find = {
	    .name = name,
	    .addr = addr,
	};

	l = g_list_find_custom (list, &find, mailbox_compare);
	if (l) {
	    mailbox = l->data;
	    mailbox->count++;
	    return TRUE;
	}

	mailbox = new_mailbox (ctx->format, name, addr);
	if (! mailbox)
	    return FALSE;

	g_list_append (list, mailbox);
	return FALSE;
    }

    key = talloc_strdup (ctx->format, addr);
    if (! key)
	return FALSE;

    mailbox = new_mailbox (ctx->format, name, addr);
    if (! mailbox)
	return FALSE;

    list = g_list_append (NULL, mailbox);
    if (! list)
	return FALSE;

    g_hash_table_insert (ctx->addresses, key, list);

    return FALSE;
}

>> +
>> +	g_list_append (list, mailbox);
>> +	return FALSE;
>>      }
>>  
>> -    mailbox = new_mailbox (ctx->format, name, addr);
>> -    if (! mailbox)
>> +    key = talloc_strdup (ctx->format, addr);
>> +    if (! key)
>>  	return FALSE;
>
> I guess this doesn't make the error handling worse; both old and new
> code silently ignore OOM if I understand correctly. Do you happen to
> understand the original choice of using ctx->format rather than that
> ctx->notmuch for a talloc parent? it doesn't seem to get deallocated any
> earlier.

I don't know or understand that part of the history. It doesn't really
matter though because the deallocation is explicitly done on all keys
and values via g_hash_table_unref.

BR,
Jani.