Re: [PATCH] commit-tree: utilize parse-options api

[Brandon Richardson <brandon1024.br@gmail.com>]

Hi Jeff,

> One of the reasons I did not bother with that condition when I added the
> OPT_NEG() and OPT_ARG() variants is that you can only get an unexpected
> NULL argument if you explicitly give the NOARG or OPTARG flags. So it's
> very easy to _forget_ to give such a flag, because you simply aren't
> thinking about that case, and your callback is buggy by default.
>
> But it's rare to actually think to give one of those flags, but then
> forget to handle it in your callback.
>
> So I'm not entirely opposed, but it does feel weird to add such a macro
> without then using it in the 99% of callbacks which expect arg to be
> non-NULL.

I'd like to agree with you here, especially given that commit-tree is a rather
small part of project source. Experimenting with it a bit, I found using
BUG_ON_OPT_NOARG() to be a big clunky. Like you said, we could
end up with some less-than-ideal usage. If I were to use this in commit-tree,
it would look something like this, which isn't very appealing:

static int callback(const struct option *opt, const char *arg, int unset)
{
     ...
     BUG_ON_OPT_NEG(unset);
     BUG_ON_OPT_NO_ARG(arg);
     ...

However, I do still see a use case for a new macro for options that cannot
be unset and arguments that must not be NULL.

> If we are going to go this route, I think you might actually want macros
> that take both "unset" and "args" and make sure that we're not in a
> situation the callback doesn't expect (e.g., "!unset && !arg"). That
> lets us continue to declare those at the top of the callback.

In doing a quick search, I found a fair number instances of this:
...
BUG_ON_OPT_NEG(unset);

if (!arg)
     return -1;
...

So a macro like this could be useful. I've also found a few instances of this:

BUG_ON_OPT_NEG(unset);
BUG_ON_OPT_ARG(arg);

Perhaps two new macros BUG_ON_OPT_NEG_NO_ARG() ("!unset || !arg")
and BUG_ON_OPT_NEG_ARG() ("!unset || arg")? I'm not a big fan of those
names though.

Brandon