Re: [PATCH] git-send-email: fix get_maintainer.pl regression

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 20, 2017, 22:34 UTC
Message-ID: <CAPig+cQQHMt9757aoBAQfyMtnxmDmrVJ_g8AmzvmmqTMAYaUXw@mail.gmail.com>
In-Reply-To: <87a7zhxm9s.fsf@linaro.org>

On Mon, Nov 20, 2017 at 5:44 AM, Alex Bennée <alex.bennee@linaro.org> wrote:

Show 13 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>> It is not at all clear, based upon this text, what this is fixing.
>> When you re-roll, please provide a description of the regression in
>> sufficient detail for readers to easily understand the problem and the
>> solution.
>
> How about:
>
> Since the removal of Mail::Address from git-send-email certain addresses
> common in MAINTAINERS now fail to get correctly parsed by
> Git::parse_mailboxes. Specifically the patterns with embedded
> parenthesis fail, for example for MAINTAINERS:
> [...snip...]

Thanks, that explanation makes the problem quite clear. It also allowed me to examine the fix with a more critical eye, which led to several additional comments and observations (sent in my previous email).

Show 15 quoted lines

>> Use write_script() to create the script:
>
> Thanks for the pointers, I'll fix it up.
>
> I missed the existence of write_script.sh while I scanned through
> t/README, perhaps a stanza in in "Test harness library":
>
>  - write_script <name> <<-\EOF && <rest of test>
>    echo '...'
>    ...
>    EOF
>
>    The write_script helper takes care of ensuring the created helper
>    script has the right shebang and is executable.
> ?

Mentioning write_script() in the "Test harness library" section might indeed be a good idea.

Show 7 quoted lines

> Ahh that makes sense. Again perhaps in the t/README "Keep in mind:"
>
>  - All the tests in a given test file run sequentially and share
>    repository state. This means you should take care not to break
>    assumptions of later tests as to which commits exist in the test
>    repository.
> ?

Maybe, maybe not. Ideally, tests should be as self-contained as possible. In practice, of course, this isn't always practical or even possible (and there is a lot of old test code which is heavily dependent upon state left over from earlier tests). Perfectly self-contained tests (or self-contained _sets_ of tests) could theoretically be run in parallel, so, in general, we'd like to encourage people to tend toward self-contained, thus the above snippet may be going in the wrong direction.