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

Add passwd completions #4209

Closed
wants to merge 5 commits into from
Closed

Conversation

PenegalECI
Copy link
Contributor

Description

This PR adds completions for the passwd command. It works for Linux, as I don't know BSD enough to reliably add completions.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

# Currently works only for Linux, written under Debian Stretch, shadow-utils 4.4
# TODO: add *BSD support

if test (uname) = "Linux"
Copy link
Member

Choose a reason for hiding this comment

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

We don't use uname tests unless it absolutely cannot be helped (e.g. for killall, since on Solaris we cannot invoke that ever because it kills all processes). This is mainly because you can install different versions of a tool on different OSs - @krader1961 installs gnu tools on macOS.

Usually, we try if $command --help >/dev/null ^&1 to detect gnu-ish versions of a command, which should also work here.

@faho
Copy link
Member

faho commented Jul 11, 2017

It works for Linux, as I don't know BSD enough to reliably add completions.

Me neither, but what I've found is that the man pages are usually accurate enough to write completions by, and that the tools are so simple that there isn't much to do.

See http://www.unix.com/man-page/freebsd/1/passwd/, https://ss64.com/osx/passwd.html and http://www.unix.com/man-page/sunos/1/passwd/.

Of note here is that, while they are quite different, I'd imagine the options they offer to be uncommonly used, and that all of them accept a username without option. So, if you want, you can add that part unconditionally and do the options if the "--help" check succeeds. The rest is nice to have but not necessary.

@PenegalECI
Copy link
Contributor Author

PenegalECI commented Jul 11, 2017

In fact, I preferred not adding completions without being able to test them, but if it's OK to do so, then I will, with the help of your command; in that case, I noticed that different BSD flavors have different options for passwd. I can't add all of them, so which ones should I add – FreeBSD and macOS, I assume – and which test to use to distinguish them?

I might add that the uname test was inspired by the code of __fish_print_packages.fish, which uses it.

@faho
Copy link
Member

faho commented Jul 11, 2017

In fact, I preferred not adding completions without being able to test them, but if it's OK to do so, then I will, with the help of your command.

Yes, for completions it's okay to do so. The worst that can happen is that executing that completion prints an error message, and that's unlikely in this case as you wouldn't be doing anything complicated.

I might add that the uname test was inspired by the code of __fish_print_packages.fish, which uses it.

It only uses that for pkg, and I'm not sure why. It's possible there's a different utility of the same name that can't be checked. Or the person doing that just didn't know.

Anyway, that function is in need of some love - I'd actually argue that it should be split up (or at least take an argument for which package it should use), since it's only ever called by completions for package management tools. If you ever install multiple of them (e.g. to create a container for a different distribution), it might print packages for the wrong package manager. It also has a comment about the emerge path being broken.

@PenegalECI
Copy link
Contributor Author

Yes, for completions it's okay to do so.

Let's go, then. However, it looks like you missed my edit: I noticed that different BSD flavors have different options for passwd. I can't add all of them, so which ones should I add – FreeBSD and macOS, I assume – and which test to use to distinguish them?

@faho
Copy link
Member

faho commented Jul 11, 2017

Humm... that's actually a good question...

This might actually really need a uname check. We could source the generated completion, but on my system that's bogus because it uses the openssl passwd man page instead (because that's stored in the "1ssl" section, because both man and openssl are annoying).

So, how about this:

  • Always offer the user completion. Any passwd will take that.

  • Try passwd --help. If it returns 0, it's the gnu-ish shadow version.

  • If it doesn't succeed, call uname once, and store the result. Check this against know OSs and offer the completions for them.

?

@PenegalECI
Copy link
Contributor Author

So be it.

Please note, though, that I'm developping that at work, as my boss allows me to improve open source software and publish that when the improvement is useful to my work. As we don't use BSD, I'm a bit outside scope, but I don't wan't to turn off your advices on improving my PR, so I'll split the difference and stick with what is AFAIK the top 3: FreeBSD, OpenBSD and macOS. Their man are easy to find and adding them won't be a big deal, while covering something like 90% of BSD users.

@faho
Copy link
Member

faho commented Jul 11, 2017

Please note, though, that I'm developping that at work, as my boss allows me to improve open source software and publish that when the improvement is useful to my work.

Neat. You're probably the second person paid to work on fish (the creator of vi-mode got a bounty for it).

@PenegalECI
Copy link
Contributor Author

Here comes the completions with the modifications you requested.

@faho
Copy link
Member

faho commented Jul 11, 2017

Also merged, thanks!

@faho faho closed this Jul 11, 2017
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants