Re: [WIP-PATCH 0/2] send-email: refactor the email parser loop

From: Eric Wong <e@80x24.org>
Date: May 27, 2016, 20:14 UTC
Message-ID: <20160527201436.GA16547@dcvr.yhbt.net>
In-Reply-To: <20160527140104.11192-1-samuel.groot@grenoble-inp.org>

Samuel GROOT <samuel.groot@grenoble-inp.org> wrote:

> While working on the new option `quote-email`, we needed to parse an
> email file. Since the work is already done, but the parsing and data
> processing are in the same loop, we wanted to refactor the parser, to
> make the code more maintainable.

Thank you for doing this work :)

Show 16 quoted lines

> This is still WIP, and one of the main issue (and we need your
> advice on that), is that around 30 tests will fail, and most of them
> are false-negatives: to pass, they rely on the format of what is
> displayed by `git send-email`, not only data.
> 
> 
> For example, several tests will fail because they do a strict compare
> between `git send-email`'s output and:
> 
>    (mbox) Adding cc: A<author@example.com>  from line 'Cc: A<author@example.com>, One<one@example.com>'
>    (mbox) Adding cc: One<one@example.com>  from line 'Cc: A<author@example.com>, One<one@example.com>'
> 
> Though `git send-email` now outputs something like:
> 
>    (mbox) Adding cc: A<author@example.com>  from line 'Cc: A<author@example.com>'
>    (mbox) Adding cc: One<one@example.com>  from line 'Cc: One<one@example.com>'

I actually like neither, and would prefer something shorter:

    (mbox) Adding cc: A <author@example.com> from Cc: header
    (mbox) Adding cc: One <one@example.com> from Cc: header
    (mbox) Adding cc: SoB <SoB@example.com> from Signed-off-by: trailer

That way, there's no redundant addresses in each line and less likely to wrap.

But I actually never noticed these lines in the past since they scrolled off my terminal :x

Since the headers are already shown after those lines, it's redundant to have the entire line. And we could add trailers after the headers (with a blank line to delimit):

    # existing header output:
    From: F <F@example.com>
    Cc: A <author@example.com>, One <one@example.com>
    Subject: foo

    # new trailer output
    Signed-off-by: SoB <SoB@example.com>
    Acked-by: ack <ack@example.com>

Show 16 quoted lines

> We can think of several solutions:
> 
>    1. Modify the parser, to give the script the knowledge of the exact
>       line the data came from.
> 
>    2. Hack the tests: modify the script using the parser to artificially
>       recreate the supposedly parsed line.
>       (e.g. with a list.join(', ')-like function)
> 
>    3. Modify the tests to replace exact cmp by greps.
> 
> 
> IMHO, we should consider option 3, since the tests should rely on data
> rather than exact outputs. It also makes the tests more maintainable,
> in the sense that they will be more resilient to future output
> modifications.

Agreed on 3.

I am not sure if anybody outside of tests parses the stdout of send-email. It's certainly a porcelain and I don't think stdout needs to be stable, and maybe the output in question should go to stderr since it could be considered debug output.

But I could be wrong...