From: Carl Worth <cworth@cworth.org>
To: Michiel Buddingh' <michiel@michielbuddingh.net>, notmuch@notmuchmail.org
In-Reply-To: <9bfdedddeab9c58cd45d8d448323d0fc@localhost>
References: <87fx8bygi7.fsf@linux.vnet.ibm.com>
	<87bpiv4t9h.fsf@yoom.home.cworth.org>
	<87y6lz39nd.fsf@yoom.home.cworth.org>
	<20091121221207.GB17268@jukie.net>
	<9cce5525b093b87fe74d427954ffad89@localhost>
	<87d43b2oif.fsf@yoom.home.cworth.org>
	<9bfdedddeab9c58cd45d8d448323d0fc@localhost>
Date: Wed, 25 Nov 2009 09:52:00 -0800
Message-ID: <87skc23327.fsf@yoom.home.cworth.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: Re: [notmuch] [PATCH] notmuch: Add Maildir directory name as tag
 name for messages
Precedence: list

On Sun, 22 Nov 2009 10:33:53 +0100, Michiel Buddingh' <michiel@michielbuddingh.net> wrote:
> On the positive side, this allows us to map these flags onto tags,
> at least for indexing (the patch at the bottom implements this), and,
> if I can pry you away from your principles, later for modification
> as well.

Hi Michiel,

I'm finally getting around to reviewing this patch. I think the
functionality of respecting these maildir flags for initial import is
going to be really important. So thanks for looking at this!

Some comments on the patches below.

> +enum storage_type
> +notmuch_config_get_storage_type (notmuch_config_t *config);
> +
>  notmuch_bool_t
>  debugger_is_active (void);
>  
> +
> +
> +
>  #endif

[nit] Try to look out for introducing excess whitespace like that.

> +    " The other value is 'storage_type', which can currently be set to\n"
> +    " 'maildir' or 'none'.\n";

This part of the patch I don't like. I've got a mail collection spanning
over a decade, and it's seen a lot of strange things. Most of my mail is
in maildir format, but not quite all of it. And I actually like the
ability to just shove random new messages into the mail store manually
without having to create a maildir name for it.

So I don't think a global configuration makes sense here. Meanwhile,
it's really easy to detect the presence of a maildir. Whenever we see
child directories of "cur", "new", and "tmp" then we should turn on the
processing of maildir flags for when processing mail in "cur" and "new".

> @@ -257,7 +262,7 @@ notmuch_config_open (void *ctx,
>  	    talloc_free (email);
>  	}
>      }
> -
> +    
>      /* When we create a new configuration file here, we  add some
>       * comments to help the user understand what can be done. */
>      if (is_new) {

[nit] Trailing whitespace inserted there as well.

Hmm... I was going to say that git ships with a pre-commit hook you can
turn on that checks for trailing whitespace and aborts the commit if
it's present. But it looks like the currently shipping pre-commit.sample
hook doesn't do this anymore. So I'll have to find out what the current
recommended way is to get this behavior. Does anybody here know?

> +	i += 3;
> +	for (; i < (path + l) && !end_of_flags; i++) {
> +	    switch (*i) {
> +	    case 'F': /* flagged */
> +		notmuch_message_add_tag (message, "flagged");
> +		break;
> +	    case 'R': /* the message has been replied to */
> +		notmuch_message_add_tag (message, "replied");
> +		break;
> +	    case 'D': 
> +		notmuch_message_add_tag (message, "draft");
> +		break;
> +	    case 'S': /* Indicate a message has been read */
> +		notmuch_message_add_tag (message, "seen");
> +		seen = TRUE;
> +		break;
> +	    case 'T': /* Indicates a message has been marked as trash */
> +		notmuch_message_add_tag (message, "trashed");
> +		break;
> +	    case 'P': /* indicates a message has been forwarded */
> +		notmuch_message_add_tag (message, "passed");
> +		break;
> +	    default:
> +		end_of_flags = TRUE;
> +		break;
> +	    }
> +	}
> +    }
> +    
> +    if (i == NULL || !seen) { 
> +	/* mark messages without any flags, or the seen flag as 'unseen' */
> +	notmuch_message_add_tag (message, "unseen");
> +    }

OK, now we're into the meat of things. Clearly, you're directly
supporting the documented flags of maildir. But we need to do a few
things differently here. Most importantly, notmuch is already using an
"unread" tag, so maildir's S flag should map that *that* rather than
adding new "unseen" and "seen" flags. So messages with the S flag would
not get the "unread" tag and messages without S would get the "unread"
tag.

The "flagged" and "replied" tags seem reasonable enough. But for
"trashed" and "passed" I think I'd rather see the tag names as "deleted"
and "forwarded". (Since I can imagine adding commands to notmuch for
"delete" and "forward" but not for "trash" nor "pass").

Finally, all of the new behavior here needs to be documented in both
notmuch.1 and notmuch.c where the current behavior with respect to
"inbox" and "unread" is already documented.

Oh, and setting the "inbox" tag correctly here based on the maildir tags
is the final and most important thing. It looks like that's missing from
the above. So, a missing "S" flag should map to adding both the "inbox"
and "unread" tags.

> (state->ignore_read_only_directories)) {
>  	state->saw_read_only_directory = TRUE;
> -	goto DONE;
> +	goto DONE;	    
>      }

Trailing whitespace strikes again! :-)


> +	    if (state->storage_type == MAILDIR) {
> +		char * leaf = basename(next);

You could save the basename call by examining the leaf name when it is
available as a standalone string up in the caller.

So this patch is close, but needs a few fixes.

Thanks again,

-Carl