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

Add support for GNU-style command line options #107

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

Conversation

kofuk
Copy link

@kofuk kofuk commented Nov 24, 2020

xclip supports only -option style option, but it's common for Unix command line tools to have
GNU style command line options (i.e. --option style.)

This PR adds support for GNU style command line option, keeping compatibility to Xrm* functions by removing
first hyphen if GNU style command line options are passed.

@hackerb9
Copy link
Collaborator

hackerb9 commented Dec 9, 2020

Your change looks reasonable and it is almost exactly what I was intending to add to close bug #1.

Have you thought about the behaviour in the very unlikely and definitely pathological, yet possible, case where a person is trying to read from a file named "--something--"? I don't see a simple and clean fix for that. Do you?

Personally, I think it is a bad idea to touch a lot of code to support GNU style long options as it could lead to complexity and breakage later. If you can't think of a succinct solution, my inclination is to merge your change and just add a BUG to the man page saying, "if your filename begins with two dashes, prefix it with ./ or xclip will try to read from the wrong file". I believe the real benefits to actual users outweighs the possible confusion to hypothetical ones.

Also, before this gets merged, I'd like @astrand to weigh in and give a thumbs up. Not only does this cause a new, albeit extremely minor, deficiency in xclip, I notice @astrand self-assigned fixing #1 and #16 back in 2016.

@@ -176,6 +176,14 @@ int clean_requestors() {
static void
doOptMain(int argc, char *argv[])
{
/* Support both -option and --option form. */
int i;
for (i = 1; i < argc; ++i) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For code hygiene, I suggest declaring the counter variable within the for loop. E.g., for (int i = 1 ...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it should be compiled with ANSI C compiler. I'll use for (int i =... style if it allowed.

@astrand
Copy link
Owner

astrand commented Dec 10, 2020

The common way of supporting a file name called --something is to treat everything after "--" as file names. See https://www.gnu.org/software/libc/manual/html_node/Using-Getopt.html .

One problem with support both -option and --option is that this is not a full GNU-style support: -option should mean the same thing as "-o -p -t -i -n". But it has been done before, see https://linux.die.net/man/1/vncviewer.

No easy solution so this problem, unfortunately, if we want to preserve backwards compatibility.

@kofuk
Copy link
Author

kofuk commented Jan 13, 2021

Sorry for slow reply.

Treating options after -- as non-option argument seems a good way to cope with filenames starting with --.
Fortunately, xclip doesn't seem using a pattern of option that this solution can't treat -- ones like --option --filename-- where --option requires an argument.
(FYR, GNU-compatible option parser treats --filename-- as an argument for --something)

I think it's not a very big problem that -option is not interpreted as -o -p -t -i -o -n, as GNU C library also provides getopt_long_only(3) function.

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.

3 participants