Date: Fri, 24 Feb 2012 18:58:47 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [RFC PATCH v5 00/11] Add NOTMUCH_MESSAGE_FLAG_EXCLUDED flag
Message-ID: <20120224235847.GJ30513@mit.edu>
References: <1329296619-7463-1-git-send-email-markwalters1009@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1329296619-7463-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Mark Walters on Feb 15 at  9:03 am:
> This is v5 of the exclude flag series. (v4 was at 
> id:"874nv9rv79.fsf@qmul.ac.uk")
> 
> This email has 4 sections, an overview of what the patch set is 
> trying to achieve, a summary of the changes from v4, some comments on 
> the status of the patches in the series and some remaining queries.
> 
> OVERVIEW
> 
> The current implementation of exclude-tags does not use excludes in
> notmuch-show.c (and thus not in notmuch-show.el). Thus when selecting 
> a thread in the search view claiming one matched message you may get 
> several matches in show all but one of which are tagged excluded.
> 
> The trivial change of adding excludes to show does not work as easily 
> as one would like. For example if you try notmuch-show 
> id:<deleted message> you get no results (see 
> id:"871uqvgrnm.fsf@qmul.ac.uk" for more discussion).
> 
> This set moves in a different direction. It returns all the results 
> but marks excluded messages with a new flag 
> (NOTMUCH_MESSAGE_FLAG_EXCLUDED) and lets the consumer decide what to 
> do with them. For example it could start with the message closed in 
> emacs show view, it could colour the headerline differently etc.
> 
> CHANGES:
> This has been rebased on top of Jani's notmuch-show command line 
> parsing patch.
> 
> The function notmuch_thread_get_flag_messages function added in v4
> has been removed. Unfortunately (as Austin pointed out) that patch 
> broke binary compatibility so should be deferred until we wish to 
> bump the library version.
> 
> I also fixed a minor bug and a style comment in the emacs part of 
> the patch.
> 
> STATUS:
> The first 3 patches in the series just add a --no-exclude option
> to notmuch-search.c and notmuch-count.c to "turn off" the excluding.
> (The 3 patches are one for the C code, one for the man pages and 
> one for the tests.) I think this change is desirable independently 
> of the rest of the series (and indeed Jameson had a use for it
> id:"878vk943ci.fsf@servo.finestructure.net").
> 
> QUERIES
> 
> 1) As with v4 the api notmuch_query_set_omit_excluded_messages
> remains: without it I can't see how a user can pass a 
> notmuch_messages_t object around which does not contain the 
> excluded messages. See id:"87fweusabh.fsf@qmul.ac.uk". 

This does seem like a useful simplification.  Another possibility
(which may not work in practice) would be to have a utility function
in the CLI that iterated a notmuch_messages_t to the next non-excluded
message.  This would push the knowledge of whether or not a format can
represent excluded messages into that format, but I can't see how to
do things like "count" this way.

> 2)  If we have a query which overrides the excludes such as "blah and
> tag:deleted" should the tag:deleted messages still be marked excluded?
> The current implementation does mark them excluded but my preference 
> would be not to. What do people think? At the moment 2 tests fail but 
> the correct output depends on the above so I will leave them until we
> have a decision on this.

I could go either way on this, but I think it should still be marked
excluded since it is, after all, excluded.

> There are some other queries mentioned in v4 but the two above are
> the significant ones.
> 
> Best wishes
> 
> Mark
> 
> 
> 
> Mark Walters (11):
>   cli: add --no-exclude option to count and search.
>   cli: Add --no-exclude to the man pages for search and count
>   test: add tests for new cli --no-exclude option
>   lib: Rearrange the exclude code in query.cc
>   lib: Make notmuch_query_search_messages set the exclude flag
>   lib: Add the exclude flag to notmuch_query_search_threads
>   cli: Make notmuch-show respect excludes.
>   man: update manpage for notmuch-show --no-exclude option
>   test: update tests to reflect the exclude flag
>   cli: omit excluded messages in results where appropriate.
>   emacs: show: recognize the exclude flag.
> 
>  emacs/notmuch-show.el     |   19 +++++++++++-
>  lib/notmuch-private.h     |    8 ++++-
>  lib/notmuch.h             |   16 ++++++++--
>  lib/query.cc              |   74 ++++++++++++++++++++++++++++++++++++++------
>  lib/thread.cc             |   18 +++++++++--
>  man/man1/notmuch-count.1  |    7 ++++
>  man/man1/notmuch-search.1 |    7 ++++
>  man/man1/notmuch-show.1   |    7 ++++
>  notmuch-count.c           |   19 ++++++++----
>  notmuch-search.c          |   26 ++++++++++++----
>  notmuch-show.c            |   31 ++++++++++++++++--
>  test/count                |   21 +++++++++++++
>  test/crypto               |    9 +++++-
>  test/encoding             |    2 +-
>  test/json                 |    6 ++--
>  test/maildir-sync         |    1 +
>  test/multipart            |    4 +-
>  test/search               |    5 +++
>  test/thread-naming        |   16 +++++-----
>  19 files changed, 246 insertions(+), 50 deletions(-)