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

Added completions/translations for zfs and zpool #4608

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@PenegalECI
Copy link
Contributor

PenegalECI commented Dec 18, 2017

Description

This PR adds completions for the OpenZFS zfs and zpool commands. It also updates the *.po files and adds corresponding translations for French, my mother tongue. There are merge conflicts, but they only belong, AFAIK, in conflicting metadata in *.po files, due to the POT regeneration I did while others did it on their side. This is only related to POT timestamp, which can be solved using either version, and to translators notes which I added in fr.po, which can be solved using my version, as there is nothing semantically conflicting here.

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

@faho faho added this to the fish-3.0 milestone Dec 18, 2017

@PenegalECI

This comment has been minimized.

Copy link
Contributor

PenegalECI commented Dec 19, 2017

I might add that I did not solve the merge conflicts, even if I could, as Github told me Don’t worry, you can still create the pull request. but, if I have to correct this myself, no problem, just tell me.

@@ -0,0 +1,3 @@
function __fish_complete_zfs_pools -d "Completes with available ZFS pools"
zpool list -o name,comment -H | sed -e 's/\t-//g'

This comment has been minimized.

@faho

faho Dec 19, 2017

Member

Use string replace -a \t'-' '' instead of sed, please.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Did it, but should the \t really be outside the quotes?

This comment has been minimized.

@faho

faho Dec 20, 2017

Member

Yes. string currently does not interpret these sequences itself, so you'll need to pass it a literal tab character. And the way to generate that is to use \t outside of quotes.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Ok, done then.

echo -e "volmode\t"(_ "How to expose volumes to OS")" (default, geom, dev, none)"
end
# User properties; the /dev/null redirection masks the possible "no datasets available"
zfs list -o all ^/dev/null | head -n 1 | string replace -a -r "\s+" "\n" | grep ":" | tr '[:upper:]' '[:lower:]'

This comment has been minimized.

@faho

faho Dec 19, 2017

Member

Try string match -e ":" | string lower

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

string lower? The current doc does not mention this function.

This comment has been minimized.

@faho

faho Dec 20, 2017

Member

It's new since fish 2.7.0, and the online documentation hasn't been updated.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Then, done.

@@ -0,0 +1,18 @@
function __fish_is_zfs_feature_enabled -a feature target -d "Returns 0 if the given ZFS feature is available or enabled for the given full-path target (zpool or dataset), or any target if none given"
set -l pool (echo "$target" | cut -d '/' -f1)

This comment has been minimized.

@faho

faho Dec 19, 2017

Member

string replace -r '/.*' '' -- $target.

The quotes here are superfluous because function -a guarantees that it'll be one element.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

OK, was unsure about that.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Done.

function __fish_is_zfs_feature_enabled -a feature target -d "Returns 0 if the given ZFS feature is available or enabled for the given full-path target (zpool or dataset), or any target if none given"
set -l pool (echo "$target" | cut -d '/' -f1)
set -l feature_name ""
set -l pattern "[[:space:]]"

This comment has been minimized.

@faho

faho Dec 19, 2017

Member

pcre2 (which string uses) has [[:space:]], and also \s.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

You mean I can safely replace | grep "$pattern" by | string match -e '\s' in the rest of this function?

This comment has been minimized.

@faho

faho Dec 20, 2017

Member

-r/--regex, not -e (which is short for --entire, which means for match to print the entire string that contained the match), but yes.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Done.

return 1
end
set -l state (echo $feature_name | cut -f3)
echo "$feature_name" | grep -q -E "(active|enabled)"

This comment has been minimized.

@faho

faho Dec 19, 2017

Member

string match -qr '(active|enabled)' -- $feature_name should work.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Indeed; besides, $state is to be tested, not $feature_name.

echo -e "atime\t"(_ "Update access time on read")" (on, off)"
echo -e "canmount\t"(_ "Is the dataset mountable")" (on, off, noauto)"
set -l additional_algs ''
if test $OS = 'FreeBSD'; or test $OS = 'SunOS'

This comment has been minimized.

@faho

faho Dec 19, 2017

Member

You could also use if contains -- $OS FreeBSD SunOS. Though I can see an argument for symmetry with the other checks.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Oh, your syntax is much better; I was just unsure how to make mine prettier.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Should I also use contains for other alike tests, or can I let test?

This comment has been minimized.

@faho

faho Dec 20, 2017

Member

You can, sure. It's a matter of taste. Here I find it nicer because it makes working with more than one possibility easier. For just one OS, I think I personally would lean towards test, unless of course the list can change, in which case contains is easier to adapt.

Pick whichever you prefer.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Let's just say it's fine as it is.

echo -e "utf8only\t"(_ "Reject non-UTF-8-compliant filenames")" (on, off)"
if test $OS = "Linux"
echo -e "overlay\t"(_ "Allow overlay mount")" (on, off)"
if command -s sestatus >/dev/null ^&1 # SELinux is enabled

