Page MenuHomeFreeBSD

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

Authored by pstef on Jul 17 2022, 8:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 11:16 PM
Unknown Object (File)
Mon, Nov 18, 10:56 PM
Unknown Object (File)
Mon, Nov 4, 6:04 PM
Unknown Object (File)
Oct 28 2024, 7:12 AM
Unknown Object (File)
Sep 25 2024, 3:39 AM
Unknown Object (File)
Sep 24 2024, 4:28 AM
Unknown Object (File)
Sep 23 2024, 5:50 PM
Unknown Object (File)
Sep 23 2024, 4:12 PM
Subscribers

Details

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

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

@jilles what do you think about this one?

This should have testcases.

bin/sh/histedit.c
279

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.

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.

bin/sh/histedit.c
281

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

bin/sh/tests/builtins/fc3.0
4

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

10–11

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.

@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 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
This revision was automatically updated to reflect the committed changes.