On 2020-03-19 15:02:37-0700, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > > > On Thu, Mar 19, 2020 at 11:53 AM Jeff King <peff@xxxxxxxx> 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
