From: David Bremner <david@tethera.net>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 6/9] cli: change the data structure for notmuch
 address deduplication
In-Reply-To:
 <d8bb01aff648f9b73ec5f09f0e86e42dac2a21a6.1441308761.git.jani@nikula.org>
References: <cover.1441308761.git.jani@nikula.org>
 <d8bb01aff648f9b73ec5f09f0e86e42dac2a21a6.1441308761.git.jani@nikula.org>
User-Agent: Notmuch/0.20.2+73~gd432116 (http://notmuchmail.org) Emacs/24.5.1
 (x86_64-pc-linux-gnu)
Date: Thu, 24 Sep 2015 09:32:13 -0300
Message-ID: <87fv242e2a.fsf@zancas.localnet>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

Jani Nikula <jani@nikula.org> writes:


> +    else
> +	v = !!m1->name - !!m2->name;

Is this really idiomatic? It seems a little difficult to follow to me.

>  /* Returns TRUE iff name and addr is duplicate. If not, stores the
>   * name/addr pair in order to detect subsequent duplicates. */
>  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;
>  
> -    key = talloc_asprintf (ctx->format, "%s <%s>", name, addr);
> -    if (! key)
> -	return FALSE;
> +    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 = g_hash_table_lookup (ctx->addresses, key);
> -    if (mailbox) {
> -	mailbox->count++;
> -	talloc_free (key);
> -	return TRUE;
> +	mailbox = new_mailbox (ctx->format, name, addr);
> +	if (! mailbox)
> +	    return FALSE;
> +
> +	/*
> +	 * XXX: It would be more efficient to prepend to the list, but
> +	 * then we'd have to store the changed list head back to the
> +	 * hash table. This check is here just to avoid the compiler
> +	 * warning for unused result.
> +	 */
> +	if (list != g_list_append (list, mailbox))
> +	    INTERNAL_ERROR ("appending to list changed list head\n");
> +
> +	return FALSE;
>      }
>  
> +    key = talloc_strdup (ctx->format, addr);
> +    if (! key)
> +	return FALSE;
> +
>      mailbox = new_mailbox (ctx->format, name, addr);
>      if (! mailbox)
>  	return FALSE;
>  
> -    g_hash_table_insert (ctx->addresses, key, mailbox);
> +    list = g_list_append (NULL, mailbox);
> +    if (! list)
> +	return FALSE;
> +
> +    g_hash_table_insert (ctx->addresses, key, list);
>  
>      return FALSE;
>  }
> @@ -401,12 +445,21 @@ _talloc_free_for_g_hash (void *ptr)
>  }
>  
>  static void
> -print_hash_value (unused (gpointer key), gpointer value, gpointer user_data)
> +_list_free_for_g_hash (void *ptr)
> +{
> +    g_list_free_full (ptr, _talloc_free_for_g_hash);
> +}
> +
> +static void
> +print_list_value (void *mailbox, void *context)
>  {
> -    const mailbox_t *mailbox = value;
> -    search_context_t *ctx = user_data;
> +    print_mailbox (context, mailbox);
> +}
>  
> -    print_mailbox (ctx, mailbox);
> +static void
> +print_hash_value (unused (void *key), void *list, void *context)
> +{
> +    g_list_foreach (list, print_list_value, context);
>  }
>  
>  static int
> @@ -792,8 +845,9 @@ notmuch_address_command (notmuch_config_t *config, int argc, char *argv[])
>  				 argc - opt_index, argv + opt_index))
>  	return EXIT_FAILURE;
>  
> -    ctx->addresses = g_hash_table_new_full (g_str_hash, g_str_equal,
> -					    _talloc_free_for_g_hash, _talloc_free_for_g_hash);
> +    ctx->addresses = g_hash_table_new_full (strcase_hash, strcase_equal,
> +					    _talloc_free_for_g_hash,
> +					    _list_free_for_g_hash);
>  
>      ret = do_search_messages (ctx);
>  
> -- 
> 2.1.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch