Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use __fish_complete_groups to complete group names for chown #3380

Merged
merged 2 commits into from
Sep 16, 2016

Conversation

andrew-schulman
Copy link
Contributor

Description

chown completion chown currently uses cat /etc/group to fetch the list of group names. In Cygwin there's no /etc/group file any more (user and group names are fetched directly from the OS), so when a user tries to tab-complete the group name they get an error message:

ASchulma@LZ77E1AASCHULMA ~/d/fish> chown ASchulma:cat: /etc/group: No such file or directory

This change fixes that by using getent group by preference to get the group names, and falling back to /etc/group. This is more portable, and it also agrees with the logic used for chgrp completion in __fish_complete_groups.

TODOs:

  • Changes to fish usage are reflected in user documenation/manpages.
  • Tests have been added for regressions fixed

@@ -9,4 +9,4 @@ complete -c chown -s v -l verbose --description "Output diagnostic for every fil
complete -c chown -s h -l help --description "Display help and exit"
complete -c chown -l version --description "Display version and exit"
complete -c chown --description "Username" -a "(__fish_print_users):"
complete -c chown --description "Username" -a "(echo (commandline -ct)| __fish_sgrep -o '.*:')(cat /etc/group |cut -d : -f 1)"
complete -c chown --description "Username" -a "(echo (commandline -ct)| __fish_sgrep -o '.*:')(if test -x /usr/bin/getent; getent group; else; cat /etc/group; end |cut -d : -f 1)"
Copy link
Member

@faho faho Sep 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might also be in /bin or /sbin or /usr/local/bin or /I'm/weird/so/this/is/my/bin - use command -s getent >/dev/null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait.... why doesn't this just use __fish_complete_groups?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__fish_complete_groups uses slightly different logic to pull two fields from /etc/group (cut -d : -f 1,4), and adds a tab at the end. Different enough to not be useful for chown completion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part after the tab is the description. You could just remove it again with string replace -r '\t.*$' ''.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though that description might be helpful here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. First time I've dug into completions. Yes, __fish_complete_groups works fine here, I'll revise.

@faho faho added bug Something that's not working as intended cygwin labels Sep 15, 2016
@faho faho added this to the fish 2.4.0 milestone Sep 15, 2016
@andrew-schulman andrew-schulman changed the title for group names in chown completion, use getent if available use __fish_complete_groups to complete group names for chown Sep 16, 2016
@faho faho merged commit 05b52ea into fish-shell:master Sep 16, 2016
@faho
Copy link
Member

faho commented Sep 16, 2016

Merged, thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended cygwin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants