Hi!

On Mon, 10 Oct 2011 10:49:15 -0300, david@tethera.net wrote:
> OK, here is my proposal to add search terms to notmuch dump.

Having worked in the same area ;-), I felt competent to review this.  And
I definitely do like David's approach.  The patches look good, with the
following comments:

What's missing is adding (roughly) the same text to the notmuch manpage,
``notmuch help dump'', NEWS file.  These should be added to the
respective patches, for enhance functionality and deprecation of output
filename.

> 2b7781d test: all dump-restore tests should be working now
> 7a203d6 notmuch-dump: treat any remaining arguments after the filename as search t

I would suggest to combine these two into one patch: enhance
implementation (7a203d6) and update the tests (2b7781d) is one unit.

> d6715d7 test: add tests for command line arguments to notmuch-dump

Specifically:

On Mon, 10 Oct 2011 10:49:17 -0300, david@tethera.net wrote:
> The plan is to add the possibility of search terms after the file name,
> and the use of -- to stop looking for an output file name.
> ---
>  test/dump-restore |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/test/dump-restore b/test/dump-restore
> index 96c4f19..699337c 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -8,6 +8,34 @@ test_expect_success "Dumping all tags" "generate_message &&
>  notmuch new &&
>  notmuch dump > dump.expected"
>  
> +test_begin_subtest "dump outfile"
> +notmuch dump dump-outfile.actual
> +test_expect_equal_file dump.expected dump-outfile.actual
> +
> +test_begin_subtest "dump outfile --"
> +notmuch dump dump-1-arg-dash.actual
> +test_expect_equal_file dump.expected dump-1-arg-dash.actual
> 
> [...]

I don't understand the purpose of the second test above.  Was this meant
to be ``notmuch dump dump-1-arg-dash.actual --'' (as suggested by the
description), or ``notmuch dump -- > dump-1-arg-dash.actual''?


Grüße,
 Thomas