Page MenuHomeFreeBSD

ls(1): Add -g, -n, and -o options for POSIX standard
AcceptedPublic

Authored by fel1x.mintchoco.development_gmail.com on Apr 2 2022, 11:25 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jan 8, 7:39 PM
Unknown Object (File)
Thu, Jan 5, 11:33 AM
Unknown Object (File)
Sat, Dec 31, 6:07 PM
Unknown Object (File)
Thu, Dec 29, 6:22 PM
Unknown Object (File)
Dec 24 2022, 4:36 PM
Unknown Object (File)
Dec 22 2022, 10:16 AM
Unknown Object (File)
Dec 19 2022, 8:22 AM
Unknown Object (File)
Dec 14 2022, 7:00 PM

Details

Reviewers
pauamma
Group Reviewers
Contributor Reviews (base)
manpages
Summary

Bugzilla bug report in 2004

According to SUSv4 and POSIX, -g, -n, and -o options are required for ls.
This has not been implemented for the compatibility for 4.3 BSD, but I think there is no need to insist it anymore.

f_flags is deleted from case 'o' because file flags are not displayed in the -o option of POSIX.

Test Plan

cd /usr/src/bin/ls
make && make install

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

The manual page should get an update as well along with this change.

fel1x.mintchoco.development_gmail.com edited the test plan for this revision. (Show Details)
fel1x.mintchoco.development_gmail.com marked an inline comment as done and an inline comment as not done.
bin/ls/ls.c
426

I added explanation in the summary.

Compatibility question: how, with that change, can I get output equivalent to current (or 12.x if it changed in 13.x) ls -ol?

This may be related to f_flags being declared and implicitly initialized to false but never set to a true value.

bin/ls/ls.h
57

For consistency with other line comments

bin/ls/print.c
243

I may be missing something (my C is very rusty), but I don't see where in the changed code f_flags is set to a true value.

In D34747#789446, @pauamma_gundo.com wrote:

Compatibility question: how, with that change, can I get output equivalent to current (or 12.x if it changed in 13.x) ls -ol?

This may be related to f_flags being declared and implicitly initialized to false but never set to a true value.

I will add -O option to replace original -o, so the new command would be ls -Ol.

bin/ls/print.c
243

Solved by adding -O option

I found out that -O does not work even though I compiled and installed it, but I cannot find the cause.

bin/ls/ls.c
379–381

ls(1) says it cannot find an option for this although I compiled and installed it. Any ideas?

markj added inline comments.
bin/ls/ls.c
379–381

You need to update the string passed to getopt_long() above. See the getopt_long.3 man page for more details.

As disgorged by grep -rH -E 'ls +-[a-zA-A0-9]*o' /usr/src, the following files in src also need changes:
/usr/src/share/skel/dot.shrc:alias ll='ls -laFo'
/usr/src/sys/contrib/zstd/tests/gzip/init.sh: perms=\ls -dgo "$d" 2>/dev/null\ &&
/usr/src/bin/ls/tests/ls_tests.sh: atf_set "descr" "Verify that the output from ls -o prints out the chflag values or '-' if none are set"
/usr/src/bin/ls/tests/ls_tests.sh: -s exit:0 ls -lo a.file
/usr/src/bin/ls/tests/ls_tests.sh: -s exit:0 ls -lo b.file
/usr/src/bin/chflags/chflags.1:You can use "ls -lo" to see the flags of existing files.

Nothing obvious in the ports tree, but that doesn't include actual source code, so I think an exp-run is in order. There's one ls -lo in the handbook (I haven't checked the rest of the doc tree). And of course, that doesn't and can't account for all the unported apps and homegrown ops or devops shell programs out there. With that in mind, I suggest a staged change instead, with introducing -O and adding a deprecation message to -o as the first stage.

bin/ls/ls.1
876
bin/ls/ls.c
379–381

Missing in the allowed options you're passing getopt_long.

bin/ls/ls.c
379–381

I fixed this by editing usage

379–381

I modified getopt_long on the 280th line.

In D34747#790015, @pauamma_gundo.com wrote:

As disgorged by grep -rH -E 'ls +-[a-zA-A0-9]*o' /usr/src, the following files in src also need changes:
/usr/src/share/skel/dot.shrc:alias ll='ls -laFo'
/usr/src/sys/contrib/zstd/tests/gzip/init.sh: perms=\ls -dgo "$d" 2>/dev/null\ &&
/usr/src/bin/ls/tests/ls_tests.sh: atf_set "descr" "Verify that the output from ls -o prints out the chflag values or '-' if none are set"
/usr/src/bin/ls/tests/ls_tests.sh: -s exit:0 ls -lo a.file
/usr/src/bin/ls/tests/ls_tests.sh: -s exit:0 ls -lo b.file
/usr/src/bin/chflags/chflags.1:You can use "ls -lo" to see the flags of existing files.

Nothing obvious in the ports tree, but that doesn't include actual source code, so I think an exp-run is in order. There's one ls -lo in the handbook (I haven't checked the rest of the doc tree). And of course, that doesn't and can't account for all the unported apps and homegrown ops or devops shell programs out there. With that in mind, I suggest a staged change instead, with introducing -O and adding a deprecation message to -o as the first stage.

Thank you for your help. I edited most of the files, but I cannot find "/usr/src/sys/contrib/zstd/tests/gzip/init.sh".

Thank you for your help. I edited most of the files, but I cannot find "/usr/src/sys/contrib/zstd/tests/gzip/init.sh".

Once I checked 14-current source as of 12ish hours ago instead of 12.2 source (I thought i was, but I must have gotten confused), I noticed that file is gone indeed, but I found 2 more:

sys/contrib/openzfs/tests/zfs-tests/tests/functional/acl/off/dosmode.ksh; sys/contrib/openzfs/tests/zfs-tests/tests/functional/acl/off/dosmode.ksh: ls -lo $path | awk '{ gsub(",", "\n", $5); print $5 }' | \
tools/test/stress2/misc/fts2.sh; sprintf(help, "ls -lo %s", p->fts_path);

LGTM as far as the manual page and the use of -o/-O in src. I'll leave deciding whether an exp-run is needed to others.

This revision is now accepted and ready to land.Apr 13 2022, 12:37 AM

I'm concerned about changing the interface in a backwards-incompatible manner for something as core as ls but will leave that to others

bin/ls/ls.c
442

Well it doesn't behave the same as -O, so suggesting that seems wrong. Printing warnings that can't be disabled is also often not well received, and it should definitely not go to stdout.

This line is also way too long for the column limit.

bin/ls/ls.h
57

f_sgroup does not show the name of the group, it _disables_ showing the name of the group, which is the opposite of the other variables here, and the opposite of what your comment says

bin/ls/print.c
238

You're missing a space after the comma

This revision now requires review to proceed.Apr 15 2022, 12:16 AM
bin/ls/ls.c
442

Then how about adding a warning message in man page, not through stdout?

bin/ls/ls.c
442

Is the manual page visible enough? (But if the deprecation message stays in source,, yes please to stderr.)

Manual page and UX both LGTM. Still needs src review.

This revision is now accepted and ready to land.Apr 18 2022, 10:50 PM
pstef added inline comments.
bin/ls/ls.c
442

Recently some systems started warning about fgrep being deprecated (this is GNU's doing) and to me that's infuriating. So I vote for moving this information to the manual page only.

bin/ls/ls.c
442

Thank you for your suggestion. I like this idea. I removed warning message in stderr and moved it to the man page.

This revision now requires review to proceed.Nov 1 2022, 11:10 PM
This revision is now accepted and ready to land.Nov 2 2022, 2:47 AM