GNU bug report logs - #24535
[PATCH] sed: fix incorrect match for title case

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 24535 in the body.
You can then email your comments to 24535 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: <bug-sed <at> gnu.org>
Subject: [PATCH] sed: fix incorrect match for title case
Date: Sun, 25 Sep 2016 11:23:50 +0900

[Message part 1 (text/plain, inline)]

sed uses fastmap feature in matching with regex.  If it is used for case
insensitive matching, affected by following bug in regex.

  https://sourceware.org/bugzilla/show_bug.cgi?id=20381

BTW, I do not know whether sed is affected by the bug or not in locales
except el_GR.iso88597.

[0001-sed-fix-incorrect-match-for-title-case.patch (text/plain, attachment)]

Message #8 received at 24535 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 24535 <at> debbugs.gnu.org
Subject: Re: bug#24535: [PATCH] sed: fix incorrect match for title case
Date: Sat, 24 Sep 2016 20:43:01 -0700

On Sat, Sep 24, 2016 at 7:23 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
> sed uses fastmap feature in matching with regex.  If it is used for case
> insensitive matching, affected by following bug in regex.
>
>   https://sourceware.org/bugzilla/show_bug.cgi?id=20381
>
> BTW, I do not know whether sed is affected by the bug or not in locales
> except el_GR.iso88597.

Thank you for that patch.
Quick feedback:

Did you intend to use the new function, require_el_iso88597_locale_?

Please replace this line from your new test (I prefer to avoid double
quoting strings intended to contain backslashes, because each must be
doubled, and that impairs readability/maintainability):

  printf "/\\(\\)\\\\1$chr/Ip" >prog || fail=1

with this:

  printf '/\(\)\\1'"$chr"/Ip >prog || fail=1

Is that a stray "-f -n" in the test script? This cannot work as intended:

  $ sed -f -n -f prog in >out
  sed: couldn't open file -n: No such file or directory

Message #11 received at 24535 <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: Jim Meyering <jim <at> meyering.net>
Cc: 24535 <at> debbugs.gnu.org
Subject: Re: bug#24535: [PATCH] sed: fix incorrect match for title case
Date: Sun, 25 Sep 2016 14:49:11 +0900

[Message part 1 (text/plain, inline)]

On Sat, 24 Sep 2016 20:43:01 -0700
Jim Meyering <jim <at> meyering.net> wrote:

> Thank you for that patch.
> Quick feedback:
> 
> Did you intend to use the new function, require_el_iso88597_locale_?
> 
> Please replace this line from your new test (I prefer to avoid double
> quoting strings intended to contain backslashes, because each must be
> doubled, and that impairs readability/maintainability):
> 
>   printf "/\\(\\)\\\\1$chr/Ip" >prog || fail=1
> 
> with this:
> 
>   printf '/\(\)\\1'"$chr"/Ip >prog || fail=1
> 
> Is that a stray "-f -n" in the test script? This cannot work as intended:
> 
>   $ sed -f -n -f prog in >out
>   sed: couldn't open file -n: No such file or directory

Thanks for quick feedback.  Sorry, I seem that I extracted the patch
from old repository.  I fixed it, and ru-run all tests and they are
passed.

[0001-sed-fix-incorrect-match-for-title-case.patch (text/plain, attachment)]

Message #14 received at 24535 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 24535 <at> debbugs.gnu.org
Subject: Re: bug#24535: [PATCH] sed: fix incorrect match for title case
Date: Sun, 25 Sep 2016 17:38:44 -0700

[Message part 1 (text/plain, inline)]

On Sat, Sep 24, 2016 at 10:49 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
>
> On Sat, 24 Sep 2016 20:43:01 -0700
> Jim Meyering <jim <at> meyering.net> wrote:
>
>> Thank you for that patch.
>> Quick feedback:
>>
>> Did you intend to use the new function, require_el_iso88597_locale_?
>>
>> Please replace this line from your new test (I prefer to avoid double
>> quoting strings intended to contain backslashes, because each must be
>> doubled, and that impairs readability/maintainability):
>>
>>   printf "/\\(\\)\\\\1$chr/Ip" >prog || fail=1
>>
>> with this:
>>
>>   printf '/\(\)\\1'"$chr"/Ip >prog || fail=1
>>
>> Is that a stray "-f -n" in the test script? This cannot work as intended:
>>
>>   $ sed -f -n -f prog in >out
>>   sed: couldn't open file -n: No such file or directory
>
> Thanks for quick feedback.  Sorry, I seem that I extracted the patch
> from old repository.  I fixed it, and ru-run all tests and they are
> passed.

Thank you. That looks great.
I have renamed the new test file to remove the "mb-" prefix, since
it's not multibyte-related, and adjusted the commit log:

[sed-title-case-fix.diff (text/plain, attachment)]

Message #17 received at 24535 <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: Jim Meyering <jim <at> meyering.net>
Cc: 24535 <at> debbugs.gnu.org
Subject: Re: bug#24535: [PATCH] sed: fix incorrect match for title case
Date: Tue, 27 Sep 2016 08:36:37 +0900

On Sun, 25 Sep 2016 17:38:44 -0700
Jim Meyering <jim <at> meyering.net> wrote:

> Thank you. That looks great.
> I have renamed the new test file to remove the "mb-" prefix, since
> it's not multibyte-related, and adjusted the commit log:

Thanks for reviwing and adjusting for the patch.  I agree to all of them.

Message #22 received at 24535-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 24535-done <at> debbugs.gnu.org
Subject: Re: bug#24535: [PATCH] sed: fix incorrect match for title case
Date: Mon, 26 Sep 2016 20:29:02 -0700

On Mon, Sep 26, 2016 at 4:36 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
> On Sun, 25 Sep 2016 17:38:44 -0700
> Jim Meyering <jim <at> meyering.net> wrote:
>
>> Thank you. That looks great.
>> I have renamed the new test file to remove the "mb-" prefix, since
>> it's not multibyte-related, and adjusted the commit log:
>
> Thanks for reviwing and adjusting for the patch.  I agree to all of them.

Thanks again. Pushed:
http://git.sv.gnu.org/cgit/sed.git/commit/?id=188207529cec0

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.