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

From: Jeff King <peff@peff.net>
Date: Feb 27, 2019, 16:35 UTC
Message-ID: <20190227163522.GA25188@sigill.intra.peff.net>
In-Reply-To: <CACsJy8Bgz6FiTqnq8pnebuyOr55Bqh67iRhr6J+WvzgxPSBLhw@mail.gmail.com>

On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:

Show 18 quoted lines

> > +static int parse_parent_arg_callback(const struct option *opt,
> > +               const char *arg, int unset)
> > +{
> > +       struct object_id oid;
> > +       struct commit_list **parents = opt->value;
> > +
> > +       BUG_ON_OPT_NEG(unset);
> > +
> > +       if (!arg)
> > +               return 1;
> 
> This "return 1;" surprises me because I think we often just return 0
> or -1. I know !arg cannot happen here, so maybe just drop it. Or if
> you want t play absolutely safe, maybe add a new macro like
> 
> BUG_ON_NO_ARG(arg);
> 
> which conveys the intention much better.

I think it should be spelled BUG_ON_OPT_NOARG() to match the other ones.

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.

Actually, there is one subtlety, which is that it can be NULL if "unset" is true. But then callbacks should already be looking at "unset" or using BUG_ON_OPT_NEG(). But that just makes things worse. Take parse_opt_patchformat(), for example. It _does_ check "unset", so should not use BUG_ON_OPT_NEG(). But if "!unset", it expects "arg" to be non-NULL. So adding an assertion there turns our nice cascade of conditionals:

  if (unset)
	...handle unset...
  else if (!strcmp(arg, "foo"))
	...handle "foo"...
  ...and so on...

into:

  if (unset)
	...handle unset...
  else {
	BUG_ON_OPT_NOARG(arg);
	if (!strcmp, "foo"))
		....
	... and so on...
  }

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.

But as you can see, it gets complicated quickly. I'm not really sure it's worth the trouble for a maintenance problem that's relatively unlikely.

-Peff