Re: [PATCH v2 3/3] mailinfo: disallow NUL character in mail's header

From: Danh Doan <congdanhqx@gmail.com>
Date: Apr 19, 2020, 14:24 UTC
Message-ID: <20200419142423.GC28207@danh.dev>
In-Reply-To: <CAN0heSpZR6HtntK80DhQxtRQZo21GBd3uAKrsJxz_opyun8n4g@mail.gmail.com>

On 2020-04-19 14:30:19+0200, Martin Ågren <martin.agren@gmail.com> wrote:

Show 23 quoted lines

> On Sun, 19 Apr 2020 at 13:05, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> > --- a/t/t4254-am-corrupt.sh
> > +++ b/t/t4254-am-corrupt.sh
> > @@ -69,9 +69,11 @@ test_expect_success "NUL in commit message's body" '
> >         grep "a NUL byte in commit log message not allowed" err
> >  '
> >
> > -test_expect_failure "NUL in commit message's header" '
> > +test_expect_success "NUL in commit message's header" '
> >         test_when_finished "git am --abort" &&
> >         write_nul_patch subject >subject.patch &&
> > +       test_must_fail git mailinfo msg patch <subject.patch 2>err &&
> > +       grep "a NUL byte in Subject is not allowed" err &&
> >         test_must_fail git am subject.patch 2>err &&
> >         grep "a NUL byte in Subject is not allowed" err
> >  '
> 
> We used to fail for some reason and it's sort of clear from the size
> of the test what that reason is. Now that we flip "failure" to "success"
> we can add some more stuff here that works. Makes sense. Of course,
> somewhere is a line for stuffing too much into the test, i.e., you'll
> reach a point where you should have separate tests, but I think this is
> ok.

I'm not really keen to check the "git-mailinfo" failure here. Since, another developer may come and decide that we should keep the mailinfo as is, i.e. keep NUL character in Subject, Author, Email, etc... And let's git-commit complains instead.

Adding a check for git-mailinfo here may limit future development. Other's feedback is welcomed.

-- 
Danh