On 11/02/2012 12:49 PM, Ben Pfaff wrote: > On Mon, Oct 29, 2012 at 03:03:13PM -0500, Adam Heath wrote: >> On 10/29/2012 11:34 AM, Ben Pfaff wrote: >>> Until now, a command like "ovs-vsctl --may-exist add-br br0" yielded a >>> confusing error message. Users had to realize that the correct form was >>> "ovs-vsctl -- --may-exist add-br br0", but instead they often reported a >>> bug or gave up in frustration. Even though the behavior was documented, it >>> was counterintuitive. >>> >>> This commit allows command-specific options to be mixed with global >>> options, making both forms of the command listed above equally acceptable. >> >> Tbh, I would actually prefer to have command-specific options that >> appear in the global area issue an error. >> >> == >> one-vsctl: Found command-specific --may-exist in global area, please >> use -- instead. >> == >> >> See (1) for a reason why I'd prefer it the way I suggested above. >> >> 1: http://lwn.net/Articles/520198/ >> you might need to wait a few days for it to become publically >> available. > > OK, it's available now, and I skimmed over it, but the analogy to > ovs-vsctl isn't really obvious to me. Can you elaborate?
Providing variants for valid parameter processing makes it harder to understand. Instead, a warning/error should be issued saying the arg is not understood(and if it appears to be a per-cmd option in global space, then a message saying such should be displayed), but allowing for per-option to work for global is actually *breaking* existing scripts that error out. Any existing script that places --may-exist in global space, without --, will currently have an error. Your patch changes that. So, that is bad. Instead, if you just change the error message displayed in those cases, then there isn't any breakage. The article applies to this senario, because your new patch will break existing scripts. The article also applies because ovs-vsctl should have had better handling of border conditions in arg processing, by providing more concise error messages. == ovs-vsctl: Found an unrecognized option(--may-exist) in the global option area; perhaps you need to use -- to start a per-command option area? == -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
