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

[Jeff King <peff@peff.net>]

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

> > +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