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

Merge bash_completions changes from upstream #10456

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

dioni21
Copy link
Contributor

@dioni21 dioni21 commented Jun 15, 2020

Motivation and Context

The current bash_completion contrib code in openzfs is very old, and
some changes have been added since.

Description

The original repo is at https://github.com/Aneurin/zfs-bash

I just made a diff and brought it all in a single chunk

How Has This Been Tested?

I've been using the original @Aneurin code since my first deploy of ZoL.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • I have run the ZFS Test Suite with this change applied.
  • All commit messages are properly formatted and contain Signed-off-by.

The current bash_completion contrib code in openzfs is very old, and
some changes have been added since.

The original repo is at https://github.com/Aneurin/zfs-bash

I've been using the original @Aneurin code since my first deploy of ZoL.

Signed-off-by: João Carlos Mendes Luís <jonny@jonny.eng.br>
@dioni21
Copy link
Contributor Author

dioni21 commented Jun 15, 2020

@ggzengel would you please take a look? The last commit in this file was yours, and it motivated me to do the update.

@dioni21
Copy link
Contributor Author

dioni21 commented Jun 15, 2020

BTW: I was not sure if it was safe to keep the shopt -u hostcomplete command in here. Reviewers, what do you think?

@Aneurin
Copy link

Aneurin commented Jun 15, 2020

BTW: I was not sure if it was safe to keep the shopt -u hostcomplete command in here. Reviewers, what do you think?

Just for some context to that, the rationale for disabling hostname completion is a combination of two things:

  • Most people using this will also be using bash_completion, which disables hostcomplete.
  • For everyone else, completion of snapshot names won't work unless hostcomplete is disabled, which is a near-existential problem for ZFS command-line completion.

It's been several years but IIRC there was no other way to opt out of hostcomplete just for a single command. Also I think the old version in practice simply didn't work at all without bash_completion, because the commit that includes this change is titled "Remove dependency on bash_completion" (Aneurin/zfs-bash@1bd8d4f).

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 15, 2020
@behlendorf
Copy link
Contributor

It's good to see these updated and the changes look good to me but I haven't tested them myself. I'm all for pulling them in as long as @Aneurin doesn't have any objection.

Not to be greedy, but we have added several new subcommands over the last few years (zpool checkpoint|initialize|resilver|trim|wait) and it would be nice to update the script in a separate PR to cover them.

@Aneurin
Copy link

Aneurin commented Jun 15, 2020

I certainly have no objection.

I've never used any of the new commands (actually, never heard of some of them) so I've not had an impetus to make any updates for a few years, but I'll review the current manpages with a view to adding the missing commands and options. That's unlikely to happen before the weekend though.

@codecov
Copy link

codecov bot commented Jun 15, 2020

Codecov Report

Merging #10456 into master will decrease coverage by 0.78%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10456      +/-   ##
==========================================
- Coverage   80.24%   79.46%   -0.79%     
==========================================
  Files         293      393     +100     
  Lines       83882   123859   +39977     
==========================================
+ Hits        67313    98427   +31114     
- Misses      16569    25432    +8863     
Flag Coverage Δ
#kernel 80.04% <ø> (-0.21%) ⬇️
#user 65.55% <ø> (?)
Impacted Files Coverage Δ
module/os/linux/spl/spl-zlib.c 55.35% <0.00%> (-28.58%) ⬇️
module/zfs/zcp_set.c 87.09% <0.00%> (-12.91%) ⬇️
module/zfs/zfs_ratelimit.c 90.47% <0.00%> (-9.53%) ⬇️
module/zfs/dmu_redact.c 82.15% <0.00%> (-6.24%) ⬇️
module/zfs/zfs_byteswap.c 79.45% <0.00%> (-5.85%) ⬇️
module/zcommon/zfs_deleg.c 92.45% <0.00%> (-5.55%) ⬇️
module/zcommon/zfeature_common.c 92.85% <0.00%> (-4.82%) ⬇️
module/zfs/dsl_deleg.c 90.07% <0.00%> (-4.48%) ⬇️
module/zfs/zio_inject.c 75.80% <0.00%> (-4.41%) ⬇️
module/zfs/fm.c 36.77% <0.00%> (-4.10%) ⬇️
... and 278 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 883a40f...f041c9c. Read the comment docs.

@dioni21
Copy link
Contributor Author

dioni21 commented Jun 16, 2020

I am super curious, and did check what @Aneurin said:

  • Most people using this will also be using bash_completion, which disables hostcomplete.

Indeed, it is set in my system, Fedora 31, at /usr/share/bash-completion/bash_completion

Maybe we could agree on a safe path:

  • Do not unset hostcomplete in here, let bash_completion be responsible for that
  • Add some readme file explaining that user should unset it if NOT using full bash_completion pack

Sorry for my inexperience, but should I do this as another commit to my branch or as a force push?

Also, what if a subsequent PR could make this installable? Probably best as a separate sub-package. I can do the RPM specs change, but I do not feel comfortable messing with other package systems.

@behlendorf
Copy link
Contributor

Sorry for my inexperience, but should I do this as another commit to my branch or as a force push?

I think it makes sense to merge this as is. I'll go ahead and get it merged right away.

Also, what if a subsequent PR could make this installable? Probably best as a separate sub-package. I can do the RPM specs change, but I do not feel comfortable messing with other package systems.

This we should definitely do as a subsequent PR. Making the change to the RPM spec file should be sufficient since we don't maintain other native packaging. Would we really need a sub-package, is there any harm in installing them as part of the zfs package?

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 16, 2020
@behlendorf behlendorf merged commit fccaea4 into openzfs:master Jun 16, 2020
@dioni21
Copy link
Contributor Author

dioni21 commented Jun 16, 2020

Would we really need a sub-package, is there any harm in installing them as part of the zfs package?

I was afraid to add another dependency (bash-completion), but I just looked into how other packages do, and they add its own completion file without requiring the main framwork. Ok, I'll see into adding it to zfs main package ASAP.

lundman referenced this pull request in openzfsonosx/openzfs Jun 19, 2020
The current bash_completion contrib code in openzfs is very old, and
some changes have been added since.

The original repo is at https://github.com/Aneurin/zfs-bash

I've been using the original @Aneurin code since my first deploy of ZoL.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: João Carlos Mendes Luís <jonny@jonny.eng.br>
Closes #10456
@dioni21 dioni21 deleted the bash_completion branch July 16, 2020 21:00
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
The current bash_completion contrib code in openzfs is very old, and
some changes have been added since.

The original repo is at https://github.com/Aneurin/zfs-bash

I've been using the original @Aneurin code since my first deploy of ZoL.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: João Carlos Mendes Luís <jonny@jonny.eng.br>
Closes openzfs#10456
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
The current bash_completion contrib code in openzfs is very old, and
some changes have been added since.

The original repo is at https://github.com/Aneurin/zfs-bash

I've been using the original @Aneurin code since my first deploy of ZoL.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: João Carlos Mendes Luís <jonny@jonny.eng.br>
Closes openzfs#10456
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants