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

comp_compgen: option -C no longer checks if OPTARG is empty #1290

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

YoruStar
Copy link

@YoruStar YoruStar commented Dec 12, 2024

The -C option in _comp_compgen, similar to the -P option in bash's compgen, should support passing an empty string, for example: compgen -P ""-f

fix #1260

@akinomyoga
Copy link
Collaborator

akinomyoga commented Dec 12, 2024

I believe this should be fixed at the side of completions/_umount.linux. It is in general suspicious to specify an invalid directory name so I don't think it is a good idea to suppress the error message unconditionally. The directory name should be checked at the caller side. In the case of _umount.linux, it seems that the error still had occurred before 6b3dfa5, but the message was redirected to /dev/null as cd is performed as command cd -- "$dircur" 2>/dev/null. The caller should ensure the validity of the path. In the present case, the caller can be updated to _comp_compgen -aC "${dircur:--}" ...

@YoruStar
Copy link
Author

YoruStar commented Dec 13, 2024

I believe this should be fixed at the side of completions/_umount.linux. It is in general suspicious to specify an invalid directory name so I don't think it is a good idea to suppress the error message unconditionally. The directory name should be checked at the caller side. In the case of _umount.linux, it seems that the error still had occurred before 6b3dfa5, but the message was redirected to /dev/null as cd is performed as command cd -- "$dircur" 2>/dev/null. The caller should ensure the validity of the path. In the present case, the caller can be updated to _comp_compgen -aC "${dircur:--}"

I tried command cd -- "" and it did not fail.

$ command cd -- ""
$ echo $?
0

In the changes at 6b3dfa5, there are many modifications besides _umount.linux, and these may also potentially have the same issue.

Moreover, in the _comp_compgen__call_generator function, there is already a check to determine if the $_dir variable is empty, making the logic for checking if $OPTARG is empty in the -C option somewhat redundant.

Of course, aside from the umount command, I haven't found similar issues elsewhere, so modifying _umount.linux alone is also feasible.

@akinomyoga
Copy link
Collaborator

I tried command cd -- "" and it did not fail.

$ command cd -- ""
$ echo $?
0

OK, thanks.

Nevertheless, I don't think it would be right just to suppress the error message when one sees an error message.

In the changes at 6b3dfa5, there are many modifications besides _umount.linux, and these may also potentially have the same issue.

Yes, they should also be fixed if they would pass an invalid directory. Before my first reply, I actually had checked it after I saw your PR. The cases for feh and sbopkg should also be fixed properly. The other cases would probably be fine, but we need to double-check them.

Moreover, in the _comp_compgen__call_generator function, there is already a check to determine if the $_dir variable is empty,

No, it's used to check whether the -C option is specified or not. In the current master, exactly because we reject an empty directory, we can use [[ $_dir ]] to check whether the -C option is specified. If you break the assumption, it doesn't work as expected, so one needs to use another variable to store whether the -C option is specified.

making the logic for checking if $OPTARG is empty in the -C option somewhat redundant.

This is false. It's the opposite. It relies on the fact that the empty directory for -C is rejected.

Of course, aside from the umount command, I haven't found similar issues elsewhere, so modifying _umount.linux alone is also feasible.

Could you also look at feh and sbopkg? I haven't looked at them closely, but they might also need modifications. If you wouldn't have time, I'll later look at them someday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error occurred when completing the "umount" command
2 participants