Bug in optflagopt

[pecastro]

I've bumped into some weird behaviour in another project whereas the use of an optflagopt option seemed to render unexpected behaviour of the option not being properly parsed.

I decided to come to the source and I extended the mod.rs test to show that something was amiss
pecastro@de5e4db

I've managed to make the test panic when you use an optflagopt in combination with other options.
Initial investigations from debugging the test show that the vals Vec of the Matches struct will have a Vec with Given rather than the option value specified and that is what's causing the return of None and triggering the panic.

self.vals:

[[(0, Val("foo"))], [(1, Val("bar"))], [], [(2, Given)]]

[ghostd]

Hi,

It seems to be expected:

getopts/src/tests/mod.rs

Lines 412 to 427 in c11eb65

	let long_args = vec!["--test=x".to_string()];
	match opts.parse(&long_args) {
	Ok(ref m) => {
	assert_eq!(m.opt_str("t").unwrap(), "x");
	assert_eq!(m.opt_str("test").unwrap(), "x");
	}
	_ => panic!(),
	}
	let long_args = vec!["--test".to_string(), "x".to_string()];
	match opts.parse(&long_args) {
	Ok(ref m) => {
	assert_eq!(m.opt_str("t"), None);
	assert_eq!(m.opt_str("test"), None);
	}
	_ => panic!(),
	}

[pecastro]

After digging a bit deeper the issue seems to be mainly to do with the parsing of an optflagopt whereby an -o=foo, -o foo, and --option=foo are parsed correctly whereas the --option foo is leaving foo as free an not consuming it as the argument to the option.

[pecastro]

@ghostd Yes, I've seen that whilst I was writing a fix and I'm correcting that test which I think it is wrong in my opinion.
The documentation for the method https://docs.rs/getopts/0.2.21/getopts/struct.Options.html#method.optflagopt doesn't specify that when the long option is used any optional parameter should be ignored ...

[pecastro]

Upon further attentive reading I did spot under the Parsing section of the Linux getopt user command https://man7.org/linux/man-pages/man1/getopt.1.html on the second to last paragraph the rules of engagement regarding the handling of optional arguments.
What tripped me was the absence in this documentation of any similar indication regarding the behaviour.
The https://man7.org/linux/man-pages/man3/getopt.3.html library does not state any such behaviour.

I've close the associated MR for now and I'll close this afterwards if no other insightful comments materialize ...

[correabuscar]

After digging a bit deeper the issue seems to be mainly to do with the parsing of an optflagopt whereby an -o=foo, -o foo, and --option=foo are parsed correctly whereas the --option foo is leaving foo as free an not consuming it as the argument to the option.

This is still true in getopts = "0.2.21" however, the docs wrongly then state:

Single-character options are expected to appear on the command line with a single preceding dash; multiple-character options are expected to be proceeded by two dashes. Options that expect an argument accept their argument following either a space or an equals sign. Single-character options don't require the space.

therefore I expected(wrongly I see) that --option foo would be equivalent to --option=foo, but it is not.

Furthermore, in accordance with that above doc, the usage via opts.usage(&brief) for something declared as opts.opt("u", "unified", "output NUM (default 3) lines of unified contex", "NUM", HasArg::Maybe, Occur::Multi);, states:

Options:
    -u, --unified [NUM] output NUM (default 3) lines of unified contex

as if using a space there, even for the long option, is accepted.

But maybe that's kind of relaxed, and assumes docs were read by whoever uses the binary, i dno, because how would that be shown otherwise?
-u, -u NUM, --unified[=NUM] (but misses -u[NUM], ie. lacking space and NUM being optional)
or
-u[NUM], -u [NUM], --unified[=NUM]
or
-u[ ][NUM], --unified[=NUM] (better, imho, but kind of confusing with that [ ] aka optional space)

That being said, gnu diff does the same thing too and treats it as free arg(in this case as the first filename):

$ /usr/bin/diff --unified 4 a b
/usr/bin/diff: extra operand 'b'
/usr/bin/diff: Try '/usr/bin/diff --help' for more information.

Upon further attentive reading I did spot under the Parsing section of the Linux getopt user command https://man7.org/linux/man-pages/man1/getopt.1.html on the second to last paragraph the rules of engagement regarding the handling of optional arguments.

that's this:

A long option normally begins with '--' followed by the long option name. If the option has a required argument, it may be written directly after the long option name, separated by '=', or as the next argument (i.e., separated by whitespace on the command line). If the option has an optional argument, it must be written directly after the long option name, separated by '=', if present (if you add the '=' but nothing behind it, it is interpreted as if no argument was present; this is a slight bug, see the BUGS). Long options may be abbreviated, as long as the abbreviation is not ambiguous.

therefore I guess the issue happens only for optional arguments of long options, not for required arguments of long options, which in retrospect makes sense now, because how could it know if --has-optional-arg this_arg is its optional arg or just a free arg that follows, whilst compared to --has-optional-arg=this_arg is obvious it's its optional arg.

Maybe the doc needs a little rewording then, to make sure long opts can't expect their optional arg after a space, only after an equals sign. And long opts that require an arg, can expect it after either a space or an equal sign. It's unclear from this Options that expect an argument accept their argument following either a space or an equals sign. that expect there means required argument and if it it did, it doesn't say that the optional argument works with equals sign, so I double expect there had the meaning of required argument.