This comment has been minimized.

@faho

faho Dec 19, 2017

Member

command has a "-q" option. command -sq sestatus.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

It seems that share/completions/mkdir.fish could use that too.

This comment has been minimized.

@faho

faho Dec 20, 2017

Member

Yes. A bunch could - it's supported since 2.5.0, and we haven't gotten around to modifying all the scripts yet. It's just cleanup, so it's not too high priority.

set feature_name (zpool get all -H | grep "$pattern")
else
set pattern "$pool$pattern$feature$pattern"
set feature_name (zpool get all -H $pool | grep "$pattern")

This comment has been minimized.

@faho

faho Dec 19, 2017

Member

Is this else needed? If $pool is empty, it'll expand to nothing, so both the $pool argument and the "$pool string should just be gone.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

I'm unsure about that, because I had a hard time testing this piece of code; I remember it failed without this else, but why? That, I can't remember. The reason may have vanished in subsequent fish releases, though, as I use 2.4.0.

This comment has been minimized.

@faho

faho Dec 20, 2017

Member

You might have gotten the quoting wrong.

In the set pattern, you want "$pool$pattern..." inside quotes because you want $pool to expand to the empty string. For zpool -H, you want it without quotes (i.e. zpool get all -H $pool, because you want it to expand to no argument.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Oh, wait! Now I remember: without a target specified, I check any available pool, as, in this case, the function only checks if the feature is activated or enabled on the system. But, if a pool or a full-path target is specified, then the function does the check against this specific pool and, then, I have to find the matching line only for the specified pool. So, the else serves to narrow the zfs call to the specified pool if possible, thus decreasing processing time; I don't know if it's worth it, but it doesn't hurt.

if test $status -ne 0 # No such feature
return 1
end
set -l state (echo $feature_name | cut -f3)

This comment has been minimized.

@faho

faho Dec 19, 2017

Member

This can also be echo $feature_name | read -l _ _ state. Which will use space and tab as the delimiter, IIRC.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

I don't understand your replacement code. At this point, $feature_name contains something like this, in tab-separated values:

pgsql   feature@lz4_compress    active  local

How does your suggestion work against this?

This comment has been minimized.

@faho

faho Dec 20, 2017

Member

Do you want the active or local part or both?

read takes variable names (the "-l" is "--local", like in set -l) and splits the line it reads from stdin into those variables, using space and tab as the delimiter if nothing else has been given (via the new "--delimiter" option in 3.0). _ here is just a throwaway variable name (we actually have $_, but its overwritten after each command - it's a bit of a hack currently, but it works and we should allow it).

So

echo "pgsql"\t"feature@lz4_compress"\t"active"\t"local" | read -l _ _ state

writes the first "field" (pgsql) into $_, the second (lz4_compress) into $_ again and the rest (because there's no variable left) into $state (keeping the tab between "active" and "local").

So if you want just the "active" part, you'll have to add one more _ after "state" (read -l _ _ state _), if you want just the "local", add one more _ before, and if you want both in separate variables add another proper variable name.

You might be familiar with something like this from tuple-unpacking in e.g. python or go - result, _ := somefunction()).

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

Oh, nice construction 😍. I'm more Ruby, but I understand the concept; nice one. I was only looking for the third column, so I go with read -l _ _ state _

end

function __fish_zfs_append -d "Internal completion function for appending string to the ZFS commandline"
set str (commandline -tc| sed -ne "s/\(.*,\)[^,]*/\1/p"|sed -e "s/--.*=//")

This comment has been minimized.

@faho

faho Dec 19, 2017

Member

Should translate reasonably well to string - you don't need to mask the parentheses (characters are special by default), so something like

commandline -ct | string replace -rf '(.*,)[^,]*' '$1' | string replace -r -- '--.*=' ''

?

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

I'll try that. Note that __fish_append could be corrected as well, as I essentially borrowed its code and stripped off what was useless to me; that's also why I used sed: I was unsure to fully understand these regexp, so I let them as is.

This comment has been minimized.

@PenegalECI

PenegalECI Dec 20, 2017

Contributor

In fact, it would be a bit difficult to test that, as my current config would not allow me to easily test that; as you know fish way better than me, I assume you are right and blindly replaced my code with yours.

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 19, 2017

if I have to correct this myself, no problem, just tell me.

Nah, we can do that. It's probably just git merge -s theirs anyway (i.e. picking your changes).

@PenegalECI

This comment has been minimized.

Copy link
Contributor

PenegalECI commented Dec 20, 2017

Here you are.

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 21, 2017

Merged as dcf9ce6 with git rebase --interactive -X ours master. (The "-X ours" means that, in case of conflicts, the checked out branch wins. Since I had this PR checked out, it meant that @PenegalECI's changes to the translations were used)

@faho faho closed this Dec 21, 2017

@faho faho added the release notes label Dec 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment