Re: [PATCH v2 1/3] check-mailmap: accept "user@host" contacts

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 21, 2024, 18:27 UTC
Message-ID: <CAPig+cRhEJP6-ZX3X9SYrGM335-XTp8V=D9Yc7xkJQAh-g8xRw@mail.gmail.com>
In-Reply-To: <mjlmmgwczact5ryprmorqztip2ynpcu5gpbulfabnoul2ubnr6@pfaxe7j4xo3h>

On Wed, Aug 21, 2024 at 1:50 PM Josh Steadmon <steadmon@google.com> wrote:

Show 15 quoted lines

> On 2024.08.19 17:07, Jacob Keller wrote:
> > diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> > @@ -73,11 +73,40 @@ test_expect_success 'check-mailmap --stdin arguments: mapping' '
> >  test_expect_success 'check-mailmap bogus contact' '
> > -     test_must_fail git check-mailmap bogus
> > +     cat >expect <<-EOF &&
> > +     <bogus>
> > +     EOF
> > +     git check-mailmap bogus >actual &&
> > +     test_cmp expect actual
> >  '
>
> I think I'd just remove this test case altogether, IIUC it' doesn't
> provide any additional value vs. the "check-mailmap simple address: no
> mapping" test below.

I had the same thought upon reading this.

Show 14 quoted lines

> >  test_expect_success 'check-mailmap bogus contact --stdin' '
> > -     test_must_fail git check-mailmap --stdin bogus </dev/null
> > +     cat >expect <<-EOF &&
> > +     <bogus>
> > +     EOF
> > +     cat >stdin <<-EOF &&
> > +     bogus
> > +     EOF
> > +     git check-mailmap --stdin <stdin >actual &&
> > +     test_cmp expect actual
> > +'
>
> Similarly, I might change this to use a real address instead of "bogus",
> as we're no longer checking for invalid input.

Ditto for this change, but even more so because this is a fairly significant semantic change. In particular, the documented and intended behavior of the command when --stdin is specified is that it will consume email addresses from *both* the command-line and from standard input, and I think the point of the original test was to verify that it still correctly recognized a bogus email address specified as an argument even when --stdin is requested. Given that understanding (assuming it's correct), then the original test was already perhaps somewhat iffy anyhow, but after this change, it is even less meaningful.