Re: [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks

From: Jeff King <peff@peff.net>
Date: Aug 31, 2023, 17:56 UTC
Message-ID: <20230831175638.GC3208283@coredump.intra.peff.net>
In-Reply-To: <xmqq4jkfxhx2.fsf@gitster.g>

On Thu, Aug 31, 2023 at 10:04:09AM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > There are a few parse-option callbacks that do not look at their "unset"
> > parameters, but also do not set PARSE_OPT_NONEG. At first glance this
> > seems like a bug, as we'd ignore "--no-if-exists", etc.
> >
> > But they do work fine, because when "unset" is true, then "arg" is NULL.
> > And all three functions pass "arg" on to helper functions which do the
> > right thing with the NULL.
> 
> Yuck.  That is ugly.

Yep. I wondered about adding a comment here warning about the situation, but it felt kind of content-less. Something like:

  /* if unset, arg is NULL and handled below */
  trailer_set_where(opt->value, arg);

Show 8 quoted lines

> > Note that this shortcut would not be correct if any callback used
> > PARSE_OPT_NOARG (in which case "arg" would be NULL but "unset" would be
> > false). But none of these do.
> 
> That is even uglier.  Unlike the BUG_ON_OPT_NEG() and BUG_ON_OPT_ARG()
> that catch discrepancies between options[] flags and the expectation
> by the callback function, there is no way for us to protect against
> such mistakes?

I guess it would be something like:

  if (!unset && !arg)
	BUG("unexpected use of PARSE_OPT_OPTARG");

I think it is less important than those other ones, though, because the mistake here is the OPT_CALLBACK declaration adding a flag that the callback is not prepared to handle. Whereas in the other ones, the bug is that the declaration _forgot_ to use a flag, which is a much more likely bug.

So I dunno. If it were more than this one case (well, three, but they are all of the same form) I'd be more worried.

-Peff