From: David Bremner <david@tethera.net>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [RFC PATCH 4/5] cli: change the data structure for notmuch
 address deduplication
In-Reply-To:
 <ffe5975fd070d3e8e22602b01ceb8bf0d7d47ae0.1440859765.git.jani@nikula.org>
References: <cover.1440859765.git.jani@nikula.org>
 <ffe5975fd070d3e8e22602b01ceb8bf0d7d47ae0.1440859765.git.jani@nikula.org>
User-Agent: Notmuch/0.20.2+60~gcb08a2e (http://notmuchmail.org) Emacs/24.5.1
 (x86_64-pc-linux-gnu)
Date: Sat, 29 Aug 2015 22:48:53 -0300
Message-ID: <87a8t9tuka.fsf@maritornes.cs.unb.ca>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

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.

> +	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?

> +
> +	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.

d