On Thu, Aug 31, 2023 at 10:04:09AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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);
> > 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
