Page MenuHomeFreeBSD
Differential D35836

sh: accept fc options grouped behind one '-'
ClosedPublic
Actions

Authored by pstef on Jul 17 2022, 8:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 14, 4:51 PM
Unknown Object (File)
Fri, Mar 7, 11:28 AM
Unknown Object (File)
Tue, Mar 4, 11:58 AM
Unknown Object (File)
Thu, Feb 27, 8:36 PM
Unknown Object (File)
Feb 6 2025, 4:17 AM
Unknown Object (File)
Feb 5 2025, 5:05 PM
Unknown Object (File)
Feb 5 2025, 2:35 PM
Unknown Object (File)
Jan 18 2025, 9:30 PM
View All Files
Subscribers
imp

Details

Reviewers
jilles
Commits
rG755a1be6d015: sh: accept fc options grouped behind one '-'
Summary

As per Utility Syntax Guidelines, accept both forms: -l -n and -ln.

To do that, anticipate the source string for the next option that will
be parsed by nextopt(). It's not always *argptr, sometimes it is
nextopt_optptr.

To simplify the check for not_fcnumber, slightly modify nextopt() to
always nullify nextopt_optptr in cases where it would have been set
to point to a NUL character.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

pstef created this revision.Jul 17 2022, 8:23 AM
Herald added a reviewer: jilles. · View Herald TranscriptJul 17 2022, 8:23 AM
pstef requested review of this revision.Jul 17 2022, 8:23 AM
pstef retitled this revision from sh: accept options grouped behind one '-' to sh: accept fc options grouped behind one '-'.Jul 17 2022, 10:20 AM
pstef edited the summary of this revision. (Show Details)
pstef updated this revision to Diff 108249.Jul 17 2022, 11:43 AM
pstef edited the summary of this revision. (Show Details)

I've made the mistake of assuming that the first token has to be an option, but of course fc -1 must work.

Slightly change the strategy to fix the original issue and keep the not_fcnumber() check first as it was originally.

pstef set the repository for this revision to rG FreeBSD src repository.Jul 18 2022, 9:40 AM
Herald added a subscriber: imp. · View Herald TranscriptJul 18 2022, 9:40 AM
pstef added a comment.Jul 22 2022, 7:17 PM

@jilles what do you think about this one?

jilles added a comment.Jul 23 2022, 11:51 AM

This should have testcases.

bin/sh/histedit.c
282

If nextopt_optptr is not equal to *argptr and is an fcnumber, it will still attempt to use *argptr below. This would affect cases like fc -n1.

pstef updated this revision to Diff 108741.Jul 30 2022, 6:57 PM

Slightly change the approach: each not_fcnumber() check, keep consuming options until we need to jump to the next argptr.
Add a couple of test cases.

pstef marked an inline comment as done.Jul 30 2022, 6:57 PM
jilles added inline comments.Aug 4 2022, 12:29 PM
bin/sh/histedit.c
304

Nit (pre-existing issue): this (char) cast is unnecessary, and other nextopt callers don't use it.

bin/sh/tests/builtins/fc3.0
3 ↗(On Diff #108741)

Please avoid the == in [ non-standard feature (but see below).

9–10 ↗(On Diff #108741)

I tend to make test scripts portable (apart from the thing being tested for, if it is not standard), so that they can run with a variety of shells. Also, if the only thing that prevents running the test successfully in shells/heirloom-sh is $(...) command substitution, I tend to replace it with the traditional form.

Here, break and continue outside of a loop is not standard, and in various shells such as bash, mksh and zsh it writes an error message (and might also abort a script; I haven't checked that). Instead, : command1 and : command2 could be used.

bin/sh/tests/functional_test.sh
37–39 ↗(On Diff #108741)

Hmm, fc1.0 and fc2.0 also fail if history support is not compiled in. I would be fine with not having this check.

pstef updated this revision to Diff 108928.Aug 6 2022, 5:43 PM
pstef marked 3 inline comments as done.Aug 16 2022, 5:56 PM
pstef added a comment.Aug 19 2022, 10:49 AM

@jilles is this patch in a good shape now?

Or maybe we should convert sh to use getopt? Custom options parsing keeps surprising people, see PR265399 for a recent example.

jilles accepted this revision.Aug 19 2022, 8:39 PM
In D35836#823357, @pstef wrote:

@jilles is this patch in a good shape now?

Yes.

Or maybe we should convert sh to use getopt? Custom options parsing keeps surprising people, see PR265399 for a recent example.

The echo builtin is defined not to support standard options parsing, and changing that will lead to compatibility issues. In the particular case of echo, I don't think it's worth it since there is no obviously best solution so there will still be many bad surprises after a fix.

As for builtins that support standard options parsing, some of them used to use getopt() from libc, but we standardized on nextopt() to reduce code duplication and code size. For example, the error messages from getopt() cannot be used because they are sent to stderr instead of a potentially redirected out2 from output.c, for example echo ".$(umask -\? 2>&1)." .

This revision is now accepted and ready to land.Aug 19 2022, 8:39 PM
Closed by commit rG755a1be6d015: sh: accept fc options grouped behind one '-' (authored by pstef). · Explain WhyAug 20 2022, 11:31 AM
This revision was automatically updated to reflect the committed changes.
pstef added a commit: rG755a1be6d015: sh: accept fc options grouped behind one '-'.

Revision Contents
Changeset List

PathSize
bin/
sh/
13 lines

Diff 108239

View Options

bin/sh/histedit.c

Show First 20 Lines • Show All 768 Lines • ▼ Show 20 Linesprint_eth_rule(struct pfctl_eth_rule *r, const char *anchor_call,
if (r->quick) if (r->quick)
printf(" quick"); printf(" quick");
if (r->ifname[0]) { if (r->ifname[0]) {
if (r->ifnot) if (r->ifnot)
printf(" on ! %s", r->ifname); printf(" on ! %s", r->ifname);
else else
printf(" on %s", r->ifname); printf(" on %s", r->ifname);
} }
if (r->bridge_to[0])
printf(" bridge-to %s", r->bridge_to);
if (r->proto) if (r->proto)
printf(" proto 0x%04x", r->proto); printf(" proto 0x%04x", r->proto);
if (r->src.isset) { if (r->src.isset) {
printf(" from "); printf(" from ");
print_eth_addr(&r->src); print_eth_addr(&r->src);
} }
if (r->dst.isset) { if (r->dst.isset) {
▲ Show 20 Lines • Show All 1,219 Lines • Show Last 20 Lines