search

savonet / ocaml-posix

Bindings to the various POSIX APIs
https://www.liquidsoap.info/ocaml-posix/
MIT License
27 stars 2 forks source link

Fix Unknown_option exception when using getopt_long #5

Closed wizeman closed 3 years ago

wizeman commented 3 years ago

When using getopt_long and the user passed a long unknown option, the Unknown_option exception would carry a NULL/zero character.

Now it should carry a string representing the actual unknown option.

Fixes #4

wizeman commented 3 years ago

This was harder than I expected because optind behaves differently when calling getopt with an unknown short option vs an unknown long option (or getopt_long with an unknown short or long option).

That is, depending on whether you're calling getopt or getopt_long and whether the user passed an unknown short or unknown long option, when getopt* returned ? then optind would be pointing to the actual unknown option, or to the option right after the unknown one.

So the only way I could get consistent behavior was to check whether optopt had a NULL value; if so, then I could assume optind was pointing to the option right after the unknown one. Otherwise, we will simply return the content of optopt, just like before, but converted to a string.

This means that if you're using getopt but the user passed an unknown long option, then the exception Unknown_option "--" will be raised, which is not perfect but it's not worse than what was happening before (where the exception would just contain the '-' character).

wizeman commented 3 years ago

BTW, I only tested this on NixOS (with glibc). I was going to say that I'm not entirely sure that this will work correctly on all getopt implementations, but in fact, when using the getopt function this is working just like it was working before, as far as I can tell (except that the unknown option character gets converted to a string).

The only difference should be when getopt* returns a ? and optopt contains a NULL character, which should only happen when using getopt_long on Linux (which is a non-standard implementation, I suppose, and which I've properly tested).

toots commented 3 years ago

That's awesome! Could you rebase or merge the latest master, this should start running the tests.

wizeman commented 3 years ago

That's awesome! Could you rebase or merge the latest master, this should start running the tests.

Done! I think you need to approve the workflow for the tests to run, though.

toots commented 3 years ago

Merging, thanks!