Page MenuHomeFreeBSD

sed "y" command bracket balancing check
ClosedPublic

Authored by yuripv on Jul 13 2020, 3:20 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 16 2024, 10:34 AM
Unknown Object (File)
Jan 17 2024, 2:25 PM
Unknown Object (File)
Dec 21 2023, 1:53 PM
Unknown Object (File)
Dec 20 2023, 3:41 AM
Unknown Object (File)
Nov 12 2023, 12:34 AM
Unknown Object (File)
Nov 10 2023, 9:03 AM
Unknown Object (File)
Nov 5 2023, 9:16 PM
Unknown Object (File)
Oct 10 2023, 11:27 PM
Subscribers

Details

Summary

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=247931

Handle '[' as ordinary character for command 'y', that is, is_tr = 1 passed to compile_delimited(), should not affect any other command (all tests pass).

Test Plan

World/kernel build with updated sed.

See added test case.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

yuripv updated this revision to Diff 74381.

remove debugging cruft

yuripv added a reviewer: pfg.

LGTM however, I would recommend an exp-run. Many times before have sed changes caused trouble in the ports tree to be later reverted.

In D25640#567601, @pfg wrote:

LGTM however, I would recommend an exp-run. Many times before have sed changes caused trouble in the ports tree to be later reverted.

Yep, I was looking at the svn log of usr.bin/sed/ and noticed quite a few backouts. I agree that exp-run should be done, but even if some ports break those would need to be fixed instead of rejecting this change as it's obvious bug (and yes, gsed agrees). Before asking for that, there's another issue where documentation disagrees with implementation -- "Within string1 and string2, a backslash followed by any character other than a newline is that literal character, and a backslash followed by an ``n'' is replaced by a newline character." -- this doesn't work (except for \n, \t, \r, the last two need to be documented here, but separately), and I'm wondering if including the fix in this change is OK (it's one-line, I'll mark the line in question in this review) or it will really need to be separate.

usr.bin/sed/compile.c
476 ↗(On Diff #74381)

This needs to be changed to just } else if (*p == '\\') { so that all other \<char> sequences are translated to <char> in 'y' command mode (gsed and common sense do agree :).

In D25640#567601, @pfg wrote:

LGTM however, I would recommend an exp-run. Many times before have sed changes caused trouble in the ports tree to be later reverted.

Yep, I was looking at the svn log of usr.bin/sed/ and noticed quite a few backouts. I agree that exp-run should be done, but even if some ports break those would need to be fixed instead of rejecting this change as it's obvious bug (and yes, gsed agrees).

If ports require fixing, then they should be fixed becfore committing the cjhange.

Before asking for that, there's another issue where documentation disagrees with implementation -- "Within string1 and string2, a backslash followed by any character other than a newline is that literal character, and a backslash followed by an ``n'' is replaced by a newline character." -- this doesn't work (except for \n, \t, \r, the last two need to be documented here, but separately), and I'm wondering if including the fix in this change is OK (it's one-line, I'll mark the line in question in this review) or it will really need to be separate.

You can include the change in the exp-run, and commit it as a separate change.

Marking it as accepted since it LGTM as well; pending exp-run, of course, though I really, really hope we won't find any ports break from this.

This revision is now accepted and ready to land.Jul 15 2020, 1:12 PM
This revision was automatically updated to reflect the committed changes.