From: Michal Sojka <sojkam1@fel.cvut.cz>
To: Thomas Schwinge <thomas@schwinge.name>, notmuch@notmuchmail.org
Subject: Re: Hallo!, and hooray!, and some first work, too
In-Reply-To: <87tyhhvqa4.fsf@kepler.schwinge.homeip.net>
References: <87tyhhvqa4.fsf@kepler.schwinge.homeip.net>
User-Agent: Notmuch/0.5-38-g923b170 (http://notmuchmail.org) Emacs/23.2.1
	(x86_64-pc-linux-gnu)
Date: Mon, 10 Jan 2011 17:41:16 +0100
Message-ID: <87pqs4k8yb.fsf@steelpick.2x.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Hi,

I went briefly through your "patch". See my comments bellow.

On Sun, 09 Jan 2011, Thomas Schwinge wrote:
> I poked at notmuch-deliver's code two weeks ago already (Ali, beware,
> there'll be some few further patches coming), and in the last hours, I
> finally had a look at notmuch.h and some of the source code.  Here is a
> diff containing some comments, or to-do items.  Not all are fully fledged
> (I have neither been using talloc, nor Xapian before), but perhaps
> someone nevertheless feels like commenting on them.  In general I can
> say, that the notmuch code makes a solid impression.  :-)
> 

[...]

> diff --git a/lib/database.cc b/lib/database.cc
> index 7a00917..b08c429 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -1012,6 +1003,7 @@ _notmuch_database_get_directory_db_path (const char *path)
>   * is an empty string, then both directory and basename will be
>   * returned as NULL.
>   */
> +/* XXX: isn't there a standard libc function that can be used?  */

I do not think so. There may be something in glib.

> @@ -1735,11 +1737,15 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
>  		status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
>  	    }
>  	}
> +        
> +        /* XXX: talloc_free (term); */

This is where talloc helps us - it frees the memory semi-automatically
when talloc_free (local) is called. It seems there is some mistake,
though. It would be IMHO better to fix it by the patch bellow.

diff --git a/lib/database.cc b/lib/database.cc
index 7a00917..289e41c 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1710,7 +1710,7 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
 	if (status)
 	    return status;
 
-	term = talloc_asprintf (notmuch, "%s%s", prefix, direntry);
+	term = talloc_asprintf (local, "%s%s", prefix, direntry);
 
 	find_doc_ids_for_term (notmuch, term, &i, &end);
 


>      } catch (const Xapian::Error &error) {
>  	fprintf (stderr, "Error: A Xapian exception occurred removing message: %s\n",
>  		 error.get_msg().c_str());
>  	notmuch->exception_reported = TRUE;
>  	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
> +
> +        /* XXX: (conditionally) talloc_free (term); */
>      }
>  
>      talloc_free (local);

Also fixed by the above patch.

> diff --git a/lib/directory.cc b/lib/directory.cc
> index 946be4f..925b1da 100644
> --- a/lib/directory.cc
> +++ b/lib/directory.cc
> @@ -140,6 +140,7 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
>  	    char *term = talloc_asprintf (local, "%s%s",
>  					  _find_prefix ("directory"), db_path);
>  	    directory->doc.add_term (term, 0);
> +            /* XXX?: talloc_free (term); */

Handled by talloc_free (local);

> diff --git a/lib/message.cc b/lib/message.cc
> index adcd07d..cf4e36a 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -449,8 +450,10 @@ _notmuch_message_add_filename (notmuch_message_t *message,
>      status = _notmuch_database_filename_to_direntry (local,
>  						     message->notmuch,
>  						     filename, &direntry);
> -    if (status)
> +    if (status) {
> +        /* XXX: talloc_free (local); */

Yes, this is missing here.

>  	return status;
> +    }
>  
>      _notmuch_message_add_term (message, "file-direntry", direntry);
>  
> @@ -730,9 +734,12 @@ _notmuch_message_add_term (notmuch_message_t *message,
>  
>      term = talloc_asprintf (message, "%s%s",
>  			    _find_prefix (prefix_name), value);
> +    /* XXX: term != NULL?  */

I think that on Linux, malloc et al never fail, but the check should be
here to comply with the C standard.

> -    if (strlen (term) > NOTMUCH_TERM_MAX)
> +    if (strlen (term) > NOTMUCH_TERM_MAX) {
> +        /* XXX: talloc_free (term); */

It might be here, but if not, term will be freed with the message.

>  	return NOTMUCH_PRIVATE_STATUS_TERM_TOO_LONG;
> +    }
>  
>      message->doc.add_term (term, 0);
>  
> @@ -820,6 +827,7 @@ notmuch_message_add_tag (notmuch_message_t *message, const char *tag)
>      if (strlen (tag) > NOTMUCH_TAG_MAX)
>  	return NOTMUCH_STATUS_TAG_TOO_LONG;
>  
> +    /* XXX: what if tag is already present -- added again?  */

I think that it is no problem and the function bellow does nothing.

>      private_status = _notmuch_message_add_term (message, "tag", tag);
>      if (private_status) {
>  	INTERNAL_ERROR ("_notmuch_message_add_term return unexpected value: %d\n",
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index e508309..ffc7f8f 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -810,6 +810,7 @@ notmuch_message_set_flag (notmuch_message_t *message,
>   * For the original textual representation of the Date header from the
>   * message call notmuch_message_get_header() with a header value of
>   * "date". */
> +/* XXX: what if Date: was missing?  */

It seems that zero is returned in this case - see _notmuch_message_set_date().

Could you please check that your proposed fixes pass the test suite and
if so send them as individual patches in order to be easily applicable
by Carl?

Thanks,
Michal