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

flag: it is not reasonable not to support "-flag x" for boolean flags. #22961

Closed
dotaheor opened this issue Dec 1, 2017 · 17 comments
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@dotaheor
Copy link

dotaheor commented Dec 1, 2017

The flag standard package says

 Command line flag syntax:

-flag
-flag=x
-flag x  // non-boolean flags only

One or two minus signs may be used; they are equivalent. 
The last form is not permitted for boolean flags because the meaning of the command

cmd -x *

will change if there is a file called 0, false, etc. 
You must use the -flag=false form to turn off a boolean flag.

It is not convinced that this exception is only made for boolean flags.
Why isn't it also made for number and string flags?
It is the same situation for number and string flags.

IMO, it is totally unnecessary to make this exception.
Shell expansions are common and normal.
Go packages shouldn't do anything to avoid some special shell expansions.

[update]
What the package documentation should do is to just let users pay attention to shell expansions.

@bradfitz bradfitz added this to the Go1.11 milestone Dec 1, 2017
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 1, 2017
@neild
Copy link
Contributor

neild commented Dec 1, 2017

If boolean flags always consumed a value, then you would need to write cmd -boolflag true * or cmd -boolflag=true *. This is not what people expect from a boolean flag.

If boolean flags only consumed boolean values, then the number of positional parameters in of cmd -boolflag a b c would vary depending on the value of a. cmd -boolflag * might or might not set -boolflag depending on the expansion of *. This is surprising and hazardous.

Non-boolean flags always consume a value, so there is no ambiguity.

@robpike
Copy link
Contributor

robpike commented Dec 1, 2017

Working as in intended, and as documented in the package-level docs. And as said above, the non-boolean flags don't have the problem because they always consume a value.

@robpike robpike closed this as completed Dec 1, 2017
@dotaheor
Copy link
Author

dotaheor commented Dec 2, 2017

@robpike

Yes, a boolean flag can consume zero or one value. If it is the last flag without consuming a value. It can be followed by a --. If it is not the last one, we don't need do any special things for it.

And how is this problem related to shell expansions. Could you provide a real example?
The example (cmd -x *) provided in the flag standard package is not convincing.

@dotaheor
Copy link
Author

dotaheor commented Dec 2, 2017

@neild

If boolean flags only consumed boolean values, then the number of positional parameters in of cmd -boolflag a b c would vary depending on the value of a. cmd -boolflag * might or might not set -boolflag depending on the expansion of *. This is surprising and hazardous.

If users are aware of the expansion (users really should), it is not surprising and hazardous.
It is users' own responsibility not to use * as argument value for a boolean flag.

@dominikh
Copy link
Member

dominikh commented Dec 2, 2017

ls -l false should list the file named false, not disable long listings.

@dotaheor
Copy link
Author

dotaheor commented Dec 2, 2017

I'm sorry, I think if false is a file, then the correct way to list it is ls -l -- false.
Yes, I know the ls command makes special handling here,
but I don't think Go flag package should learn the handling from ls command.
Is it a shell standard?

btw, the -l flag of the ls command is not allowed specified any argument value at all.
It is not the same situation for go boolean flags.

@docmerlin
Copy link

"Is it a shell standard?"

  • yes, nearly every command behaves this way for boolean flags.

@dotaheor
Copy link
Author

dotaheor commented Dec 2, 2017

yes, nearly every command behaves this way for boolean flags.

could you provide more examples?

As I have mentioned above, for the -l flag of the ls command is not allowed to consume any value at all, so the above mentioned ls -l false example is not a good example.

Shell commands even allow flags appear after non-flag arguments, but Go flags will ignore such flags.
So it looks Go flags have already broken shell standard.

@as
Copy link
Contributor

as commented Dec 2, 2017

So it looks Go flags have already broken shell standard.

Sorry, this is incorrect. Your GNU program reorders the argument vector at runtime and puts your options before your operands. Your expectation of this feature to be ubiquitous doesn't make it the standard, in fact: I would consider it harmful.

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html

Guideline 9:
All options should precede operands on the command line.

@dotaheor
Copy link
Author

dotaheor commented Dec 2, 2017

Ok. But argument vector reordering is NOT the core of the issue.

What problem does the exception the flag package makes try to solve?
Just to avoid cmd -x * being interpreted as cmd -x=false abc xyz if there are really three files false, abc and xyz in the current directory?
For such cases, uses should deserve what they do.
The correct way is to run cmd -x -- * or cmd -x true * or cmd -x=true * instead.

The standard package shouldn't try to fix the mistakes made by users.

