From: Mark Walters <markwalters1009@gmail.com>
To: Michal Sojka <sojkam1@fel.cvut.cz>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 00/10] "notmuch address" command
In-Reply-To: <1415058622-21162-1-git-send-email-sojkam1@fel.cvut.cz>
References: <1415058622-21162-1-git-send-email-sojkam1@fel.cvut.cz>
User-Agent: Notmuch/0.18.1+86~gef5e66a (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Tue, 04 Nov 2014 09:23:42 +0000
Message-ID: <87oasn2uap.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


On Mon, 03 Nov 2014, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> Hi all,
>
> this is v2 of "notmuch address" patchset. It obsoletes [1].
>
> Don't be scared by the number of patches. Most of them are trivial
> refactoring. Patches 1-4 refactor the code so that "notmuch search"
> command is easier to split. Patch 5 is Jani's hierarchical command
> line parsing patch. Patch 6 splits search functionality to new address
> command. Patch 7 is minor refactoring. Patches 8-10 correspond to
> patches 5-7 in the original "notmuch search
> --output=sender/recipients" patch series [2].
>
> Changes from v1:
>
> - Rebased to current master (conflicted with Jani's "notmuch search
>   --duplicate=N with --output=messages" patch)
> - Fixed printing of false "Unrecognized option" error message in
>   hierarchical command line parser.

Overall this series looks good. My inclination is to leave patch 10
until after the freeze but I am happy either way. Tests all work and my
testing is fine. 

A couple of other comments:

you should add something to devel/schemata detailing the fields in the
structured address output.

Please check the new manpage: I think some of it is slightly outdated. I
mentioned in my comments the exclude=false, but missed exclude=all. Also
the manpage could say what actually gets printed in the default text
case (in particular to emphasise that it includes the real name)

Best wishes

Mark


>
> Regards,
> -Michal
>
> [1] id:1414889400-30977-1-git-send-email-sojkam1@fel.cvut.cz
> [2] id:1414792441-29555-1-git-send-email-sojkam1@fel.cvut.cz
>
>
> Jani Nikula (1):
>   cli: add support for hierarchical command line option arrays
>
> Michal Sojka (9):
>   cli: search: Rename options to context
>   cli: search: Move more variables into search_context_t
>   cli: search: Convert ctx. to ctx->
>   cli: search: Split notmuch_search_command to smaller functions
>   cli: Introduce "notmuch address" command
>   cli: search: Convert --output to keyword argument
>   cli: address: Do not output duplicate addresses
>   cli: address: Add --output=count
>   cli: address: Add --filter-by option to configure address filtering
>
>  command-line-arguments.c           |  16 +-
>  command-line-arguments.h           |   1 +
>  completion/notmuch-completion.bash |  48 +++-
>  completion/notmuch-completion.zsh  |  11 +-
>  doc/man1/notmuch-address.rst       | 140 ++++++++++++
>  doc/man1/notmuch-search.rst        |  21 +-
>  doc/man1/notmuch.rst               |   7 +-
>  notmuch-client.h                   |   3 +
>  notmuch-search.c                   | 454 +++++++++++++++++++++++++------------
>  notmuch.c                          |   2 +
>  test/T095-address.sh               | 148 ++++++++++++
>  test/T097-address-filter-by.sh     |  73 ++++++
>  12 files changed, 751 insertions(+), 173 deletions(-)
>  create mode 100644 doc/man1/notmuch-address.rst
>  create mode 100755 test/T095-address.sh
>  create mode 100755 test/T097-address-filter-by.sh
>
> -- 
> 2.1.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch