-
Notifications
You must be signed in to change notification settings - Fork 379
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
Support for setfacl #639
base: main
Are you sure you want to change the base?
Support for setfacl #639
Conversation
Could not find an existing completion script for filesystem acl modification. Adapted from chown. Understands but does not suggest short options.
I've marked this as draft for now because I wasn't sure how to best account for groups that contain colons (as |
Hm, we should add a template for pull requests so that the request to ask upstream to include the completion first is more prominent. Anyway, for time being, see https://raw.githubusercontent.com/scop/bash-completion/master/.github/ISSUE_TEMPLATE/feature_request.md and the start of CONTRIBUTING.md. |
No response from them. Maybe you'd have more luck. |
Still nothing. Did you have any luck? |
I did hear back, but they never confirmed whether they even tried it, and said it had to be sent as a patch. I kinda gave up. |
Re: BASH completion (written but not reviewed)
This means that they will review if you submit it in a proper form. |
@akinomyoga and I don't know what that proper form is, because I don't know where they would want that file in the tree, how it should be included/referenced, whether it needs a It's not complicated to test; no compilation, no install, download and source a single script in bash. I took the response as ‘we'll look at this when you've done it the right way, but you haven't, and we won't tell you exactly what that is, so we won't look at it.’ |
The proper form is just the form of posting an email. There is a specific way of posting a patch in the mailing lists of traditional open-source projects, and there are reasons. However, you didn't follow it, which would cause problems. See other posts on the mailing list. It's not related to how much the actual changes in the patch are correct or not. You should submit a patch with a temporary location of the file and wait for their comments. If they don't like the file location, they should tell you the preferred path. |
I'd say the |
if [[ $cur != *:* && -z $prefix_default && "$other_rules" && ${#COMPREPLY[@]} -eq 1 ]]; then | ||
COMPREPLY=("${other_rules##*:}${COMPREPLY[0]}") | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked the actual behavior, but I guess this has the same problem as #913 (comment) when bind 'set show-all-if-ambiguous on'
is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably wouldn't have been picked up by the setfacl
team, and you'd know better than I how to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no robust way to make the displayed list compact. You'll need to always prefix the other_rules
in the same way as PR #913. For example,
if [[ $cur != *:* && -z $prefix_default && "$other_rules" && ${#COMPREPLY[@]} -eq 1 ]]; then | |
COMPREPLY=("${other_rules##*:}${COMPREPLY[0]}") | |
fi | |
if [[ $cur != *:* && -z $prefix_default && "$other_rules" ]]; then | |
local i | |
for i in "${!COMPREPLY[@]}"; do | |
COMPREPLY[i]=${other_rules##*:}${COMPREPLY[i]} | |
done | |
fi |
But I haven't tried to understand the part OK, I think I now understand it.[[ $cur != *:* && -z $prefix_default ]]
, so it might not result in an expected result. Could you explain the condition [[ $cur != *:* && -z $prefix_default ]]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re #913:
I'm not sure why the original only prepended the prefix back when there was only one completion, so I'm worried I'm missing something.
My understanding was that omitting the prefix when there are multiple candidates makes the list more concise, thus easier to visually scan & less likely to cause pagination. It also provides the ability to create richer results, as demonstrated in this article.
Without your suggestion the behaviour is like this:
roy@RoyBook:~$ setfacl --set user:w
whoopsie: www-data:
roy@RoyBook:~$ setfacl --set user:whoopsie:rw
rw, rwx, rwX,
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,group:a
adm: audio: avahi: avahi-autoipd:
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,group:adm:
-, r, rw, rwx, rwX, rx, rX, w, wx, wX, x, X,
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,group:adm:rw,
default: group: mask: other: user:
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,group:adm:rw,other:
-, r, rw, rwx, rwX, rx, rX, w, wx, wX, x, X,
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,group:adm:rw,other:r
With it, the previous definition's mode is prepended to the next definition, which is a drop in readability:
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,
rwX,default: rwX,group: rwX,mask: rwX,other: rwX,user:
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,group:a
adm: audio: avahi: avahi-autoipd:
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,group:adm:
-, r, rw, rwx, rwX, rx, rX, w, wx, wX, x, X,
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,group:adm:rw,
rw,default: rw,group: rw,mask: rw,other: rw,user:
I tried to replicate the prefix loss per the last snippet of your comment, but it did not seem to occur. Perhaps this function is not affected due to the slightly different behviour around colons?
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,group:avahi
avahi: avahi-autoipd:
roy@RoyBook:~$ setfacl --set user:whoopsie:rwX,group:av<TAB>
@akinomyoga Exactly my point. I don't know what |
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
That's not the problem. The first patch doesn't need to be at an acceptable level. It is normal to revise the patch multiple times after discussing each version of the submitted patch. It just needs to be submitted in a proper form. I repeat that the proper form is not about the content of the patch. You provided the change with a link. This is one of the most problematic ways of posting anything on the mailing list. Links are in general considered to become unavailable in the future. On the other hand, we often need to look back on the mailing list log to investigate the background of the codebase, so all the discussions need to be trackable, i.e., all the necessary information should be included in the emails themselves. Otherwise, they can't even start a discussion. You are blocking the discussion there. |
Thanks for the updates! |
Honestly, this is the problem of the upstream acl project. It's not your fault. If you would like to communicate with them, you need to be patient. I'm not so enthusiastic to make it include the upstream. |
Could not find an existing completion script for filesystem acl modification and I don't see any completion code in the upstream acl project.
Adapted from chown. Understands but does not suggest short options.