sed "y" command bracket balancing check

[yuripv]

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.

[yuripv]

remove debugging cruft

[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.

[yuripv]

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 :).

[pfg]

In D25640#567710, @yuripv wrote:

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.

[kevans]

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.