https://bugzilla.redhat.com/show_bug.cgi?id=2249375 Ben Beasley <code@xxxxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+ --- Comment #7 from Ben Beasley <code@xxxxxxxxxxxxxxxxxx> --- (In reply to Antoine Damhet from comment #6) > Hi Ben, I updated the package to take into account the tox/license issues > but I > don't agree with the `/usr/bin/pass-audit` file. Since it's a plugin of > `pass` > the user should call the entrypoint via `pass audit <options>` (that will > make > pass load and call the `/usr/lib/password-store/extensions/audit.bash` file). This makes sense. I agree, and retract the finding about the entry point. > The man page correctly references the `pass audit <xx>` command as for the > completion files the bash completion file is working as expected but looking > closer into it I think the zsh completion is broken: > > It triggers with `pass-audit` and the output looks like: > > ``` > $ pass-audit zsh: _pass_complete_entries_with_subdirs: command not found... > -zsh: _pass_complete_entries_with_subdirs: command not found... > pass-audit - > --help -h -- display help information > --name -n -- check only passwords with this filename > --quiet -q -- be quiet > --verbose -v -- be verbose > --version -V -- display version information > ``` > > I'll look into fixing it but I don't know how yet, can we move forward with > the > package ? If so, should I remove the zsh completion file while it's broken ? Hmm. I tried installing the package into a mock chroot, and I can reproduce what you found with the zsh completions. The zsh error goes away if you try tab-completion on pass before trying it on pass-audit, which makes sense because the _pass_complete_entries_with_subdirs function is in /usr/share/zsh/site-functions/_pass, so it’s only found if that script is autoloaded first. However, it still triggers on pass-audit rather than "pass audit". The bash completions seem to be subtly broken too. I can only get completions for "pass audit" to work (both "audit" showing up as a completion for "pass", and the proper options for "pass audit" to show up) if I have already attempted to complete the non-existent "pass-audit" command in that particular shell. I think it will be hard to trigger the zsh error by accident while doing something other than investigating the completions, so I think you can make your own choice as to whether install the broken zsh completions or omit it for now. The bash completions are definitely worth installing, but it would also be nice to get them working properly, as they’ll just be quietly broken in almost all cases (since people will not try completing pass-audit first). Maybe upstream will have some insight into what’s going wrong here. > Thanks for the review anyway :) You’re welcome. Looking at the spec-file diff for your updated submission, I see that you dropped -t from %pyproject_buildrequires, added -l to %pyproject_save_files, and dropped the manual license-file listing. Since these changes address the suggestions from the original review, and since my finding about a /usr/bin/pass-audit file was off base, the package is now APPROVED. -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2249375 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202249375%23c7 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