In other words, users should be aware of shell expansions.
What about if there are two files in the current directory: -option and aValidOptionValue,
and the program to be run accidentally has an option string flag?
When we run the Go program by ignoring the option flag: cmd *,
shell expansion will add the option for us automatically. This is not what we expect.
Like the current exception made for bool flags, we can apply the exception for string flags too, to solve the problem.
But the current flag package doesn't do this. Why? It is inconsistent.

@dotaheor
Copy link
Author

dotaheor commented Dec 2, 2017

My point is the exception made by the flag package is a bad design.
And the fmt package documentation tries to convince people that it is not a bad design, but fails.
As the Go 1 compatibility can't be broken, my opinion is to view it as a doc bug.
The doc shouldn't use shell expansion as an excuse for the exception, for it is just not convincing.
My recommendation is to warn users that shell expansions may cause unexpected results in doc,
and only show the formal flag usage form is -flag=value.
The doc can also mention that for string and number flags, the = can be omitted in -flag=value.

@ianlancetaylor
Copy link
Contributor

@dotaheor As you say, the flag package is not going to change. What change to the documentation do you suggest? The current flag package documentation does not mention shell expansion.

@dotaheor
Copy link
Author

dotaheor commented Dec 2, 2017

What do the two lines mean?
By the explanations from others above, I think it means * might be expanded to 1 or false.

cmd -x *
will change if there is a file called 0, false, etc.

My last comment shows my suggestion for the doc change.

@rkulla
Copy link

rkulla commented Dec 2, 2017

@dotaheor I take it to mean that * might expand to any of 1, 0, t, f, T, F, true, false, TRUE, FALSE, True, False (which change the value -boolflag); Whereas non-bool values would cause a strconv.ParseBool parse error.

Perhaps the docs could mention the difference between -stringflag * and stringflag=*. For example, if a file named -z exists, the former might set the value of -stringflag to -z but the latter it to the literal "*".

The Go docs should help Go developers. The shell docs (and possibly the authors of an app) should help end users.

For example, most end users are used to flags being booleans, whose non-existence means false and existence means true. They just don't set them explicitly like Go allows (ex: ls -l=true, wc -l=false are illegal). Likewise --l would be illegal rather than acceptable like it is for boolflags in Go.

@as
Copy link
Contributor

as commented Dec 2, 2017

But argument vector reordering is NOT the core of the issue.

Then why did you mention it?

I'm sorry, I think if false is a file, then the correct way to list it is ls -l -- false.

That's incredibly pedantic

I agree with the idea that the package shouldn't mention shell expansion. Some supported platforms don't expand * so the doc would be misleading on those systems.

My last comment shows my suggestion for the doc change

I contest. That's hideous, and it's unfortunate = made it into the package along with GNU long options.

The confusion that I often clarify when approached with the question: "Why are Go flags so ugly" is the similarity of -x y and -x=y and --x=y and --x y.

The fact that -x y isn't the only way to specify that option is a huge pain point when working with others in docker, k8s, and other programs that use that ambiguous style. Go should enforce a consistent command line style, and changing everything to equals signs in the doc isn't the way to cultivate that.

@dotaheor
Copy link
Author

dotaheor commented Dec 3, 2017

The reason I submitted this issue is that I was confused by the two lines

cmd -x *
will change if there is a file called 0, false, etc.

I totally had not any ideas on what the two lines want to express.
I googled it and Google returned this answer: https://stackoverflow.com/questions/46706417/golang-boolean-flag-parsing-restriction/46706559

The stackoverflow answer might not be a good answer.
But the word "file" in the fmt doc really makes an imply to let people think it is related to shell expansion.

I think the word "file" is not essential in the doc.
Another fail of the doc is it doesn't mention bool flags can consume zero or one value at all.

My new suggestion for the doc changes are:

Command line flag syntax: 

-flag // boolean flags only
-flag=x
-flag x  // non-boolean flags only

One or two minus signs may be used; they are equivalent. 
The first form is only permitted for boolean flags. 
The last form is not permitted for boolean flags. 
If it is permitted for boolean flags, the meaning of the command 

cmd -x false

would be ambiguous.

I still think the exception breaks the beauty of consistency.
I think a boolean flag should try to consume most one value.
But I don't think the exception is a big problem.
Yet the docs really confused me much.

@gopherbot
Copy link

Change https://golang.org/cl/82015 mentions this issue: flag: clarify comment to avoid shell syntax confusion

gopherbot pushed a commit that referenced this issue Dec 6, 2017
Updates #22961

Change-Id: Ib2f41aefb4f6470598d8637611da5491156ea840
Reviewed-on: https://go-review.googlesource.com/82015
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Dec 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

10 participants