Re: [PATCH 1/6] t4061: use POSIX compliance regex(7)

From: Danh Doan <congdanhqx@gmail.com>
Date: Mar 20, 2020, 01:35 UTC
Message-ID: <20200320013526.GE1858@danh.dev>
In-Reply-To: <xmqqwo7gc8aq.fsf@gitster.c.googlers.com>

On 2020-03-19 15:02:37-0700, Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Thu, Mar 19, 2020 at 11:53 AM Jeff King <peff@peff.net> wrote:
> >> On Thu, Mar 19, 2020 at 09:00:02PM +0700, Đoàn Trần Công Danh wrote:
> >> > Fix it by using literal `+` instead.
> >>
> >> This makes sense, I think. It could hurt a sed which is expected ERE and
> >> needs the "+" escaped, but I think such a sed would be wrong (and I
> >> imagine would break things elsewhere).
> >
> > I had the same thought and considered suggesting a character class:
> >
> >     sed -n -e "1,4d" -e "s/^[+]//p" <"$1" >.tmp-1
> >
> > to make it painfully obvious that "+" is not special in the
> > expression. But then I thought better of it -- for the same reason as
> > you (to wit: such a 'sed' would be wrong) -- and decided against
> > saying anything.
> 
> I have only one thing that needs fixing, which is s/compliance/compliant/;
> on the title.  Other than that, it looks good.
> 
> Having said that, I would have done the [+] thing if I were doing
> this patch myself.  As long as we see no "wrong" sed that is broken
> by this change, I am OK with it, though.

Well, `[+]` thing was my first version for this change, but I change in to this version afterward.

However, your comment in a later patch:

> IOW, I do not have trouble changing the test so that it works with
> noncompliant "diff".  But then in the same series, I would prefer to
> see the existing test keeps working with a possibly noncompliant
> "sed" implementation that has been working well with the tests.

changed my mind.

I will use `[+]` here in the reroll.

-- 
Danh