Page MenuHomeFreeBSD

sed "y" command bracket balancing check
ClosedPublic

Authored by yuripv on Jul 13 2020, 3:20 AM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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.